Shireesh Anjal has posted comments on this change. Change subject: engine: Method to fetch SSH fingerprint of server ......................................................................
Patch Set 3: Responses to Alon's comments below. New patch-set to follow. "You already have the server key in serverKey after connection, no need to extract this as you did." >> Got it. Will change the code to use serverKey received in the verifier. Note >> that it is received only if we wait for ClientSession.WAIT_AUTH (or actually >> perform authentication). So I'm going to add this 'waitFor()' call inside >> the 'createSession()' method. "There is no reason to disconnect if user approves." >> Fingerprint will be fetched and verified 'before' the actual bootstrapping >> starts. The bootstrapping will be triggered from another UI action - a >> totally different request to the engine. So I don't think the same instance >> of MinaInstallWrapper will be used in both requests, and hence felt it is >> important to close the connection created here. However I do understand that >> in this case, the connection will be closed by the finalize method whenever >> the object is garbage collected. Though normally I wouldn't have wanted to >> wait till that happens, I'm going to remove the call to wrapperShutdown(), >> so that this method looks consistent with all other methods of the >> MinaInstallWrapper. "Either a callback is appropriate or connect can have two phases, connectOnly and authenticate." >> I'm not sure what kind of callback you are suggesting. I've already >> extracted the 'connectOnly' part into a method 'createSession()' which is >> now used from the existing method 'connect()' as well as the new one >> 'getFingerprint()'. In any case, let me send another patch-set as per the >> comments above, and you can then tell me if you want me to change some more. "BTW: I don't think adding features that are not used is good... These kind of changes should go with the functionality that actually uses it." >> a) Why do you think so? Is that a standard practice in oVirt development? I >> personally prefer working in an agile way - incrementally adding useful >> functionality to the system, as long as it adds value. e.g. While the >> primary use of getFingerprint() method will be in the 'import gluster >> cluster' feature we are working on, it could also be consumed parallaly by >> someone else to enhance the current 'add host' flow to display the >> fingerprint before triggering the installation. I've been told that it is >> very much required there as well. b) There have been instances where my other patches have received comments to break them down into smaller multiple patches so that they are easier to review. c) While I've written this code to fetch the fingerprint, a different developer is working on consuming it for the actual feature. Would you like a single patch with code written by two developers? -- To view, visit http://gerrit.ovirt.org/6927 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I398fac6cbf641b49c8281937280ae0c351e1d3b6 Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Shireesh Anjal <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Dhandapani Gopal <[email protected]> Gerrit-Reviewer: Doron Fediuck <[email protected]> Gerrit-Reviewer: Omer Frenkel <[email protected]> Gerrit-Reviewer: Shireesh Anjal <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
