Alon Bar-Lev has posted comments on this change. Change subject: core: Adds service configuration based on provided JBoss ......................................................................
Patch Set 5: (14 comments) https://gerrit.ovirt.org/#/c/40152/5/packaging/services/ovirt-engine/ovirt-engine.py File packaging/services/ovirt-engine/ovirt-engine.py: Line 35: Line 36: _JBOSS_VERSION_REGEX = re.compile( Line 37: flags=re.VERBOSE, Line 38: pattern=r"[^\d]*(?P<major>\d+)\.(?P<minor>\d+).(?P<revision>\d+).*", Line 39: ) verbose re should be as: pattern=r""" ^ [^\d]* (?P<major>\d+) \. (?P<minor>\d+) \. (?P<revision>\d+) .* """, 1. easier to follow 2. notice the ^ at prefix 3. notice the \. Line 40: Line 41: def __init__(self): Line 42: super(Daemon, self).__init__() Line 43: self._tempDir = None Line 225: Line 226: if major is None: Line 227: raise RuntimeError(_('Cannot detect JBoss version')) Line 228: Line 229: return major, minor, revision why don't you set and return the dictionary? I am not sure why this is a function, it used one time without the _detectJBossVersion, so belongs to there. Line 230: Line 231: def _detectJBossVersion(self): Line 232: proc = subprocess.Popen( Line 233: executable=self._executable, Line 248: stdout, Line 249: stderr, Line 250: ) Line 251: Line 252: major, minor, revision = self._parseJBossVersion(stdout) if within function or not, you should return the dictionary and you can debug the dictionary as-is, eg: "%s", dict Line 253: Line 254: self.logger.debug( Line 255: "Detected JBoss version: %s.%s.%s", Line 256: major, Line 261: self._jbossVersion.update({ Line 262: 'JBOSS_MAJOR': major, Line 263: 'JBOSS_MINOR': minor, Line 264: 'JBOSS_REVISION': revision, Line 265: }) not sure why update, it is a must for our process, so you can set it here only, you can put None in constructor if you wish. Line 266: Line 267: def daemonSetup(self): Line 268: Line 269: if os.geteuid() == 0: Line 503: pass Line 504: # if self._tempDir: Line 505: # self._tempDir.destroy() Line 506: # if self._jbossRuntime: Line 507: # self._jbossRuntime.destroy() ? Line 508: Line 509: Line 510: if __name__ == '__main__': Line 511: service.setupLogger() https://gerrit.ovirt.org/#/c/40152/5/packaging/services/ovirt-engine/ovirt-engine.xml.in File packaging/services/ovirt-engine/ovirt-engine.xml.in: Line 142 Line 143 Line 144 Line 145 Line 146 can you please submit a base patch for the reformatting, so we can review this patch easier? Line 2: Line 3: #if $JBOSS_MAJOR < 7 Line 4: <server xmlns="urn:jboss:domain:1.1"> Line 5: #else Line 6: <server xmlns="urn:jboss:domain:2.1"> I wounder if jboss-8 does not support 1.1 Line 7: #end if Line 8: <extensions> Line 9: <extension module="org.jboss.as.clustering.infinispan"/> Line 10: <extension module="org.jboss.as.connector"/> Line 67: <properties path="/dev/null"/> Line 68: </authentication> Line 69: </security-realm> Line 70: Line 71: #if $JBOSS_MAJOR >= 7 and $getboolean('ENGINE_HTTPS_ENABLED') you do not need the $getboolean('ENGINE_HTTPS_ENABLED'), the realm can be defined in any case even if it is not used. I wounder if the realm cannot be defined in any case for jboss eap as well. Line 72: <!-- This is required by the HTTPS listener: --> Line 73: <security-realm name="https"> Line 74: <server-identities> Line 75: <ssl> Line 156: </logger> Line 157: <logger category="jacorb.config"> Line 158: <level name="ERROR"/> Line 159: </logger> Line 160: #end if why do we need conditional for this? we can just install all logger, you can put a comment such as: TODO: remove when eap is no longer supported. but there is no need to add complexity. Line 161: Line 162: <!-- Loggers for the engine: --> Line 163: <logger category="org.ovirt" use-parent-handlers="false"> Line 164: <level name="INFO"/> Line 304: managed-executor-service="java:jboss/ee/concurrency/executor/default" Line 305: managed-scheduled-executor-service="java:jboss/ee/concurrency/scheduler/default" Line 306: managed-thread-factory="java:jboss/ee/concurrency/factory/default"/> Line 307: </subsystem> Line 308: #end if is this a must? doesn't it support the ee:1.0? or at least most of the above as default? Line 309: Line 310: #if $JBOSS_MAJOR < 7 Line 311: <subsystem xmlns="urn:jboss:domain:ejb3:1.2"> Line 312: <session-bean> Line 392: <default-missing-method-permissions-deny-access value="true"/> Line 393: </subsystem> Line 394: #end if Line 395: Line 396: #if $JBOSS_MAJOR < 7 same here Line 397: <subsystem xmlns="urn:jboss:domain:infinispan:1.1" default-cache-container="ovirt-engine"> Line 398: <cache-container Line 399: name="ovirt-engine" Line 400: default-cache="timeout-base" Line 491: <expose-resolved-model/> Line 492: <expose-expression-model/> Line 493: <remoting-connector/> Line 494: </subsystem> Line 495: #end if same, actually all subsystems... doesn't jboss-8 support the older subsystems? and if it does not, doesn't it support most of what you add as default per subsystem? Line 496: Line 497: #if $JBOSS_MAJOR < 7 Line 498: <subsystem xmlns="urn:jboss:domain:jpa:1.0"> Line 499: <jpa default-datasource=""/> Line 533: </login-module> Line 534: #if $JBOSS_MAJOR >= 7 Line 535: <login-module code="RealmDirect" flag="required"> Line 536: <module-option name="password-stacking" value="useFirstPass"/> Line 537: </login-module> why do we need this? Line 538: #end if Line 539: </authentication> Line 540: </security-domain> Line 541: <security-domain name="jboss-web-policy" cache-type="default"> Line 741: port="$getinteger('ENGINE_HTTPS_PORT')" Line 742: #else Line 743: port="0" Line 744: #end if Line 745: /> won't it be easier to have 2 ports and name it: name="redirect-True" name="redirect-False" and refer as: redirect-$getboolean('ENGINE_PROXY_ENABLED') so that config will be simpler? Line 746: #end if Line 747: Line 748: <socket-binding name="txn-recovery-environment" port="8704"/> Line 749: <socket-binding name="txn-status-manager" port="8705"/> -- 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: 5 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Peřina <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Peřina <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: Oved Ourfali <[email protected]> Gerrit-Reviewer: Sandro Bonazzola <[email protected]> Gerrit-Reviewer: Simone Tiraboschi <[email protected]> Gerrit-Reviewer: Yedidyah Bar David <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
