-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Check if required setters are set #2670
Check if required setters are set #2670
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: phanimarupaka 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 |
kyaml/setters2/list.go
Outdated
requiredVal := required.Document().Value | ||
println(requiredVal) | ||
|
||
isSet, err := definitions.Pipe(yaml.Lookup(key, "x-k8s-cli", "setter", "isSet")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe just test err != nil by itself?
then check the requiredVal stuff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the PR with new implementation.
kyaml/setters2/list.go
Outdated
continue | ||
} | ||
requiredVal := required.Document().Value | ||
println(requiredVal) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is println intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seeped in by mistake. Thanks.
As discussed offline, I think we should try to check this using the openapi api, assuming any krm files have already been read and any definitions merged into the schema. |
fa1b8af
to
f591e30
Compare
@mortent Done. |
Looks good to me. I'll give @pwittrock some time to take a look too. |
kyaml/setters2/list.go
Outdated
|
||
// CheckRequiredSettersSet iterates through all the setter definitions in openAPI | ||
// schema and returns error if any of the setter has required field true and isSet false | ||
func CheckRequiredSettersSet() error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Could we put this at least in a separate file? It doesn't seem to really have anything to do with listing setters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mortent Done.
kyaml/setters2/list_test.go
Outdated
dir, err := ioutil.TempDir("", "") | ||
assert.NoError(t, err) | ||
defer os.RemoveAll(dir) | ||
err = ioutil.WriteFile(filepath.Join(dir, "kustomization"), []byte(test.inputOpenAPIfile), 0600) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use Krmfile
instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Looks good from my end. Address the nits. |
f591e30
to
0d90b76
Compare
/lgtm |
@mortent @pwittrock
This PR is to make check for all the setters and return error if a package consumer didn't set the setters which are marked as required by package publisher. This is a follow up PR for the PR in kustomize #2635. Description in the kustomize PR and test cases in current PR clearly describe the expected behavior.
The motivation for this PR is to address the feature request kptdev/kpt#261