Eliraz Levi has posted comments on this change. Change subject: webadmin: allowing netmask as mask staticIP conf ......................................................................
Patch Set 2: (7 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 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 asser Done 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 36: return result; Line 37: Line 38: } Line 39: Line 40: // TODO elevi should i have abstract to share this function? > It could be a nice touch but should be in a different patch. Do note that s so for now will keep it this way Line 41: private ValidationResult failWith(ValidationResult result, String errorMessage) { Line 42: result.setSuccess(false); Line 43: result.setReasons(Arrays.asList(errorMessage)); Line 44: return result; 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, Done 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 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? please note the white space "31_" 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. Done 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". Done 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 i think it will get to technical ("in binary rep, string format should be as: 1^n + 0^k , where n+k=32) 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: 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
