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
Run kustomize build with kustomize localize and add a no-verify flag. #5544
base: master
Are you sure you want to change the base?
Run kustomize build with kustomize localize and add a no-verify flag. #5544
Conversation
This PR has multiple commits, and the default merge method is: merge. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Welcome @sanaasy! |
Hi @sanaasy. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
I'm still working on tests for this issue so keeping this in draft! Edit: this is done! |
b32e146
to
a58784b
Compare
a58784b
to
874ec03
Compare
874ec03
to
c576b85
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: christianh814, sanaasy The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
Hello there, @sanaasy! 👋
Thank you very much for your contribution.
With regards to using exec.Command
, I am not sure if I understand the reasoning for choosing it over invoking the build
command function directly, since localize
and build
are relatively co-located in the codebase. Would you mind elaborating on that decision?
Sure! When I first wrote the code, I didn't see that the build command |
Hi there, @sanaasy! Sorry for the delay in responding! I have some concerns with the The scenario I have in mind is something like this:
Please let me know if my understanding is incorrect, or if this scenario doesn't make sense! |
cf48606
to
cc2aff7
Compare
…e localize command is run.
That's a fair callout! I have updated the PR to use the local build commands instead and have verified that things work as expected. If you could take another look please! |
cc2aff7
to
facafce
Compare
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.
Hi there, @sanaasy! 👋🏻
Apologies for my delayed response! I took a look at the KEP for the localize command and found a few points that need changes.
Let me know if these make sense!
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.
Hi there, @sanaasy! 👋🏻
Thank you for your contribution! 🙏🏻
This PR is well on its way, I just have a couple of suggestions in terms of test coverage.
- name: helmTest | ||
repo: testRepo |
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.
If you'd like to have something that pulls real content from somewhere, we have quite a few usages of
- name: external-dns
repo: oci://registry-1.docker.io/bitnamicharts/
in tests.
@@ -66,6 +82,58 @@ func TestScopeFlag(t *testing.T) { | |||
loctest.CheckFs(t, testDir.String(), expected, actual) | |||
} | |||
|
|||
func TestNoVerifyFlag(t *testing.T) { | |||
kustomization := map[string]string{ | |||
"kustomization.yaml": `namePrefix: test- |
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.
I think it would be interesting to have an extra test case here with some localizable content. Perhaps a resource such as https://raw.githubusercontent.com/kubernetes-sigs/kustomize/v1.0.6/examples/helloWorld/deployment.yaml could be a good candidate (since it is part of this repo)?
/test all |
/label tide/merge-method-squash |
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.
Found a few more bits!
} | ||
|
||
if localizedBuild == originalBuild { | ||
log.Printf("VERIFICATION SUCCESS: `kustomize build` for %s and %s are the same after localization.", args.target, dst) |
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.
minor: I think this log could benefit of having a line break at the end, since Printf()
won't add it by default.
log.Printf("VERIFICATION SUCCESS: `kustomize build` for %s and %s are the same after localization.", args.target, dst) | ||
} else { | ||
log.Println(copyutil.PrettyFileDiff(originalBuild, localizedBuild)) | ||
log.Fatalf("VERFICATION FAILED: `kustomize build` for %s and %s are different after localization.", args.target, dst) |
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.
Would be nice to have a line break at the end of this message as well.
@@ -62,6 +68,26 @@ kustomize localize https://github.com/kubernetes-sigs/kustomize//api/krusty/test | |||
if err != nil { | |||
return errors.Wrap(err) | |||
} | |||
|
|||
if !f.noVerify && f.scope == "" { |
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.
localize
with verification should still work even if --scope
is specified.
Context
Closes issue: #5276
This PR adds functionality to auto-run
kustomize build
when thekustomize localize
command is run. . This will run when--no-verify
is not specified as false by the user or not specified at all when running the localize command. When the--no-verify
flag is added, this check will be skipped.🎩 Tophatted by doing:
make kustomize
~/go/bin/kustomize localize . localized-results
~/go/bin/kustomize localize . localized-results --no-verify
~/go/bin/kustomize localize . localized-results --scope "/Desktop"