Michael Pasternak has posted comments on this change.
Change subject: restapi: copy template permissions REST part
......................................................................
Patch Set 3: I would prefer that you didn't submit this
(10 inline comments)
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd
Line 1938: <xs:element name="origin" type="xs:string" minOccurs="0"/>
Line 1939: <xs:element name="high_availability"
type="HighAvailability" minOccurs="0"/>
Line 1940: <xs:element name="display" type="Display" minOccurs="0"
maxOccurs="1"/>
Line 1941: <xs:element name="stateless" type="xs:boolean"
minOccurs="0"/>
Line 1942: <xs:element name="copy_permissions" type="xs:boolean"
minOccurs="0"/>
we already have similar concept - vm.disks.clone [1] when we want to clone vm
disks,
[1]
<vm>
...
<disks>
<clone>true|false</clone>
</disks>
</vm>
please do the same, but for permissions, i.e
<vm>
...
<permissions>
<clone>true|false</clone>
</permissions>
</vm>
Line 1943: <xs:element name="delete_protected" type="xs:boolean"
minOccurs="0"/>
Line 1944: <xs:element name="timezone" type="xs:string" minOccurs="0"
maxOccurs="1"/>
Line 1945: <xs:element ref="domain" minOccurs="0" maxOccurs="1"/>
Line 1946: <xs:element ref="usb" minOccurs="0" maxOccurs="1"/>
Line 2133: <xs:element name="creation_time" type="xs:dateTime"
minOccurs="0"/>
Line 2134: <xs:element name="origin" type="xs:string" minOccurs="0"/>
Line 2135: <xs:element name="stateless" type="xs:boolean"
minOccurs="0"/>
Line 2136: <xs:element name="delete_protected" type="xs:boolean"
minOccurs="0"/>
Line 2137: <xs:element name="copy_permissions" type="xs:boolean"
minOccurs="0"/>
same
Line 2138: <xs:element name="timezone" type="xs:string" minOccurs="0"
maxOccurs="1"/>
Line 2139: <xs:element ref="domain" minOccurs="0" maxOccurs="1"/>
Line 2140: <xs:element name="custom_properties"
type="CustomProperties" minOccurs="0"/>
Line 2141: <xs:element name="payloads" type="Payloads" minOccurs="0"/>
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/resources/rsdl_metadata.yaml
Line 124: vm.high_availability.enabled: xs:boolean
Line 125: vm.domain.name: xs:string
Line 126: vm.description: xs:string
Line 127: vm.stateless: xs:boolean
Line 128: vm.template.copy_permissions: xs:boolean
please redo after the mentioned schema change
Line 129: vm.delete_protected: xs:boolean
Line 130: vm.cpu.mode: xs:string
Line 131: vm.cpu.topology.sockets: xs:int
Line 132: vm.placement_policy.affinity: xs:string
Line 2442: template.usb.enabled: xs:boolean
Line 2443: template.usb.type: xs:string
Line 2444: template.tunnel_migration: xs:boolean
Line 2445: template.vm.disks.disk--COLLECTION: {disk.id: 'xs:string',
storage_domains.storage_domain--COLLECTION: {storage_domain.id: 'xs:string'}}
Line 2446: template.vm.copy_permissions: xs:boolean
same
Line 2447: template.cpu.cpu_tune.vcpu_pin--COLLECTION:
{vcpu_pin.vcpu: 'xs:int', vcpu_pin.cpu_set: 'xs:string'}
Line 2448: urlparams: {}
Line 2449: headers:
Line 2450: Content-Type: {value: application/xml|json, required: true}
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendTemplatesResource.java
Line 92:
GetVmTemplateParameters.class));
Line 93: }
Line 94:
Line 95: void setupCopyVmPermissions(Template template,
AddVmTemplateParameters params) {
Line 96: if (template.isSetVm() &&
template.getVm().isSetCopyPermissions() &&
template.getVm().isCopyPermissions()) {
why entering in to this flow only when template.getVm().isCopy
Permissions() == true?
i'm not sure this condition is necessary (even if default is false), this will
give you ability to use NULL as a default and to see user 's will, - bottom
line you won't have to touch logic in the client/s when you change it in the
engine.
Line 97:
params.setCopyVmPermissions(template.getVm().isCopyPermissions());
Line 98: }
Line 99: }
Line 100:
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendVmsResource.java
Line 221: return performCreate(VdcActionType.AddVmFromSnapshot,
Line 222: params,
Line 223: new
QueryIdResolver<Guid>(VdcQueryType.GetVmByVmId, IdQueryParameters.class));
Line 224: }
Line 225:
does this relevant for createVmFromSnapshot() as well?
Line 226: private Response cloneVmFromTemplate(VmStatic staticVm, VM vm,
Guid templateId) {
Line 227: AddVmFromTemplateParameters params = new
AddVmFromTemplateParameters(staticVm, getDisksToClone(vm.getDisks(),
templateId), Guid.Empty);
Line 228: params.setVmPayload(getPayload(vm));
Line 229: if (vm.isSetMemoryPolicy() &&
vm.getMemoryPolicy().isSetBallooning()) {
....................................................
File
backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendTemplatesResourceTest.java
Line 50: AddVmTemplateParameters params = new AddVmTemplateParameters();
Line 51: control.replay();
Line 52:
Line 53: collection.setupCopyVmPermissions(getModel(1), params);
Line 54: assertTrue(params.isCopyVmPermissions());
these two tests are more likely to reside in the engine where you test command
parameters classes, while in api tests should test api flows
Line 55: }
Line 56:
Line 57: @Test
Line 58: public void testRemove() throws Exception {
Line 51: control.replay();
Line 52:
Line 53: collection.setupCopyVmPermissions(getModel(1), params);
Line 54: assertTrue(params.isCopyVmPermissions());
Line 55: }
please add testAddWithClonePermissions()
thanks
Line 56:
Line 57: @Test
Line 58: public void testRemove() throws Exception {
Line 59: setUpGetEntityExpectations();
....................................................
File
backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendVmsResourceTest.java
Line 82: control.replay();
Line 83:
collection.setupCopyTemplatePermissions(createVmWithCopyPermissionsSetTo(true),
params);
Line 84: assertTrue(params.isCopyTemplatePermissions());
Line 85: }
Line 86:
these two tests are more likely to reside in the engine where you test command
parameters classes, while in api tests should test api flows
Line 87: private VM createVmWithCopyPermissionsSetTo(boolean
copyPermissions) {
Line 88: VM vm = new VM();
Line 89: vm.setTemplate(new Template());
Line 90: vm.getTemplate().setCopyPermissions(copyPermissions);
Line 88: VM vm = new VM();
Line 89: vm.setTemplate(new Template());
Line 90: vm.getTemplate().setCopyPermissions(copyPermissions);
Line 91: return vm;
Line 92: }
this test does not check api flow but boolean value assignment to the
vm.template.copypermissions,
please see other add() tests for details, note that there are at least two
flows to cover: cloneVmFromTemplate(), addVm()
Line 93:
Line 94: @Test
Line 95: public void testListIncludeStatistics() throws Exception {
Line 96: try {
--
To view, visit http://gerrit.ovirt.org/16199
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I64a0bc3d30cf5ccfb08efe1d02e1c9466d01fe66
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Tomas Jelinek <[email protected]>
Gerrit-Reviewer: Michael Pasternak <[email protected]>
Gerrit-Reviewer: Tomas Jelinek <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
