Change in vdsm[master]: vdsm_config: move download certificate

dougsland at redhat.com dougsland at redhat.com
Wed May 7 03:33:33 UTC 2014


Douglas Schilling Landgraf has posted comments on this change.

Change subject: vdsm_config: move download certificate
......................................................................


Patch Set 26:

(4 comments)

http://gerrit.ovirt.org/#/c/26718/26/vdsm_reg/vdsm-reg-setup.in
File vdsm_reg/vdsm-reg-setup.in:

Line 213:                 "Engine fingerprint [%s]",
Line 214:                 self.cfg_fprint.upper(),
Line 215:                 engine_fprint
Line 216:             )
Line 217:             if File(ca_path).exists():
> os.path.exists() should be used these days, I think.
I was running away from it but I am ok as well to use static "/config" in os.path.exists to check if file is persisted.
Line 218:                 ovirtfunctions.ovirt_safe_delete_config(
Line 219:                     ca_path
Line 220:                 )
Line 221:             os.unlink(ca_path)


Line 217:             if File(ca_path).exists():
Line 218:                 ovirtfunctions.ovirt_safe_delete_config(
Line 219:                     ca_path
Line 220:                 )
Line 221:             os.unlink(ca_path)
> this should be indented, no reason to unlink if not exists.
Agreed
Line 222: 
Line 223:         return ret
Line 224: 
Line 225:     def execute(self):


Line 234:             fOK = self.renameBridge()
Line 235:             logging.debug("execute: after renameBridge: %s", fOK)
Line 236: 
Line 237:         if fOK and os.path.exists(self.engineWebCACert) and \
Line 238:                 self.cfg_fprint != "None":
> move the "None" into the function, always return true and drop this != "Non
If you don't mind I prefer to stay with this cfg_fprint. If we don't have cfg_fprint no need to use _compare_fingeprint
Line 239:             self._compare_fingerprint(self.engineWebCACert)
Line 240: 
Line 241:         if fOK and not os.path.exists(self.engineWebCACert):
Line 242:             fd_tmp, file_tmp = tempfile.mkstemp()


Line 259:                     )
Line 260:                     fOk = False
Line 261: 
Line 262:                 if fOK and self.cfg_fprint != "None" and \
Line 263:                         download_certificate_succeeded:
> if you move the "None" to function all it should be should be:
We need the download_certificate_succeeded otherwise 80 port will achieve here because of all fOK until here.
Line 264:                     if not self._compare_fingerprint(file_tmp):
Line 265:                         fOK = False
Line 266:                         sys.exit(1)
Line 267: 


-- 
To view, visit http://gerrit.ovirt.org/26718
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2c025eedd2be92b9418ddbe01efc02c913af2a7
Gerrit-PatchSet: 26
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf <dougsland at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list