kubernetes-sigs
feat: Detect no matching resources
This PR aims to resolve the rotten issue no. #4379 . As #4715 has rotten as well, this PR is continuing the contributor (@laxmikantbpandhare ) work maintaining the already existing commits.
This PR adds the enforcement for resources selectors not matching any resources at all by throwing an error if no resource has been selected.
A new option has been added, allowNoTargetMatch, allowing to avoid this error, which will cause a warning to be printed and resources to rendered if no further error is encountered.
The Options attribute has been replaced from a map[string]bool to a new type under the api directory named PatchArgs. This will provide a better documentation and will help to avoid typing errors.
NOTE: The changes done in this PR have been requested in the PR mentioned above by @annasong20 .
This PR has multiple commits, and the default merge method is: merge. You can request commits to be squashed using the label: tide/merge-method-squash
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-sigs/prow repository.
The committers listed above are authorized under a signed CLA.
- :white_check_mark: login: adoramshoval / name: Adoram Shoval (f329f91f35b927448f52540712bbc78b3d40298f)
- :white_check_mark: login: laxmikantbpandhare / name: Laxmikant Bhaskar Pandhare (a4d61b8ea7cc2ef41c4568d984813884e430de0c, 2f127dd34e13e9ad6236c079309b1198ccfaae03)
![linux-foundation-easycla[bot] avatar](https://avatars.githubusercontent.com/in/17893?v=4 "Published by a pub.dev verified publisher"){kind=small}
Welcome @adoramshoval!
It looks like this is your first PR to kubernetes-sigs/kustomize 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.
You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.
You can also check if kubernetes-sigs/kustomize has its own contribution guidelines.
You may want to refer to our testing guide if you run into trouble with your tests not passing.
If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!
Thank you, and welcome to Kubernetes. :smiley:
Hi @adoramshoval. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.
Once the patch is verified, the new status will be reflected by the ok-to-test label.
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-sigs/prow repository.
Hi @stormqueen1990 , thanks for your reply.
It's a good idea to refactor the code to use an object type instead of a map for patch options. Could we perhaps have that split out in a separate PR (with its tests), as it would result in a smaller change set to be reviewed for both PRs?
That is correct, thanks, I will work on that.
I also have a question: it looks like the new option
allowNoTargetMatchisfalseby default. That would amount to a breaking change in the current kustomize flow, as current kustomizations with no matching resources do not fail to apply, but would start failing once this is merged. Could you please confirm that would be the case?
Yes, that is the case. Is there a certain work flow I should go through in case of a breaking change?
Hi @stormqueen1990 , I have created another PR (#5930 ) which contains only the addition of the PatchArgs API type as requested. This PR was marked as draft and it is based on the other PR including the same commit. I hope I did everything as you expected, let me know if anything is wrong and I'll fix it.
Yes, that is the case. Is there a certain work flow I should go through in case of a breaking change?
Hi there, @adoramshoval!
In this case, we'll need some input from the current maintainers. @koba1t and @varshaprasad96, could you please confirm whether this change requires extra steps due to being a breaking one?
Hi @stormqueen1990 , I have created another PR (#5930 ) which contains only the addition of the PatchArgs API type as requested. This PR was marked as draft and it is based on the other PR including the same commit. I hope I did everything as you expected, let me know if anything is wrong and I'll fix it.
Thank you very much! I'll take a look at the new PR soon.
/ok-to-test
Hi @koba1t , I have fixed the errors. Is it possible to re-run the tests?
@koba1t , @stormqueen1990 allow me some time while I make some changes based on #5930 .
hi @adoramshoval I merged https://github.com/kubernetes-sigs/kustomize/pull/5930, Could you rebase master?
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: adoramshoval Once this PR has been reviewed and has the lgtm label, please assign koba1t for approval. For more information see the Code Review Process.
The full list of commands accepted by this bot can be found here.
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
hi @adoramshoval I merged #5930, Could you rebase master?
Hi @koba1t , thanks a lot!
I have now finished rebasing master with the changes introduced in the mentioned PR.
I will remove the Draft label from the PR name.
Please let me know if there is anything else I am missing.
@stormqueen1990 , @koba1t Could you go over this again and let me know?
Hi @stormqueen1990 , its been a while, could you give me the clarifications needed in order to continue with this PR?