Lior Vernia has posted comments on this change. Change subject: webadmin: integrate CidrValidator to frontend ......................................................................
Patch Set 7: (7 comments) http://gerrit.ovirt.org/#/c/32541/7/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/validation/CidrValidation.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/validation/CidrValidation.java: Line 13: Line 14: @Override Line 15: public ValidationResult validate(Object value) { Line 16: //This validation must be applied to a String Line 17: assert value==null ||value instanceof String; Please add spacing on each side of the "==" operator. Line 18: String cidr = (String) value; Line 19: ValidationResult result = new ValidationResult(); Line 20: result.setSuccess(true); Line 21: if (!CidrValidator.isCidrFormatValid(cidr)) { Line 16: //This validation must be applied to a String Line 17: assert value==null ||value instanceof String; Line 18: String cidr = (String) value; Line 19: ValidationResult result = new ValidationResult(); Line 20: result.setSuccess(true); This is not required, take a look the the no-arg constructor of ValidationResult. Line 21: if (!CidrValidator.isCidrFormatValid(cidr)) { Line 22: failWith(result, getThisFieldMustContainCidrInFormatMsg()); Line 23: } Line 24: http://gerrit.ovirt.org/#/c/32541/7/frontend/webadmin/modules/uicommonweb/src/test/java/org/ovirt/engine/ui/uicommonweb/validation/CidrValidationTest.java File frontend/webadmin/modules/uicommonweb/src/test/java/org/ovirt/engine/ui/uicommonweb/validation/CidrValidationTest.java: Line 22: Line 23: private String cidr; Line 24: private boolean isCidrValid; Line 25: Line 26: public CidrValidationTest(String cidr, boolean boolNotInUse, boolean isCidrValid) { What is boolNotInUse for? Line 27: this.cidr = cidr; Line 28: this.isCidrValid = isCidrValid; Line 29: } Line 30: Line 80: { "255.255.255.255/1", true, false}, //$NON-NLS-1$ Line 81: { "255.255.255.253/2", true, false}, //$NON-NLS-1$ Line 82: { "255.255.255.247/3", true, false}, //$NON-NLS-1$ Line 83: { "255.255.255.16/8", true, false}, //$NON-NLS-1$ Line 84: { "255.255.200.17/14", true, false}, //$NON-NLS-1$ In the above validations, I don't understand the logic of how you change the IP address according to the prefix. They should all be fine even if you leave the address as "255.255.255.255". Line 85: { "255.254.128.0/17", true, false}, //$NON-NLS-1$ Line 86: { "255.1.0.0/24", true, false}, //$NON-NLS-1$ Line 87: { "255.0.0.64/25", true, false}, //$NON-NLS-1$ Line 88: { "253.0.0.0/26", true, false}, //$NON-NLS-1$ Line 82: { "255.255.255.247/3", true, false}, //$NON-NLS-1$ Line 83: { "255.255.255.16/8", true, false}, //$NON-NLS-1$ Line 84: { "255.255.200.17/14", true, false}, //$NON-NLS-1$ Line 85: { "255.254.128.0/17", true, false}, //$NON-NLS-1$ Line 86: { "255.1.0.0/24", true, false}, //$NON-NLS-1$ I think the two previous ones are valid network CIDRs. Line 87: { "255.0.0.64/25", true, false}, //$NON-NLS-1$ Line 88: { "253.0.0.0/26", true, false}, //$NON-NLS-1$ Line 89: { "1.0.0.0/32", true, false}, //$NON-NLS-1$ Line 90: Line 85: { "255.254.128.0/17", true, false}, //$NON-NLS-1$ Line 86: { "255.1.0.0/24", true, false}, //$NON-NLS-1$ Line 87: { "255.0.0.64/25", true, false}, //$NON-NLS-1$ Line 88: { "253.0.0.0/26", true, false}, //$NON-NLS-1$ Line 89: { "1.0.0.0/32", true, false}, //$NON-NLS-1$ As are these two. Line 90: Line 91: // valid CIDR Line 92: { "255.255.255.255/0", true, true}, //$NON-NLS-1$ Line 93: { "255.255.255.254/1", true, true}, //$NON-NLS-1$ Line 103: { "1.0.0.0/21", true, true}, //$NON-NLS-1$ Line 104: { "0.255.255.255/0", true, true}, //$NON-NLS-1$ Line 105: { "0.255.255.254/1", true, true}, //$NON-NLS-1$ Line 106: { "05.255.255.248/3", true, true}, //$NON-NLS-1$ Line 107: { "0.255.0.0/16", true, true} //$NON-NLS-1$ I think all of these are actually invalid network CIDRs (didn't actually look at all of them, but at least plenty of them are). If your tests are passing, then I think your utility isn't working well. Line 108: }); Line 109: } Line 110: -- To view, visit http://gerrit.ovirt.org/32541 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib09d8943b0265960a35f3dd328dae20247b69fd3 Gerrit-PatchSet: 7 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Eliraz Levi <el...@redhat.com> Gerrit-Reviewer: Eliraz Levi <el...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@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
