Arik Hadas has posted comments on this change. Change subject: core: fix support for pattern-based name for pool ......................................................................
Patch Set 3: (1 comment) http://gerrit.ovirt.org/#/c/26356/3/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVmPoolWithVmsCommandTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddVmPoolWithVmsCommandTest.java: Line 58: String patternBaseName = "aa-??bb"; Line 59: command.getParameters().getVmStaticData().setName(patternBaseName); Line 60: command.getParameters().getVmPool().setName(patternBaseName); Line 61: assertTrue(command.validateInputs()); Line 62: } > +1 now we need a negative test for adding a VM with that exact pattern right, that's a gap we have since the validation framework was introduced in ovirt - I see that their tests are missing almost everywhere. how about keeping this patch focused on the regression it is supposed to solve (the added test is required since we introduce something new so it should come with a test) and fix other issues in separate patch? I prefer not to backport general unrelated changes to 3.4, I'm sure that while trying to test the validations for add-vm we'll detect things in our tests that should be changed to make the validations work, as I found in this case (like the number of monitors that should be changed). -- To view, visit http://gerrit.ovirt.org/26356 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5bfa9fb37d464dd0e77b0bc206c1804713ed9575 Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Roy Golan <rgo...@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
