Martin Peřina has posted comments on this change. Change subject: core: Adds service configuration based on provided JBoss ......................................................................
Patch Set 4: (8 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. Done Line 220: break Line 221: elif 'WildFly' in line: Line 222: # Line: WildFly 8.1.0.Final "Kenny" Line 223: versionStr = parts[1] Line 224: break > won't it better to parse it using regular expression? Done Line 225: if versionStr is not None: Line 226: parts = versionStr.split('.') Line 227: major = parts[0] Line 228: minor = parts[1] Line 237: '-v' Line 238: ]) Line 239: proc = subprocess.Popen( Line 240: executable=self._executable, Line 241: args=detectVersion, > no reason why not: Done 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? It's a bit problematic, because currently all tested JBoss versions returned 1 when '-v' parameter was used to determine version 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. Done Line 262: Line 263: self.logger.debug( Line 264: "Detected JBoss version: %s.%s.%s", Line 265: major, Line 270: self._config.values.update({ Line 271: 'JBOSS_MAJOR': major, Line 272: 'JBOSS_MINOR': minor, Line 273: 'JBOSS_REVISION': revision, Line 274: }) > it should not be part of config, better to use a different object/function Done Line 275: Line 276: def daemonSetup(self): Line 277: Line 278: if os.geteuid() == 0: 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? Done 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. Done 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: Jenkins CI 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-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
