Lior Vernia has posted comments on this change. Change subject: webadmin: Add managemenet network field to new/edit cluster dialog ......................................................................
Patch Set 9: (14 comments) Adding Oved to comment on the location of the new UI field in the dialog. http://gerrit.ovirt.org/#/c/37141/9/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/Linq.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/Linq.java: Line 1340: return lexoNumeric.compare(cluster1.getName(), cluster2.getName()); Line 1341: } Line 1342: } Line 1343: Line 1344: public final static class NetworkComparator implements Comparator<Network>, Serializable { In a previous patch I suggested to modify this to only be used in a specific cluster context - I still stand behind that suggestion, and in this case you'll be displaying all the networks in the DC so this comparator shouldn't be used (and therefore shouldn't be modified in this patch at all). In this case you only need to sort by name, i.e. use NameableComparator - note that this is only relevant in the new cluster case, so I don't think you'll have any information on whether the network functions as management in any other cluster (and even if you did, I don't think it'd be right to factor that in when sorting). Line 1345: Line 1346: private static final long serialVersionUID = 990203400356561587L; Line 1347: private final LexoNumericComparator lexoNumeric = new LexoNumericComparator(); Line 1348: http://gerrit.ovirt.org/#/c/37141/9/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/dataprovider/AsyncDataProvider.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/dataprovider/AsyncDataProvider.java: Line 656 Line 657 Line 658 Line 659 Line 660 Why did you add this method in a previous patch only to remove it? It can still be used by both isClusterEmpty() and isManagementNetwork()... Line 649: aQuery); Line 650: } Line 651: Line 652: public void getDefaultManagementNetwork(AsyncQuery aQuery, Guid dataCenterId) { Line 653: aQuery.converterCallback = new AsIsAsyncConverter(); Why not convert to Network here? Line 654: Frontend.getInstance().runQuery( Line 655: VdcQueryType.GetDefaultManagementNetwork, Line 656: new IdQueryParameters(dataCenterId), Line 657: aQuery); Line 657: aQuery); Line 658: } Line 659: Line 660: public void getManagementNetwork(AsyncQuery aQuery, Guid clusterId) { Line 661: aQuery.converterCallback = new AsIsAsyncConverter(); And here? Line 662: Frontend.getInstance().runQuery( Line 663: VdcQueryType.GetManagementNetwork, Line 664: new IdQueryParameters(clusterId), Line 665: aQuery); http://gerrit.ovirt.org/#/c/37141/9/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 762: final Network managementNetwork = model.getManagementNetwork().getSelectedItem(); Line 763: final Guid managementNetworkId = managementNetwork == null ? null : managementNetwork.getId(); Line 764: clusterOperationParameters = new AddClusterOperationParameters(cluster, managementNetworkId); Line 765: } else { Line 766: actionType = VdcActionType.UpdateVdsGroup; I'm not sure in this case you don't have to choose a management network - if the cluster was detached and is re-attached to a DC, a choice of management network should be made. So you likely need to modify the update command as well. Line 767: clusterOperationParameters = new VdsGroupOperationParameters(cluster); Line 768: } Line 769: Frontend.getInstance().runAction( Line 770: actionType, http://gerrit.ovirt.org/#/c/37141/9/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 1320 Line 1321 Line 1322 Line 1323 Line 1324 If you want to allow as much flexibility as possible, why not cast to Iterable<StoragePool>? You don't use any added functionality from Collection or List. Line 1362: clusterModel.setMigrateOnErrorOption(clusterModel.getEntity().getMigrateOnError()); Line 1363: Line 1364: if (getIsEdit()) { Line 1365: if (dataCenterId == null) { Line 1366: setEmptyNetworkList(clusterModel); In this case you shouldn't have to do anything - it doesn't hurt to have the items as null. Line 1367: } else { Line 1368: loadCurrentClusteManagementNetwork(); Line 1369: } Line 1370: } Line 1377: final AsyncQuery getManagementNetworkQuery = new AsyncQuery(this, new INewAsyncCallback() { Line 1378: @Override Line 1379: public void onSuccess(Object model, Object returnValue) { Line 1380: final ClusterModel clusterModel = (ClusterModel) model; Line 1381: final Network managementNetwork = (Network) returnValue; All you have to do here is invoke clusterModel.setSelectedItem(managementNetwork) - doesn't matter if it's null or not. And since the ListModel should be disabled, the list of items doesn't really matter. So the code below isn't needed. Line 1382: if (managementNetwork == null) { Line 1383: setEmptyNetworkList(clusterModel); Line 1384: } else { Line 1385: clusterModel.getManagementNetwork().setItems(Collections.singletonList(managementNetwork)); Line 1389: }); Line 1390: AsyncDataProvider.getInstance().getManagementNetwork(getManagementNetworkQuery, getEntity().getId()); Line 1391: } Line 1392: Line 1393: private void setEmptyNetworkList(ClusterModel clusterModel) { I don't think there's any case where invoking this method is actually necessary, see other comments, so can be removed. Line 1394: clusterModel.getManagementNetwork().setItems(Collections.<Network> emptyList()); Line 1395: } Line 1396: Line 1397: private void loadDcNetworks(final Guid dataCenterId) { Line 1395: } Line 1396: Line 1397: private void loadDcNetworks(final Guid dataCenterId) { Line 1398: if (dataCenterId == null) { Line 1399: setEmptyNetworkList(this); I think returning here is enough. Line 1400: return; Line 1401: } Line 1402: final AsyncQuery getAllDataCenterNetworksQuery = new AsyncQuery(this, new INewAsyncCallback() { Line 1403: @Override Line 1421: } Line 1422: } Line 1423: } Line 1424: }); Line 1425: AsyncDataProvider.getInstance() How about cache the default management network per cluster in a Map<Guid, Network> member? And if the network had already been fetched for a specific cluster, use the cached reference instead of firing another query? Remember the user can go back and forth between choices in the list box. Line 1426: .getDefaultManagementNetwork(getDefaultManagementNetworkQuery, dataCenterId); Line 1427: } Line 1428: }); Line 1429: AsyncDataProvider.getInstance().getAllDataCenterNetworks(getAllDataCenterNetworksQuery, dataCenterId); Line 1836: } Line 1837: }; Line 1838: AsyncDataProvider.getInstance().getDataCenterVersions(_asyncQuery, selectedDataCenter.getId()); Line 1839: Line 1840: if (!getIsEdit()) { This doesn't need to depend on !getIsEdit() - the DC can only be changed if it's a new cluster or if it's detached and re-attached to a new DC. If it's re-attached - you need to choose a management network... Line 1841: loadDcNetworks(selectedDataCenter.getId()); Line 1842: } Line 1843: } Line 1844: http://gerrit.ovirt.org/#/c/37141/9/frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/UIConstants.java File frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/UIConstants.java: Line 2507: Line 2508: @DefaultStringValue("This field is not a valid Guid (use 0-9,A-F format: 00000000-0000-0000-0000-000000000000)") Line 2509: String invalidGuidMsg(); Line 2510: Line 2511: @DefaultStringValue("Changing management network is allowed through manage networks screen") "Changing management network is only permitted via the 'Manage Cluster Networks' dialog." Line 2512: String prohibitManagementNetworkChangeInEditClusterInfoMessage(); http://gerrit.ovirt.org/#/c/37141/9/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/cluster/ClusterPopupView.ui.xml File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/cluster/ClusterPopupView.ui.xml: Line 170: </g:FlowPanel> Line 171: <ge:StringEntityModelTextBoxEditor ui:field="nameEditor" /> Line 172: <ge:StringEntityModelTextBoxEditor ui:field="descriptionEditor" /> Line 173: <ge:StringEntityModelTextBoxEditor ui:field="commentEditor" /> Line 174: <e:ListModelListBoxEditor ui:field="managementNetworkEditor" /> Please consult with Oved whether this is where this field should be (maybe they'd prefer to have it below the architecture, etc.). Line 175: <e:ListModelListBoxEditor ui:field="architectureEditor" /> Line 176: <e:ListModelListBoxEditor ui:field="cpuEditor" /> Line 177: <e:ListModelListBoxEditor ui:field="versionEditor" /> Line 178: <g:VerticalPanel ui:field="servicesCheckboxPanel"> -- 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: 9 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
