tests: record fields that are never set #3435
Conversation
| } | ||
|
|
||
| // Any XYZRef field was already handled and handling the children will just double count | ||
| if strings.Contains(fieldPath, "Ref") { |
There was a problem hiding this comment.
What's an example of this? Because I think you've already checked strings.HasSuffix(fieldPath, "Ref") { above...
There was a problem hiding this comment.
Yeah for references in particular I decided to call it set if the field ending in Ref specifies either external or name.
For instance, when we visit projectRef we run the special handling to determine if either is set.
but then in our depth first traversal we also visit the fields children again and can end up in a situation where we call out that external is not set when name is:
+ [missing_field] crd=alloydbbackups.alloydb.cnrm.cloud.google.com version=v1beta1: field ".spec.clusterNameRef.external" is not set in unstructured objects
There was a problem hiding this comment.
Ah gotcha!
| if strings.Contains(fieldPath, "Ref") { | |
| // Any fooRef field was already handled and handling the children will just double count | |
| // when we visit fooRef.namespace, fooRef.name and fooRef.external etc | |
| if strings.Contains(fieldPath, "Ref.") { |
acpana
force-pushed
the
acpana/new-fieldtool
branch
from
5671196 to
d69a244
Compare
| // Check if field exists in any unstructured object | ||
| missing := true | ||
| for _, obj := range unstructs { | ||
| if hasField(obj.Object, fieldPath) { |
There was a problem hiding this comment.
Oooh, can we have another linter that checks for fields we hallucinated :-) (I.e. checks the other way round) There might be a refactor where we introduce something like CollectFields(obj *unstructured.Unstructured) sets[string]
There was a problem hiding this comment.
Oh interesting so the check is there to make sure that the fields in the list are defined in the CRD, right ?
There was a problem hiding this comment.
Right. I think we've got most of them, but there really shouldn't be an exception list here. Though if you implement this, I think it's reasonable to first creating an exception list and then we can farm them out by type!
|
Looks good, a few nits but then I think we should merge |
acpana
force-pushed
the
acpana/new-fieldtool
branch
from
d69a244 to
69f7bf0
Compare
Signed-off-by: Alex Pana <[email protected]>
acpana
force-pushed
the
acpana/new-fieldtool
branch
from
69f7bf0 to
efac31e
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justinsb The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
google-oss-prow
bot
merged commit 687c2c0
into
GoogleCloudPlatform:master
We should check that all fields for a CRD are set in the fixtures at some point (either in create.yaml or update.yaml. Further, doing that we automate a fair amount of PR reviewing away from checking the CRD definition and how it's tested in the resourcefixtures
( This is actually a better incarnation of #2326 )