Alon Bar-Lev has posted comments on this change. Change subject: core: Adds service configuration based on provided JBoss ......................................................................
Patch Set 4: (7 comments) https://gerrit.ovirt.org/#/c/40152/4/packaging/services/ovirt-engine/ovirt-engine.py File packaging/services/ovirt-engine/ovirt-engine.py: Line 415 Line 416 Line 417 Line 418 Line 419 here, please add the -c, put the jbossConfigFile in object state. args=self._engineArgs + [ '-c', os.path.basename(self._jbossConfigFile), ], Line 231: return major, minor, revision Line 232: Line 233: def _detectJBossVersion(self, engineArgs): Line 234: detectVersion = [] Line 235: detectVersion.extend(engineArgs) better: detectVersion = engineArgs + ['-v'] btw: even if you want to copy, then: detectVersion = engineArgs[:] Line 236: detectVersion.extend([ Line 237: '-v' Line 238: ]) Line 239: proc = subprocess.Popen( Line 237: '-v' Line 238: ]) Line 239: proc = subprocess.Popen( Line 240: executable=self._executable, Line 241: args=detectVersion, no reason why not: args=engineArgs + ['-v'], and drop temp var. hmmm... or better remove the argument and use: args=self._engineArgs + ['-v'], Line 242: env=self._engineEnv, Line 243: stdout=subprocess.PIPE, Line 244: stderr=subprocess.PIPE, Line 245: close_fds=True, Line 253: "Return code: %s, \nstdout: '%s, \nstderr: '%s'", Line 254: proc.returncode, Line 255: stdout, Line 256: stderr, Line 257: ) if proc.returncode is non zero we should probably fail, no? Line 258: Line 259: major, minor, revision = self._parseJBossVersion(stdout) Line 260: if major is None: Line 261: raise RuntimeError(_('Cannot detect JBoss version')) Line 257: ) Line 258: Line 259: major, minor, revision = self._parseJBossVersion(stdout) Line 260: if major is None: Line 261: raise RuntimeError(_('Cannot detect JBoss version')) this exception should be raised from the function. Line 262: Line 263: self.logger.debug( Line 264: "Detected JBoss version: %s.%s.%s", Line 265: major, Line 457: 'ENGINE_VAR': self._config.get('ENGINE_VAR'), Line 458: 'ENGINE_CACHE': self._config.get('ENGINE_CACHE'), Line 459: }) Line 460: Line 461: self._detectJBossVersion(self._engineArgs) why are you passing object state as parameter? Line 462: Line 463: jbossConfigFile = self._processTemplate( Line 464: template=os.path.join( Line 465: os.path.dirname(sys.argv[0]), Line 471: Line 472: # Add argument to execute ovirt-engine instance Line 473: self._engineArgs.extend([ Line 474: '-c', os.path.basename(jbossConfigFile), Line 475: ]) please move this to daemon context, keep the engineArgs pure, see bellow. Line 476: Line 477: def daemonStdHandles(self): Line 478: consoleLog = open( Line 479: os.path.join( -- To view, visit https://gerrit.ovirt.org/40152 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic35f8a0c276735b9685affea1e068f6ef7298f8c Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Peřina <mper...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Martin Peřina <mper...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Sandro Bonazzola <sbona...@redhat.com> Gerrit-Reviewer: Simone Tiraboschi <stira...@redhat.com> Gerrit-Reviewer: Yedidyah Bar David <d...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
