Yevgeny Zaspitsky has posted comments on this change. Change subject: webadmin: Add managemenet network field to new/edit cluster dialog ......................................................................
Patch Set 34: (4 comments) http://gerrit.ovirt.org/#/c/37141/34/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/clusters/ClusterListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/clusters/ClusterListModel.java: Line 731: Line 732: final ManagementNetworkOnClusterOperationParameters clusterOperationParameters; Line 733: final VdcActionType actionType; Line 734: final Network managementNetwork = model.getManagementNetwork().getSelectedItem(); Line 735: final Guid managementNetworkId = managementNetwork == null ? null : managementNetwork.getId(); > I'd expect managementNetwork == null to be impossible once you add the vali Done Line 736: if (model.getIsNew()) { Line 737: actionType = VdcActionType.AddVdsGroup; Line 738: clusterOperationParameters = new ManagementNetworkOnClusterOperationParameters(cluster, managementNetworkId); Line 739: } else { Line 740: actionType = VdcActionType.UpdateVdsGroup; Line 741: if (model.isClusterDetached()) { Line 742: clusterOperationParameters = new ManagementNetworkOnClusterOperationParameters(cluster, managementNetworkId); Line 743: } else { Line 744: clusterOperationParameters = new ManagementNetworkOnClusterOperationParameters(cluster); > I don't know how you chose to implement the backend command, but maybe it'd I wouldn't fail the command on wrong management network id if it isn't detached. The current implementation ignores that parameter if the cluster isn't detached. That is in order to be backward compatible otherwise we make that new parameter a mandatory one, whcih breaks the current API. Having two different constructors makes tracing different flows easier and would make it easier separate them in future (if we would want to). Line 745: } Line 746: } Line 747: Frontend.getInstance().runAction( Line 748: actionType, http://gerrit.ovirt.org/#/c/37141/34/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/clusters/ClusterModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/clusters/ClusterModel.java: Line 1874: public boolean validate(boolean validateCpu) Line 1875: { Line 1876: return validate(true, validateCpu, true); Line 1877: } Line 1878: > How about adding a non-empty validation to getManagementNetworks().validate Done Line 1879: public boolean validate(boolean validateStoragePool, boolean validateCpu, boolean validateCustomProperties) Line 1880: { Line 1881: getName().validateEntity(new IValidation[] { Line 1882: new NotEmptyValidation(), Line 1950: && getRngHwrngSourceRequired().getIsValid() Line 1951: && getGlusterHostPassword().getIsValid() Line 1952: && (getIsImportGlusterConfiguration().getEntity() ? (getGlusterHostAddress().getIsValid() Line 1953: && getGlusterHostPassword().getIsValid() Line 1954: && getSerialNumberPolicy().getCustomSerialNumber().getIsValid() > Make sure to also add && getManagementNetwork().getIsValid() somewhere here Done Line 1955: && isFingerprintVerified()) : true); Line 1956: setValidTab(TabName.GENERAL_TAB, generalTabValid); Line 1957: ValidationCompleteEvent.fire(getEventBus(), this); Line 1958: return generalTabValid && getCustomPropertySheet().getIsValid() && getSpiceProxy().getIsValid(); -- To view, visit http://gerrit.ovirt.org/37141 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I055babd6037f127235c349499f1545396e38333f Gerrit-PatchSet: 34 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yevgeny Zaspitsky <yzasp...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Yevgeny Zaspitsky <yzasp...@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
