Moti Asayag has posted comments on this change. Change subject: core: add keystone url field to external providers ......................................................................
Patch Set 1: (5 comments) http://gerrit.ovirt.org/#/c/34880/1/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/Provider.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/Provider.java: Line 49: private Map<String, String> customProperties; Line 50: Line 51: @Valid Line 52: private P additionalProperties; Line 53: Is there a need for validating Url format ? Can this field be null ? Line 54: private String keystoneURL; Line 55: Line 56: @Override Line 57: public String getName() { Line 50: Line 51: @Valid Line 52: private P additionalProperties; Line 53: Line 54: private String keystoneURL; keystone is a specific identity service. It is relevant only for openstack related services (or any service which integrated with it). However, providers represents a wider range than openstack. Please rename to an abstract name, i.e. authUrl or authenticationUrl and also the relevant getter and setter. Also please maintain camelCase convention in variable/method names. Line 55: Line 56: @Override Line 57: public String getName() { Line 58: return name; http://gerrit.ovirt.org/#/c/34880/1/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/provider/ProviderPopupView.ui.xml File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/provider/ProviderPopupView.ui.xml: Line 70: <ge:EntityModelCheckBoxEditor ui:field="requiresAuthenticationEditor" /> Line 71: <ge:StringEntityModelTextBoxEditor ui:field="usernameEditor" addStyleNames="{style.authField}" /> Line 72: <ge:StringEntityModelPasswordBoxEditor ui:field="passwordEditor" addStyleNames="{style.authField}" /> Line 73: <ge:StringEntityModelTextBoxEditor ui:field="keystoneURLEditor" addStyleNames="{style.authField}" /> Line 74: <ge:StringEntityModelTextBoxEditor ui:field="tenantNameEditor" addStyleNames="{style.authField}" /> IMO it should be either the first or the last entity in that dialog, i.e.: User Name: Password: Tenant Name: KeyStone Url: or KeyStone Url: User Name: Password: Tenant Name: but not in the middle. In addition, we don't use explicit openstack service names in ovirt. It is either "OpenStack Network" or "OpenStack Image Repository". I think we shouldn't break this convention and name the property on UI "Authentication URL" and have a tool tip mentioning "OpenStack Identity Service URL" Line 75: </g:FlowPanel> Line 76: </g:FlowPanel> Line 77: <g:FlowPanel> Line 78: <g:Image ui:field="testResultImage" addStyleNames="{style.testResultImage}" /> http://gerrit.ovirt.org/#/c/34880/1/packaging/dbscripts/providers_sp.sql File packaging/dbscripts/providers_sp.sql: s/keystone/auth/g Line 1: Line 2: Line 3: ---------------------------------------------------------------- Line 4: -- [providers] Table http://gerrit.ovirt.org/#/c/34880/1/packaging/dbscripts/upgrade/03_06_0520_add_keystone_url.sql File packaging/dbscripts/upgrade/03_06_0520_add_keystone_url.sql: Line 1: SELECT fn_db_add_column('providers', 'keystone_url', 'TEXT DEFAULT NULL'); s/keystone/auth/g Line 2: Line 3: UPDATE providers set keystone_url = (select option_value from vdc_options where option_name = 'KeystoneAuthUrl') Line 4: WHERE auth_required; Line 5: -- To view, visit http://gerrit.ovirt.org/34880 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib1aa930cd8dec576a6408402dd883ab5162e1f9d Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@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
