Lior Vernia has posted comments on this change. Change subject: webadmin: allowing netmask as mask staticIP conf ......................................................................
Patch Set 2: (9 comments) Please disregard my previous comment, I was at error! See new comments. http://gerrit.ovirt.org/#/c/35145/2/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/validation/SubnetMaskValidation.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/validation/SubnetMaskValidation.java: Line 47 Line 48 Line 49 Line 50 Line 51 > I think your validation is different than the original validation - it seem Sorry, my bad! Looks fine. Line 70 Line 71 Line 72 Line 73 Line 74 Do note that here "0.0.0.0" is considered an invalid subnet and in your implementation I think it's considered valid - find out which of them is more correct (and why) and potentially fix your implementation. Line 22: } Line 23: Line 24: @Override Line 25: public ValidationResult validate(Object value) { Line 26: // This validation must be applied to a String Instead of writing this in a comment, you could add it as part of the assertion using the ':' operator - this will be nicer since a developer working in debug mode will be able to see the problem directly in the debug log, instead of having to find the reason in the code documentation. Line 27: assert value == null || value instanceof String; Line 28: String mask = (String) value; Line 29: ValidationResult result = new ValidationResult(); Line 30: if (!MaskValidator.getInstance().isMaskFormatValid(mask)) { Line 37: Line 38: } Line 39: Line 40: // TODO elevi should i have abstract to share this function? Line 41: private ValidationResult failWith(ValidationResult result, String errorMessage) { Seems like there's no added value in passing ValidationResult as argument, and that it can be created as part of this method (and make the method simpler to use). Line 42: result.setSuccess(false); Line 43: result.setReasons(Arrays.asList(errorMessage)); Line 44: return result; Line 45: } http://gerrit.ovirt.org/#/c/35145/2/frontend/webadmin/modules/uicommonweb/src/test/java/org/ovirt/engine/ui/uicommonweb/validation/SubnetMaskValidationTest.java File frontend/webadmin/modules/uicommonweb/src/test/java/org/ovirt/engine/ui/uicommonweb/validation/SubnetMaskValidationTest.java: Line 49: { "255.255.0", false }, //$NON-NLS-1$ Line 50: { "255.255.0.0.0", false }, //$NON-NLS-1$ Line 51: { "255.255.0.0.", false }, //$NON-NLS-1$ Line 52: Line 53: // Prefix not valid Please add "0.0.0.0" here if it isn't valid, or down below if it is valid... Line 54: { "255.255.0.1", false }, //$NON-NLS-1$ Line 55: { "255.0.255.0", false }, //$NON-NLS-1$ Line 56: { "255.0.0.255", false }, //$NON-NLS-1$ Line 57: { "224.0.255.0", false }, //$NON-NLS-1$ Line 60: { "255.255.0.0", true }, //$NON-NLS-1$ Line 61: { "255.255.255.255", true }, //$NON-NLS-1$ Line 62: Line 63: // 0-32 bad format format Line 64: { "31 ", false }, //$NON-NLS-1$ How can this be both false and true? Line 65: { "33", false }, //$NON-NLS-1$ Line 66: { "31/", false }, //$NON-NLS-1$ Line 67: { "31*", false }, //$NON-NLS-1$ Line 68: { "//31 ", false }, //$NON-NLS-1$ Line 67: { "31*", false }, //$NON-NLS-1$ Line 68: { "//31 ", false }, //$NON-NLS-1$ Line 69: Line 70: // 0-32 valid Line 71: { "31", true }, //$NON-NLS-1$ How about "/31"? Should be valid, the common utility has to support it. Line 72: Line 73: }); Line 74: } Line 75: http://gerrit.ovirt.org/#/c/35145/2/frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/UIConstants.java File frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/UIConstants.java: Line 896: Line 897: @DefaultStringValue("This field can be empty or contain an IP address in format xxx.xxx.xxx.xxx") Line 898: String emptyOrValidIPaddressInFormatMsg(); Line 899: Line 900: @DefaultStringValue("This field must contain a subnet in the following format:\n\txxx.xxx.xxx.xxx\n\txx where xx between 0-32") Firstly, should be "in either of the following formats". Secondly, since you're already improving the error message, maybe add a 0-255 to the first format? Similar to thisFieldMustContainCidrInFormatMsg(). Thirdly, you're missing "is" in "where xx is between 0-32". Line 901: String thisFieldMustContainSubnetFormatAsNetmaskOrPrefixMsg(); Line 902: Line 903: @DefaultStringValue("Invalid mask value") Line 904: String inValidNetmask(); Line 899: Line 900: @DefaultStringValue("This field must contain a subnet in the following format:\n\txxx.xxx.xxx.xxx\n\txx where xx between 0-32") Line 901: String thisFieldMustContainSubnetFormatAsNetmaskOrPrefixMsg(); Line 902: Line 903: @DefaultStringValue("Invalid mask value") This isn't a very informational message - should be more along the lines of cidrNotNetworkAddress(). Line 904: String inValidNetmask(); Line 905: Line 906: @DefaultStringValue("This field must contain a CIDR in format xxx.xxx.xxx.xxx/yy, where xxx is between 0 and 255 and yy is between 0 and 32.") Line 907: String thisFieldMustContainCidrInFormatMsg(); -- To view, visit http://gerrit.ovirt.org/35145 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If909eda53a61bda5030c342156a01e53576e77cb Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: 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
