Martin Peřina has posted comments on this change. Change subject: core: Adds service configuration based on provided JBoss ......................................................................
Patch Set 5: (12 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: Done Line 40: Line 41: def __init__(self): Line 42: super(Daemon, self).__init__() Line 43: self._tempDir = None Line 219: match = self._JBOSS_VERSION_REGEX.match(line) Line 220: if match is not None: Line 221: major = match.group('major') Line 222: minor = match.group('minor') Line 223: revision = match.group('revision') > please cast to int() Done Line 224: break Line 225: Line 226: if major is None: Line 227: raise RuntimeError(_('Cannot detect JBoss version')) 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? Done 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 deb Done Line 253: Line 254: self.logger.debug( Line 255: "Detected JBoss version: %s.%s.%s", Line 256: major, Line 503: pass Line 504: # if self._tempDir: Line 505: # self._tempDir.destroy() Line 506: # if self._jbossRuntime: Line 507: # self._jbossRuntime.destroy() > ? Sorry, forgot to uncomment that back 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 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 We need to define keystore for https inside <security-realm> for undertow and this is not supported with 1.1 version 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 d I will remove ENGINE_HTTPS_ENABLED condition, but we can define keystore only in 2.1 version (see above comment) Line 72: <!-- This is required by the HTTPS listener: --> Line 73: <security-realm name="https"> Line 74: <server-identities> Line 75: <ssl> 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 abov Done 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 Done 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 subsyste The above is the default as defined in standalone.xml of WildFly 8. I will try to minimize it a bit more, but not sure about results 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? It's default for WildFly 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: Sorry, but I don't understand. Could you please elaborate? 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 <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
