Eldan Shachar has posted comments on this change. Change subject: userportal\webadmin: cluster parameters override ......................................................................
Patch Set 16: (4 comments) http://gerrit.ovirt.org/#/c/31582/16/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/RunOnceModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/RunOnceModel.java: Line 1007: if (returnValue != null) { Line 1008: Set<String> emulatedSet = new TreeSet<String>((HashSet<String>) returnValue); Line 1009: String oldVal = getEmulatedMachine().getSelectedItem(); Line 1010: getEmulatedMachine().setItems(emulatedSet); Line 1011: getEmulatedMachine().setSelectedItem(oldVal); > no need to check if the oldVal is not empty? if it's empty it means 'use cluster default', the conversion function below translates it into the explicit cluster value(e.g. 'rhel6.5.0'). if the conversion fails and the user submit an empty field then the original vm static value will be used(which is also a valid behavior). Line 1012: } Line 1013: } Line 1014: }), clusterId); Line 1015: Line 1037: // replace 'cluster cpu' with the explicit run-time value Line 1038: if (StringHelper.isNullOrEmpty(oldVal) Line 1039: && !cpuList.isEmpty()) { Line 1040: getCustomCpu().setSelectedItem(cpuList.get( Line 1041: cpuList.size() - 1)); > why the last and not the first? the last is the cluster default (the newest cpu model in the list) Line 1042: } else { Line 1043: getCustomCpu().setSelectedItem(oldVal); Line 1044: } Line 1045: } Line 1039: && !cpuList.isEmpty()) { Line 1040: getCustomCpu().setSelectedItem(cpuList.get( Line 1041: cpuList.size() - 1)); Line 1042: } else { Line 1043: getCustomCpu().setSelectedItem(oldVal); > you want to call this only if the oldVal is not empty, I'd say same as in 'emulated machine' in the comment above. Line 1044: } Line 1045: } Line 1046: } Line 1047: }), cluster.getcpu_name()); Line 1049: Line 1050: // replace 'cluster emulated machine' with the explicit run-time value Line 1051: if (StringHelper.isNullOrEmpty(getEmulatedMachine().getSelectedItem()) Line 1052: && cluster.getEmulatedMachine() != null) { Line 1053: getEmulatedMachine().setSelectedItem(cluster.getEmulatedMachine()); > You have a race here - there is no guarantee that this will be called after Good catch, fixed it like you suggested, thanks. Line 1054: } Line 1055: } Line 1056: } Line 1057: }), clusterId); -- To view, visit http://gerrit.ovirt.org/31582 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia288e46f38c03935ec9b0817833b1921f15ec9f8 Gerrit-PatchSet: 16 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Eldan Shachar <eshac...@redhat.com> Gerrit-Reviewer: Eldan Shachar <eshac...@redhat.com> Gerrit-Reviewer: Tomas Jelinek <tjeli...@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
