Yair Zaslavsky has posted comments on this change. Change subject: aaa: Extensions tester tool ......................................................................
Patch Set 6: (13 comments) http://gerrit.ovirt.org/#/c/27814/6/backend/manager/extension-tool/pom.xml File backend/manager/extension-tool/pom.xml: Line 22: <dependency> Line 23: <groupId>${engine.groupId}</groupId> Line 24: <artifactId>common</artifactId> Line 25: <version>${engine.version}</version> Line 26: </dependency> > it should not depend on common Done Line 27: <dependency> Line 28: <groupId>${engine.groupId}</groupId> Line 29: <artifactId>aaa</artifactId> Line 30: <version>${engine.version}</version> Line 32: <dependency> Line 33: <groupId>${engine.groupId}</groupId> Line 34: <artifactId>utils</artifactId> Line 35: <version>${engine.version}</version> Line 36: </dependency> > I sure like to believe that on utils either Done Line 37: <dependency> Line 38: <groupId>commons-lang</groupId> Line 39: <artifactId>commons-lang</artifactId> Line 40: </dependency> http://gerrit.ovirt.org/#/c/27814/6/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/ExtensionsTool.java File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/ExtensionsTool.java: Line 28: this.extensionsManager = extensionsManager; Line 29: try { Line 30: switch (args.getAction()) { Line 31: case ExtensionsToolArguments.ACTION_AUTH_SEQUENCE: Line 32: doAuthSequence(args); > args can be member, and better split into constructor and run() to perform Done Line 33: break; Line 34: Line 35: case ExtensionsToolArguments.ACTION_QUERY_BY_IDS: Line 36: doQueryByIds(args); Line 84: user Line 85: ).mput( Line 86: Authn.InvokeKeys.CREDENTIALS, Line 87: password Line 88: ) > please align... it should be single indent... Done Line 89: ); Line 90: ExtMap authRecord = processAuthnResult(outMap, "AUTHENTICATE_CREDENTIALS"); Line 91: Line 92: if (args.get(ExtensionsToolArguments.ARG_AUTHZ_NAME) != null) { http://gerrit.ovirt.org/#/c/27814/6/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/ExtensionsToolArguments.java File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/ExtensionsToolArguments.java: Line 39: public static final String ARG_PAGE_SIZE = "--page-size"; Line 40: public static final String ARG_IDS = "--ids"; Line 41: public static final String ARG_NAME = "--name"; Line 42: public static final String ARG_RESOLVE_GROUPS_RECURSIVE = "--resolve-groups-recursive"; Line 43: public static final String ARG_MAX_QUERY_RESULTS = "--max-query-results"; > I think better to have: why? in many other tools you have different parameters, what's wrong in that? Line 44: Line 45: // Query entity values Line 46: public static final String QUERY_ENTITY_PRINCIPAL = "principal"; Line 47: public static final String QUERY_ENTITY_GROUP = "group"; Line 238: parser.addArg(new ArgumentBuilder(). Line 239: longName(ARG_RESOLVE_GROUPS_RECURSIVE). Line 240: valueRequired(false). Line 241: build()); Line 242: > I never understood why this is not metadata as resource... but it is the be I am not against discussing future improvements here, but - let's discuss this after we're done with AAA features. Line 243: } Line 244: return parser; Line 245: } Line 246: http://gerrit.ovirt.org/#/c/27814/6/backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/ExtensionsToolExecutor.java File backend/manager/extension-tool/src/main/java/org/ovirt/engine/exttool/ExtensionsToolExecutor.java: Line 18: public static void setupLogging(String log4jConfig, String logFile, String logLevel) { Line 19: URL cfgFileUrl = null; Line 20: try { Line 21: if (log4jConfig == null) { Line 22: cfgFileUrl = ExtensionsToolExecutor.class.getResource("/extensions-tool/log4j.xml"); > qfdn of class please where do u you want me to use ths fqdn of the class, what am I missing? Line 23: } else { Line 24: cfgFileUrl = new File(log4jConfig).toURI().toURL(); Line 25: } Line 26: Log4jUtils.setupLogging(cfgFileUrl); http://gerrit.ovirt.org/#/c/27814/6/backend/manager/extension-tool/src/main/resources/extensions-tool/help.properties File backend/manager/extension-tool/src/main/resources/extensions-tool/help.properties: Line 1: help.usage=usage: extensions-tool <action> [<args>] > I hate we duplicate usage between code and file... remove this until we clo What do you mean? this is exactly how it it is done in manage domains. Line 2: Line 3: help.actions=\ Line 4: Available actions:\ Line 5: \n\tauth-sequence runs an authentication sequence (authenticate, fetch principal, logout)\ http://gerrit.ovirt.org/#/c/27814/6/backend/manager/modules/extensions-manager/src/main/java/org/ovirt/engine/core/extensions/mgr/ExtensionsManager.java File backend/manager/modules/extensions-manager/src/main/java/org/ovirt/engine/core/extensions/mgr/ExtensionsManager.java: Line 296: entry.activated = true; Line 297: dumpConfig(entry.extension); Line 298: setChanged(); Line 299: notifyObservers(); Line 300: return entry.extension; > squash this into[1]? sure, now I Understand what you wanted me to squash in that patch :) Line 301: } Line 302: } Line 303: Line 304: private Extension loadExtension(Properties props) throws Exception { http://gerrit.ovirt.org/#/c/27814/6/backend/manager/pom.xml File backend/manager/pom.xml: Line 15: <module>modules</module> Line 16: <module>tools</module> Line 17: <module>extension-tool</module> Line 18: </modules> Line 19: </project> > squash this into[1]? squash what exactly? the patch you suggested does not include the tool... http://gerrit.ovirt.org/#/c/27814/6/ovirt-engine.spec.in File ovirt-engine.spec.in: Line 556: # Line 557: # /var creation Line 558: # Line 559: install -dm 755 "%{buildroot}/%{engine_state}"/{content,setup/answers} Line 560: install -dm 755 "%{buildroot}/%{engine_log}"/{host-deploy,setup,notifier,engine-manage-domains,dump,extensions-tool} > you should not put logs into /var when running utilities. this is the same Done Line 561: install -dm 755 "%{buildroot}/%{engine_cache}" Line 562: install -dm 755 "%{buildroot}/%{engine_run}/notifier" Line 563: Line 564: # Line 1006: %ghost %config(noreplace) %{engine_etc}/notifier/notifier.conf Line 1007: %{_bindir}/engine-backup Line 1008: %{_bindir}/engine-config Line 1009: %{_bindir}/engine-manage-domains Line 1010: %{_bindir}/extensions-tool > ovirt-engine prefix for new tools please Done Line 1011: %{_mandir}/man8/engine-backup.* Line 1012: %{_mandir}/man8/engine-config.* Line 1013: %{_mandir}/man8/engine-manage-domains.* Line 1014: %{engine_data}/bin/engine-backup.sh Line 1014: %{engine_data}/bin/engine-backup.sh Line 1015: %{engine_data}/bin/engine-config.sh Line 1016: %{engine_data}/bin/engine-manage-domains.sh Line 1017: %{engine_data}/bin/engine-prolog.sh Line 1018: %{engine_data}/bin/extensions-tool.sh > ovirt-engine prefix for new tools please Done Line 1019: %{engine_data}/bin/ovirt-engine-role.sh Line 1020: %{engine_data}/conf/jaas.conf Line 1021: %{engine_data}/services/ovirt-engine-notifier Line 1022: %{engine_etc}/engine-config/engine-config.*properties -- To view, visit http://gerrit.ovirt.org/27814 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7ea2f9c62ced5bdd3801c9f6d8087a35e3c21886 Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <yzasl...@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
