Tomas Jelinek has posted comments on this change. Change subject: webadmin: add template from snapshot ......................................................................
Patch Set 12: (3 comments) https://gerrit.ovirt.org/#/c/41259/12/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmSnapshotListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmSnapshotListModel.java: Line 672: (HelpTag.new_templat you can not use this one since the definithion contains this: "VMs Tab > Make Template" which is not correct for this. Please add a new entry to HelpTag which will contain the correct path Line 747: new AddVmTemplateFromSnapshotParameters(newVm.getStaticData(), Line 748: model.getName().getEntity(), Line 749: model.getDescription().getEntity(), Line 750: getSelectedItem().getId()); Line 751: parameters.setPublicUse(model.getIsTemplatePublic().getEntity()); I don't particularly like this copy-paste of the parameters creating... I'm pretty sure that when some new device will be added to template it will be forgotten to be added here. Please either extract it to a builder (similar to model building but with parameters) or use some other abstraction. But builder should be good - we could it than reuse also on other places. Line 752: Line 753: parameters.setDiskInfoDestinationMap( Line 754: model.getDisksAllocationModel().getImageToDestinationDomainMap()); Line 755: parameters.setSoundDeviceEnabled(model.getIsSoundcardEnabled().getEntity()); Line 783: VM resultVm = new VM(); Line 784: resultVm.setId(vm.getId()); Line 785: BuilderExecutor.build(model, resultVm.getStaticData(), new CommonUnitToVmBaseBuilder()); Line 786: BuilderExecutor.build(vm.getStaticData(), resultVm.getStaticData(), Line 787: new KernelParamsVmBaseToVmBaseBuilder(), Similar to the thing above - please extract this 4 builders to one composite builder like "VmBaseToVmBaseForTemplateCompositeBaseBuilder". It can be a simple composite builder similar to "CommonUnitToVmBaseBuilder" which will have in the constructor enumerated all this builders and in case something new will be added it will not be forgotten... Line 788: new DedicatedVmForVdsVmBaseToVmBaseBuilder(), Line 789: new MigrationOptionsVmBaseToVmBaseBuilder(), Line 790: new UsbPolicyVmBaseToVmBaseBuilder()); Line 791: return resultVm; -- To view, visit https://gerrit.ovirt.org/41259 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idcf50dce6c749008b03cb94cf666ba295b0b9da6 Gerrit-PatchSet: 12 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Betak <mbe...@redhat.com> Gerrit-Reviewer: Tomas Jelinek <tjeli...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
