Conversation
|
Can one of the admins verify this patch? |
|
ok to test |
| if err := NonEmpty(v); err != nil { | ||
| return err | ||
| } | ||
| if ip := net.ParseIP(v); ip == nil || !strings.Contains(v, ".") { |
There was a problem hiding this comment.
this is to distinguish between ipv4 and ipv6, ya?
installer/pkg/validate/validate.go
Outdated
| return err | ||
| } | ||
|
|
||
| if mask, err := strconv.Atoi(strings.Split(v, "/")[1]); err != nil || mask < 16 || mask > 28 { |
There was a problem hiding this comment.
this depends implicitly on the split slice having at least two elements. we know that this should be the case since we have validated the cidr already.
Instead of splitting and parsing the string, I think it would be better to:
_, c, err := net.ParseCIDR(v)
if err != nil {
return errors.New("not a valid CIDR")
}
if _, mask = c.Mask.Size(); mask < 16 || mask > 28 {
...
}
installer/pkg/validate/validate.go
Outdated
| return nil | ||
| } | ||
|
|
||
| // Host checks if the given string is either a valid v4 IP address or a valid domain name and returns an error if not. |
installer/pkg/validate/validate.go
Outdated
| if err := NonEmpty(v); err != nil { | ||
| return err | ||
| } | ||
| if i, err := strconv.Atoi(v); err != nil || i < 1 || i > 65535 { |
There was a problem hiding this comment.
here you could consider composing with IntRange
| const invalidPortMsg = "invalid port number" | ||
| const noCIDRNetmaskMsg = "must provide a CIDR netmask (eg, /24)" | ||
|
|
||
| type Test struct { |
There was a problem hiding this comment.
should not be an exported type
| t.Errorf("For IntRange(%q, %v, %v), expected %q, got %q", test.in, test.min, test.max, test.expected, err) | ||
| } | ||
| } | ||
|
|
squat
left a comment
squat
left a comment
There was a problem hiding this comment.
Overall looks pretty good! I have a few small nits. Also, can you please add a BUILD.bazel file so this package can be compiled and tested, and then add the test to https://github.com/coreos/tectonic-installer/blob/master/installer/BUILD.bazel#L34
Please let me know if you need help with this
3 participants
Based on the frontend validators from frontend/validate.js and frontend/fields.js.