Added extended parameter checks #1715
Conversation
Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
ANeumann82
requested review from
alenkacz,
gerred,
kensipe,
nfnt and
zen-dog
as code owners
| res := verifier.NewResult() | ||
| for _, p := range pf.Params.Parameters { | ||
| if p.DisplayName == "" { | ||
| res.AddParamWarning(p.Name, "has no displayName") |
There was a problem hiding this comment.
Do we have a use for the displayName now? I always thought that it is some poc that went nowhere and that it can be part of hint/description.
There was a problem hiding this comment.
It's part of the UI definition:
- displayName -> Label
- hint -> Shows on-screen format hints, etc. (short one-liner)
- description -> Extended description, normally a (?) sign with mouse-over.
| res := verifier.NewResult() | ||
| for _, p := range pf.Params.Parameters { | ||
| if p.Group == "" { | ||
| res.AddParamWarning(p.Name, "has no group") |
There was a problem hiding this comment.
This would probably be too verbose - top-level parameters without a group are perfectly fine, aren't' they?
There was a problem hiding this comment.
Well... Maybe? :) Generally I agree, that's why this is a separate check, but in this case I wanted to know which parameters don't have a group assigned yet, thats why this check is here...
| } | ||
| } | ||
| if v.VerifyParamDisplayName && g.DisplayName != "" { | ||
| if strings.HasSuffix(g.DisplayName, ":") { |
There was a problem hiding this comment.
That's oddly specific - maybe just make sure that it ends with a character?
There was a problem hiding this comment.
Yeah - it's specific because I ran into it :D I started to add the ':' to some displayNames before I noticed that it should be consistent.
I'm not sure if "ends with a character" would maybe be too limiting though. I'm up for more checks though.
|
|
||
| func (v ExtendedParametersVerifier) verifyParamGroup(pf *packages.Files) verifier.Result { | ||
| res := verifier.NewResult() | ||
| for _, p := range pf.Params.Parameters { |
There was a problem hiding this comment.
Shouldn't we make sure that the corresponding p.Group actually exists in pf.Params.Groups?
There was a problem hiding this comment.
Yeah, but that's actually checked in the mandatory checks, as it is an error. I've added a comment here to make this clear.
|
|
||
| func (v ExtendedParametersVerifier) verifyGroups(pf *packages.Files) verifier.Result { | ||
| res := verifier.NewResult() | ||
| for _, g := range pf.Params.Groups { |
There was a problem hiding this comment.
This is almost the same as the check in L94 above. Why having two verifier methods at all? Why not have one verifyGroups method that checks:
- valid group decoration
- parameter belongs to a valid and existing group
This verifier could call sub-methods but both things should be checked in conjunction, don't you think?
There was a problem hiding this comment.
I don't think they're really related. I can see a case where one would want to verify that
a) All defined groups have displayName and description
b) Not require a group for all parameters.
Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
# Conflicts: # pkg/kudoctl/cmd/package_verify.go
What this PR does / why we need it:
When working on the Cassandra I really wanted to have something to check the additional attributes for parameters, but we don't want to make this mandatory.
I've implemented a couple of checks that can now be enabled with:
There are a couple more checks for params, listed in the help