Juan Hernandez has posted comments on this change.
Change subject: restapi: add support to regenerate ids when adding vm from
configuration
......................................................................
Patch Set 2:
(4 comments)
I wonder if this is conceptually correct. If the user is POSTing to the
collection of VMs it will expect a new VM to be created, which will require new
ids. Shouldn't the backend by default ignore the id provided by the user and
generate a new one?
If we don't want to do this in the backend, I think that we could reuse the
"force" parameter instead of adding a new one.
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendResource.java
Line 215: }
Line 216:
Line 217: protected boolean isRegenerate() {
Line 218: return getBooleanMatrixConstraint(REGENERATE_CONSTRAINT);
Line 219: }
You are adding the definition and the getter for this parameter in the top root
of the resource hierarchy. I think it it would be better to place it in the
resource that actually needs it (BackendVmsResource) as it isn't generic enough.
In addition you are later with BackendHostResource.REGENERATE_CONSTRAINT, which
is a bit confusing.
Line 220:
Line 221: protected void badRequest(String message) {
Line 222: throw new WebFaultException(null, message,
Response.Status.BAD_REQUEST);
Line 223: }
....................................................
File
backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendVmsResourceTest.java
Line 635: }
Line 636:
Line 637: @Test
Line 638: public void testAddFromConfigurationWithRegenerateTrue() throws
Exception {
Line 639:
setUriInfo(setUpGetMatrixConstraintsExpectations(BackendHostResource.REGENERATE_CONSTRAINT,
Use here the class where the REGENERATE_CONSTRAINT is actually defined, using
BackendHostResource is confusing.
Line 640: true,
Line 641: "true",
Line 642: setUpBasicUriExpectations(),
Line 643: false));
Line 679: verifyModel((VM) response.getEntity(), 2);
Line 680: }
Line 681:
Line 682: @Test
Line 683: public void testAddFromConfiguration() throws Exception {
I think that this test shouldn't be modified. The default of the
ImportAsNewEntity parameter should be such that there is no change in behavior.
If you explicitly add the parameter then we aren't testing that.
You may want to keep this test at is was before and add another one that
explicitly adds the new parameter with the false value.
Line 684:
setUriInfo(setUpGetMatrixConstraintsExpectations(BackendHostResource.REGENERATE_CONSTRAINT,
Line 685: false,
Line 686: "false",
Line 687: setUpBasicUriExpectations(),
Line 704: returnedVM);
Line 705:
setUpCreationExpectations(VdcActionType.ImportVmFromConfiguration,
Line 706: ImportVmParameters.class,
Line 707: new String[] { "Vm", "VdsGroupId",
"ImportAsNewEntity"},
Line 708: new Object[] { returnedVM,
Guid.createGuidFromString(model.getCluster().getId()), false},
This is were I think we shouldn't explicitly pass ImportAsNewEntity and false.
Better create a new test for that.
Line 709: true,
Line 710: true,
Line 711: GUIDS[2],
Line 712: VdcQueryType.GetVmByVmId,
--
To view, visit http://gerrit.ovirt.org/22207
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9ccf0f61b58ad49aa50d1efe3aa0b9a8bd31da70
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Liron Ar <lara...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com>
Gerrit-Reviewer: Juana Garcia <garciajuan...@gmail.com>
Gerrit-Reviewer: Liron Ar <lara...@redhat.com>
Gerrit-Reviewer: Michael Pasternak <mpast...@redhat.com>
Gerrit-Reviewer: Sergey Gotliv <sgot...@redhat.com>
Gerrit-Reviewer: Tal Nisan <tni...@redhat.com>
Gerrit-Reviewer: Vered Volansky <vvola...@redhat.com>
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
