Fix name suffix not being applied when "patchesJson6902" is used #4266

Serializator · 2021-11-03T16:20:37Z

k8s-ci-robot · 2021-11-03T16:20:45Z

Welcome @Serializator!

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. 😃

k8s-ci-robot · 2021-11-03T16:20:45Z

Hi @Serializator. 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/test-infra repository.

natasha41575

This PR needs a regression test, I suggest adding it to api/krusty/multipatch_test.go. You can look at the other tests in api/krusty for examples.

First commit should introduce the test with the wrong output demonstrating the failing behavior. Second commit should provide the fix, and correct the output in the test.

natasha41575 · 2021-11-05T21:16:11Z

api/internal/target/kusttarget_configplugin.go

@@ -48,13 +48,13 @@ func (kt *KustTarget) configureBuiltinTransformers(
 	tc *builtinconfig.TransformerConfig) (
 	result []resmap.Transformer, err error) {
 	for _, bpt := range []builtinhelpers.BuiltinPluginType{
+		builtinhelpers.PatchJson6902Transformer,
 		builtinhelpers.PatchStrategicMergeTransformer,
 		builtinhelpers.PatchTransformer,
 		builtinhelpers.NamespaceTransformer,


While I agree that the patchJson6902Transformer should be placed next to the other patch transformers for consistency in behavior, I'm wondering if there might be any other unintended or possibly undesirable consequences for changing the order?

Serializator · 2021-11-05T22:30:40Z

This PR needs a regression test, I suggest adding it to api/krusty/multipatch_test.go. You can look at the other tests in api/krusty for examples.

Excuse me, I completely neglected to run the tests locally, that would've made me aware of the regression before creating a pull request. I will check what and why it broke.

Using bisect I also pinpointed the commit in which it was broken (bd4580d), I will use that as reference point. It obviously isn't the order which broke it as this hasn't really changed since +/- 2019. I was too focused on the code rather than take the history into consideration.

I will add the regression test once I pinpointed the root cause. Thank you for the feedback!

Serializator · 2021-11-06T10:27:34Z

I will communicate further status updates through the original issue (#4111). This way the scope of the conversation in the pull request is kept to the concrete implementation of the final solution rather than findings along the way.

Serializator · 2021-11-07T09:52:37Z

I pushed the proper solution I came up with after finding out this was caused by build annotations not being preserved after a JSON 6902 patch is applied.

I will a add the proper regression tests this evening or tomorrow.

@natasha41575, are files in api/builtins/ generated automatically or should I run a command locally do to that? I'm not at all familiar with Pluginator and couldn't really find a helpful "quick start" documentation for now.

natasha41575 · 2021-11-09T23:18:31Z

are files in api/builtins/ generated automatically or should I run a command locally do to that? I'm not at all familiar with Pluginator and couldn't really find a helpful "quick start" documentation for now.

There is a command you can run, looking at the Makefile it might be make generate-kustomize-builtin-plugins? If that doesn't work I'm certain you can run make prow-presubmit-check and that will generate the code in api/builtins as well as run all the tests locally.

/ok-to-test

…-sigs#4111)

k8s-ci-robot · 2021-11-10T20:38:33Z

@Serializator: 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/test-infra repository.

natasha41575

LGTM apart from this extremely minor formatting nit. Thank you!

natasha41575 · 2021-11-10T20:41:50Z

plugin/builtin/patchjson6902transformer/PatchJson6902Transformer.go

@@ -6,6 +6,7 @@ package main

 import (
 	"fmt"
+	"sigs.k8s.io/kustomize/kyaml/kio/kioutil"


nit: Move this to line 17 with the other sigs.k8s.io imports (and please make sure they are in alphabetical order)

natasha41575 · 2021-11-10T20:42:03Z

plugin/builtin/patchtransformer/PatchTransformer.go

@@ -6,6 +6,7 @@ package main

 import (
 	"fmt"
+	"sigs.k8s.io/kustomize/kyaml/kio/kioutil"


same comment here

natasha41575 · 2021-11-10T20:43:32Z

Also please take a look at the CI errors, it looks like there are some go-gets you have to run.

Serializator · 2021-11-10T20:50:42Z

@natasha41575, running make prow-presubmit-check locally to ensure that it will completely pass on my next commit. Resolved the nitpicks and will commit after all the tests pass! 👍🏼

natasha41575 · 2021-11-10T21:35:04Z

/lgtm
/approve

k8s-ci-robot · 2021-11-10T21:35:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: natasha41575, Serializator

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:

~~OWNERS~~ [natasha41575]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

natasha41575 · 2021-11-10T21:39:43Z

/retest

natasha41575 · 2021-11-10T21:40:25Z

/test all

Serializator · 2021-11-10T21:51:09Z

I don't understand why it is failing out of the sudden... @natasha41575, is it maybe because I didn't commit the api/builtins changes? Should I commit those or is that handled by the "kustomize-presubmit-master" job?

natasha41575 · 2021-11-10T21:57:54Z

I don't understand why it is failing out of the sudden... @natasha41575, is it maybe because I didn't commit the api/builtins changes? Should I commit those or is that handled by the "kustomize-presubmit-master" job?

Yes, please commit all your local changes, including all changes in api/builtins(except for hidden files or go.sum files).

Serializator · 2021-11-10T22:34:33Z

@natasha41575 I made the last changes and should pass all tests now. Sorry for the many back and forwards, still learning the workflow and how everything works / is structured within Kustomize 😄

natasha41575 · 2021-11-10T23:11:53Z

/lgtm

Thanks for working on this!

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 3, 2021

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 3, 2021

k8s-ci-robot requested review from monopole and natasha41575 November 3, 2021 16:20

natasha41575 reviewed Nov 5, 2021

View reviewed changes

Serializator mentioned this pull request Nov 6, 2021

Kustomize doesn't append nameSuffix from overlay when patch is applied in patchesJson6902 section in version 4.2.0 #4111

Closed

Serializator force-pushed the issue-4111-patchJson6902 branch from 5a23361 to bb7f7ec Compare November 7, 2021 09:47

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 7, 2021

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 9, 2021

add test to demonstrate internal annotations getting lost (kubernetes…

b16e4ec

…-sigs#4111)

Serializator force-pushed the issue-4111-patchJson6902 branch from bb7f7ec to d70bc84 Compare November 10, 2021 20:38

natasha41575 reviewed Nov 10, 2021

View reviewed changes

Serializator force-pushed the issue-4111-patchJson6902 branch from d70bc84 to cff4b66 Compare November 10, 2021 21:00

k8s-ci-robot assigned natasha41575 Nov 10, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 10, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 10, 2021

natasha41575 removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 10, 2021

fix build annotations getting lost after applying JSON 6902 patch

b6cb6c8

Serializator force-pushed the issue-4111-patchJson6902 branch from cff4b66 to b6cb6c8 Compare November 10, 2021 22:03

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 10, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 10, 2021

k8s-ci-robot merged commit 0676d0b into kubernetes-sigs:master Nov 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix name suffix not being applied when "patchesJson6902" is used #4266

Fix name suffix not being applied when "patchesJson6902" is used #4266

Serializator commented Nov 3, 2021

k8s-ci-robot commented Nov 3, 2021

k8s-ci-robot commented Nov 3, 2021

natasha41575 left a comment •

edited

natasha41575 Nov 5, 2021

Serializator commented Nov 5, 2021 •

edited

Serializator commented Nov 6, 2021

Serializator commented Nov 7, 2021

natasha41575 commented Nov 9, 2021

k8s-ci-robot commented Nov 10, 2021

natasha41575 left a comment

natasha41575 Nov 10, 2021

natasha41575 Nov 10, 2021

natasha41575 commented Nov 10, 2021

Serializator commented Nov 10, 2021

natasha41575 commented Nov 10, 2021

k8s-ci-robot commented Nov 10, 2021

natasha41575 commented Nov 10, 2021

natasha41575 commented Nov 10, 2021

Serializator commented Nov 10, 2021

natasha41575 commented Nov 10, 2021 •

edited

Serializator commented Nov 10, 2021

natasha41575 commented Nov 10, 2021

Fix name suffix not being applied when "patchesJson6902" is used #4266

Fix name suffix not being applied when "patchesJson6902" is used #4266

Conversation

Serializator commented Nov 3, 2021

k8s-ci-robot commented Nov 3, 2021

k8s-ci-robot commented Nov 3, 2021

natasha41575 left a comment • edited

Choose a reason for hiding this comment

natasha41575 Nov 5, 2021

Choose a reason for hiding this comment

Serializator commented Nov 5, 2021 • edited

Serializator commented Nov 6, 2021

Serializator commented Nov 7, 2021

natasha41575 commented Nov 9, 2021

k8s-ci-robot commented Nov 10, 2021

natasha41575 left a comment

Choose a reason for hiding this comment

natasha41575 Nov 10, 2021

Choose a reason for hiding this comment

natasha41575 Nov 10, 2021

Choose a reason for hiding this comment

natasha41575 commented Nov 10, 2021

Serializator commented Nov 10, 2021

natasha41575 commented Nov 10, 2021

k8s-ci-robot commented Nov 10, 2021

natasha41575 commented Nov 10, 2021

natasha41575 commented Nov 10, 2021

Serializator commented Nov 10, 2021

natasha41575 commented Nov 10, 2021 • edited

Serializator commented Nov 10, 2021

natasha41575 commented Nov 10, 2021

natasha41575 left a comment •

edited

Serializator commented Nov 5, 2021 •

edited

natasha41575 commented Nov 10, 2021 •

edited