Replacements should throw errors on invalid targets #4789
Conversation
|
Skipping CI for Draft Pull Request. |
k8s-ci-robot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: KnVerey 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 |
|
/test all |
k8s-ci-robot
added
size/L
KnVerey
force-pushed
the
replacements_regression
branch
from
4bb642d
to
65cd90e
Compare
KnVerey
force-pushed
the
replacements_regression
branch
from
65cd90e
to
1a42e2b
Compare
|
@KnVerey: 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. |
| - select: | ||
| name: deploy2 | ||
| fieldPaths: | ||
| - spec.template.spec.containers |
There was a problem hiding this comment.
This was testing that when given a fieldspec that we cannot satisfy, we do nothing. This PR proposes that we should be throwing an error in this exact case.
There was a problem hiding this comment.
I can't recall if this was intentional or an oversight when this was introduced. But I agree with you that it is odd behavior to silently do nothing, and that it is more intuitive to throw an error instead.
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
/hold For @natasha41575 opinion on the behaviour change, since there was an existing test. (other reviewers feel free to do the review itself though) |
k8s-ci-robot
added
the
do-not-merge/hold
| @@ -546,3 +547,103 @@ metadata: | |||
| name: red-dc6gc5btkc | |||
| `) | |||
| } | |||
|
|
|||
| func TestIssue4761_path_not_in_target_with_create_true(t *testing.T) { | |||
There was a problem hiding this comment.
Could we have these two tests as tests for the replacements filter in api/filters/replacement/replacement_test.go instead of in krusty? The error is thrown by the filter, so I think it makes more sense to have it as a unit test for the filter, keeping the test closer to the logic that it is testing.
The tests here in api/krusty/replacementtransformer_test.go are intended to cover specifically integration test cases that we wouldn't be able to test in the filter unit tests, such as base/overlay structures, name references, interactions with other fields, etc.
There was a problem hiding this comment.
Yep, will move them!
KnVerey
added
the
tide/merge-method-squash
|
Updated with the tests moved. /unhold (since you commented agreeing with throwing an error) |
KnVerey
removed
the
do-not-merge/hold
|
/lgtm |
k8s-ci-robot
added
the
lgtm
…4789) * Replacements should throw errors on invalid targets * Refactor to satisfy complexity linter * Move new tests to filter suite
…4789) * Replacements should throw errors on invalid targets * Refactor to satisfy complexity linter * Move new tests to filter suite
|
Kind of wished there was a deprecation for this behavior change. This wasn't something that was being checked before or made any noise, so "non-matching" replacements were skipped over (which was ok), until now it's broken. The behavior was relied upon when it came to writing a replacement file that was applied to a YAML file with multiple resources of the same kind (or whatever target), but had different fields being replaced. |
|
In my case, this was the desired behavior. I am using kustomize to create ephemeral environments, which reference dozens of application overlays, and they share some common env variables, or settings. So there is a need to modify those env variables or manifests for multiple resources in bulk if they already exist, and if they do not exist, then quietly skip/ignore them. This fix breaks my process completely, so I am not able to use replacements with kustomize v5 anymore. Is there any workaround or plans to make this a togglable feature? |
|
This was also the desired behavior in my case. I'm trying to tackle repetition in our Kustomize codebase, a majority of which comes down to environment-specific GCP project IDs in otherwise identical resources, that need to live in overlays only for that reason. Patches are a solution to that specific problem, but in reality very similar patches would be required in each of our service directories to address Project ID differences, creating another dimension of repetition. To make matters worse, different fields in GCP Config Connector resources require GCP Project IDs in different formats : as a standalone string, as The age-old simplest solution to this extremely common real-world problem is unstructured templating, but I won't be foolish enough to try and raise this topic again here ;) Replacements are hailed as the idiomatic (if limited) Kustomize alternative to the eschewed feature of unstructured edits. So I decided to go all-in with them and set out to create a shared component that can simply be imported in each of our services environment overlays, to perform generic replacements of the GCP Project IDs in our most commonly used Config Connector resource types, which would cover 80% of the repetition in our repo. Because of the variety of formats this string can appear in and the limited power of the That is, until I found out about this PR, which makes writing a single shared component that performs most common replacements nearly impossible, because it would have to limit itself to the lowest common denominator of resources and fields that are present in the exact same format in each and every service that imports the component. I really hope I'm missing something here and you can suggest an alternative I didn't consider. Because to be honest, between this and the "vars loophole" for unstructured edits being plugged, I am getting a bit disheartened at the hoops Kustomize users are made to jump through to address such common real world problems, in the name of software architecture purity (which I acknowledge is an important and difficult thing to consider as a maintainer, but should be balanced with the real world usability of what is ultimately, a tool to get a job done.) On a more constructive note, @KnVerey @natasha41575, can I maybe suggest that a Thank you. |
Closes #4761
by fixing the panic.
However, I concluded that this is not a true regression, in that the original behaviour was also a bug: we were ignoring replacement specifications that were invalid instead of throwing errors. Specifically, if the given path did not exist and options include
create: false, we would silently do nothing.The real fix for the issue author's use case is #4561, which I'm also working on and intend to include in the same release if possible.
Please take a look at the commits individually. The fix itself is in the first commit and is a small change. The second commit is a refactor for clarity, since the linter (rightfully) complained that the complexity of this function had grown too high with this PR.