baremetal: platform host validations #3232

andfasano · 2020-03-04T14:30:48Z

This change proposes the same host validations already described in another PR (#3208 thanks to @stbenjam) but using the go-playground/validator module to make the validation process easier, reusable and more scalable, and helping in reducing the amount of code requested.
This change only affects the host validations by reusing an existing validation rule and adding a custom one for detecting duplicated values, but it could also be easily adopted for other kind of validations.

stbenjam · 2020-03-04T14:44:05Z

/label platform/baremetal

stbenjam · 2020-03-04T14:53:20Z

On 45da73f, the commit message could have a little more detail. It's not just duplicates, and should mention that it's for baremetal.

@abhinavdahiya PTAL - alternative approach to #3208

stbenjam · 2020-03-04T14:53:37Z

/assign @abhinavdahiya

metal3ci · 2020-03-04T15:07:13Z

Build FAILURE, see build http://10.8.144.11:8080/job/dev-tools/1563/

abhinavdahiya · 2020-03-04T15:30:07Z

Personally I don't see how this helps, it's more magic with fields parent tags etc, uses reflect for later reporting.

The only benefit was required was reduced, and that kind of already easy to DRY out.. and in cases where required needs more special handling like required when this other thing is set, we will end up creating these new tags or write that logic anyway.

So I like the previous method as it's more easy to ready, extend.

andfasano · 2020-03-04T15:59:24Z

Personally I don't see how this helps, it's more magic with fields parent tags etc, uses reflect for later reporting.

The only benefit was required was reduced, and that kind of already easy to DRY out.. and in cases where required needs more special handling like required when this other thing is set, we will end up creating these new tags or write that logic anyway.

So I like the previous method as it's more easy to ready, extend.

A validation rule is identified by a tag. So it's just a matter to annotate which fields of which struct are required to be validate against which rules. And the framework already offers many pre-baked rules, and by default allows to apply the validation to nested struct (see for example the validate:"required" tag on baremetal.BMC and baremetal.Host structs).
So, no additional code has been added for that and the readability has been improved as the reader can immediately understand which rules will be applied by simply looking at the struct definition.
The uniqueField custom rule is instead an example of how a custom rule could be injected in the underlying framework to address those validations that are not supported by default, and the framework makes it easy to apply to any field that has been properly tagged of any nested struct, and offers all the necessary parameters to perform the validation.
And there's no need to explicitly reference any specific field, thus allowing for a more generic implementation (and less amount of code).

pkg/types/baremetal/validation/platform.go

stbenjam · 2020-03-04T16:22:13Z

I like the annotation based approach for validations, rather than having the same checks over and over again in each platform's validations. The general case is more common than a special case ("if this field, then that one" or other constraints) and would probably simplify most platforms.

abhinavdahiya · 2020-03-04T17:55:31Z

more than happy to approve the vendoring after this PR is LGTM from baremetal-approvers. If the baremetal validations are more successful with this approach we can look at expanding them.

pkg/types/baremetal/validation/platform.go

abhinavdahiya · 2020-03-04T17:59:12Z

pkg/types/baremetal/validation/platform.go

+				fieldValues[fieldValue] = struct{}{}
+			}
+		} else {
+			panic(fmt.Sprintf("Cannot apply validation rule 'uniqueField' on field %s", fl.FieldName()))


panics are less desirable in the code for installer

Wasn't sure about the standard adopted, I noticed that it was used somewhere (https://github.com/openshift/installer/search?q=panic+-path%3A%2Fvendor&unscoped_q=panic+-path%3A%2Fvendor) and thought it could have been useful in case of configuration mistake. But if it's not I'll remove it.

A panic seems fair here, this would only happen if a dev adds the annotation that a field is uniqueValue and it's not Comparable.

metal3ci · 2020-03-06T14:50:09Z

Build FAILURE, see build http://10.8.144.11:8080/job/dev-tools/1581/

metal3ci · 2020-03-06T19:26:27Z

Build FAILURE, see build http://10.8.144.11:8080/job/dev-tools/1584/

andfasano · 2020-03-09T08:16:33Z

/test pj-rehearse

stbenjam · 2020-03-12T19:33:47Z

/test golint

stbenjam · 2020-03-12T19:38:56Z

I'm happy with this version. @andfasano Could you clean up the commit history? Should be 1 vendor commit, 1 code commit

andfasano · 2020-03-18T16:14:34Z

Cleanup done, please let me know if it's ok

stbenjam · 2020-03-18T16:16:49Z

/lgtm

@abhinavdahiya PTAL, thanks!

abhinavdahiya · 2020-03-18T17:02:39Z

b47a051 LGTM

/approve

stbenjam · 2020-03-19T16:27:45Z

vendor/modules.txt

@@ -919,10 +926,15 @@ github.com/keybase/go-crypto/rsa
 github.com/konsorten/go-windows-terminal-sequences
 # github.com/kr/fs v0.1.0
 github.com/kr/fs
+<<<<<<< HEAD


Looks like there's still an unresolved conflict

just run go mod tidy && go mod vendor instead of trying to solve the merge conflict.. maybe?

Sorry my fault I'll do it asap

This module simplify and make more scalable the validation steps, It's annotations based, offers a good set of pre-existing validation rules and allows to add easily new custom validation rules

baremetal.Host is validated using go-playground/validator module and a custom defined rule for duplicates

metal3ci · 2020-03-20T04:49:03Z

Build SUCCESS, see build http://10.8.144.11:8080/job/dev-tools/1658/

andfasano · 2020-03-20T08:56:42Z

/hold cancel

andfasano · 2020-03-20T10:22:10Z

/retest

stbenjam · 2020-03-20T14:11:54Z

/lgtm

openshift-bot · 2020-03-20T14:20:02Z

/retest