[Address #3937] Add labels to template/metadata if includeTemplates is true #4209

aibarbetta · 2021-09-27T19:08:47Z

k8s-ci-robot · 2021-09-27T19:08:55Z

Hi @aibarbetta. 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.

aibarbetta · 2021-09-27T19:11:29Z

/retitle [Fix #3937] Add labels to template/metadata if includeSelectors is false

k8s-ci-robot · 2021-09-27T19:11:36Z

@aibarbetta: Re-titling can only be requested by trusted users, like repository collaborators.

In response to this:

/retitle [Fix #3937] Add labels to template/metadata if includeSelectors is false

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 · 2021-11-01T22:24:39Z

/ok-to-test
/lgtm

natasha41575 · 2021-11-01T22:30:18Z

/test all

api/internal/target/kusttarget_configplugin.go

natasha41575 · 2021-11-01T22:43:47Z

api/krusty/inlinelabels_test.go

@@ -70,6 +70,7 @@ spec:
    metadata:
      labels:
        a: b
+        c: d


why does foo: bar not show up here?

In this test, /app/deployment.yaml is:

apiVersion: apps/v1 kind: Deployment metadata: name: my-deployment spec: template: spec: containers: - name: my-deployment livenessProbe: httpGet: path: /healthz port: 8080 --- apiVersion: example.dev/v1 kind: MyCRD metadata: name: crd

^here the pod template has no metadata section. But apparently when kustomize runs and adds the a: b label, it creates the pod template metadata section. (a: b pair is configured with includeSelectors: true)

- pairs: a: b includeSelectors: true

After that, the rest of the labels are applied in metadata and template/metadata as expected. The pair foo: bar is declared before a: b therefore it never gets added to template/metadata

@natasha41575 I fixed the test by completing the initial deployment.yaml, as I believe selector and template/metadata are both required fields of the Deployment kind. Let me know if this is correct

k8s-ci-robot · 2021-11-08T20:52:09Z

@aibarbetta: 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.

linux-foundation-easycla · 2021-11-15T21:23:44Z

The committers are authorized under a signed CLA.

✅ Agustina Barbetta (923c21f, 620931c, a8a0e8c)

natasha41575 · 2022-01-21T22:53:09Z

Giving this a little more thought, and though I agree that this is desired behavior, it is a backwards incompatible change and I'm not sure what we can do that at the moment.

To maintain backwards compatibility, I think it would be best to include only metadata/labels by default when includeSelectors is false as it currently does, and perhaps add a new field includeTemplate that is false by default, but when set will also apply labels to spec/template/metadata/labels.

aibarbetta · 2022-01-31T01:13:55Z

Giving this a little more thought, and though I agree that this is desired behavior, it is a backwards incompatible change and I'm not sure what we can do that at the moment.

To maintain backwards compatibility, I think it would be best to include only metadata/labels by default when includeSelectors is false as it currently does, and perhaps add a new field includeTemplate that is false by default, but when set will also apply labels to spec/template/metadata/labels.

thanks for the review @natasha41575, I added the includeTemplates flag, let me know if it looks good

aibarbetta · 2022-03-24T15:57:51Z

@natasha41575 @monopole changes done, can you please review? 🙏

aibarbetta · 2022-04-13T22:25:57Z

@natasha41575 can you review please? 🙏

natasha41575

Overall LGTM. Just a question about why we are changing a test.

Also, it would be good to get @KnVerey's input on the syntax. @KnVerey this PR proposes

labels:
- pairs:
    a: b
  includeSelectors: true # this already exists, and adds all the fieldSpecs from commonLabels
- pairs
    c: d
  includeTemplates: true # new field which will add the label to pod specs, but not matchLabels

natasha41575 · 2022-04-14T00:04:39Z

/lgtm

natasha41575 · 2022-04-14T00:10:47Z

api/internal/target/kusttarget_configplugin.go

+					if err != nil {
+						return nil, err
+					}
+				}


It won't let me comment on it, but the linter is complaining about line 292 below. You can either wrap the error or add //nolint:wrapcheck to the end of the line.

It would probably be better to use errors.Wrap.

seems there was also a failure downloading a module here, can we re-run?

I don't know why the line number is wrong, but I think it is actually complaining about L249. It shouldn't complain about existing code, and the signature of the error in question is func (sigs.k8s.io/kustomize/api/types.FsSlice).MergeOne(x sigs.k8s.io/kustomize/api/types.FieldSpec) (sigs.k8s.io/kustomize/api/types.FsSlice, error).

Regardless, please rebase the PR. We've made major improvements to the linter config since the commit it is current based on, and I'd like to make sure this runs with the latest config before merging.

PR is rebased, I wrapped the error in that line and ran the unit tests locally, can we re-run? 🙏

natasha41575 · 2022-04-19T20:36:42Z

Rerunning the tests!

natasha41575 · 2022-04-19T20:37:20Z

/lgtm

aibarbetta · 2022-04-19T23:06:08Z

Rerunning the tests!

@natasha41575 this is weird, it failed with the same lint error https://github.com/kubernetes-sigs/kustomize/pull/4209/files#annotation_3280759392 did I wrap it correctly?

natasha41575 · 2022-05-04T18:37:04Z

api/internal/target/kusttarget_configplugin.go

@@ -281,7 +289,7 @@ var transformerConfigurators = map[builtinhelpers.BuiltinPluginType]func(
 		p := f()
 		err = kt.configureBuiltinPlugin(p, c, bpt)
 		if err != nil {
-			return nil, err
+			return nil, errors.Wrap(err, "failed to configure transformer")


You can add // nolint:wrapcheck to the end of this line to bypass the lint error. I'm not sure why it's still failing.

KnVerey · 2022-05-04T22:37:57Z

Also, it would be good to get @KnVerey's input on the syntax. @KnVerey this PR proposes

The syntax makes sense to me. 👍
Can you please update the docs as well?

This reverts commit 0a203df.

Documentation for changes introduced in kubernetes-sigs/kustomize#4209

aibarbetta · 2022-05-05T02:31:43Z

Also, it would be good to get @KnVerey's input on the syntax. @KnVerey this PR proposes

The syntax makes sense to me. 👍 Can you please update the docs as well?

I opened a PR to add doc here: kubernetes-sigs/cli-experimental#268

natasha41575 · 2022-05-05T18:15:34Z

/lgtm
/approve

k8s-ci-robot · 2022-05-05T18:15:48Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 27, 2021

k8s-ci-robot requested review from monopole and natasha41575 September 27, 2021 19:09

aibarbetta changed the title ~~[Fixes #3937] Add labels to template/metadata if includeSelectors is false~~ [Fix #3937] Add labels to template/metadata if includeSelectors is false Sep 27, 2021

aibarbetta changed the title ~~[Fix #3937] Add labels to template/metadata if includeSelectors is false~~ [Address #3937] Add labels to template/metadata if includeSelectors is false Sep 27, 2021

k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Sep 27, 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 1, 2021

k8s-ci-robot assigned natasha41575 Nov 1, 2021

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

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

natasha41575 reviewed Nov 1, 2021

View reviewed changes

natasha41575 added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Nov 8, 2021

aibarbetta requested a review from natasha41575 January 20, 2022 16:36

aibarbetta changed the title ~~[Address #3937] Add labels to template/metadata if includeSelectors is false~~ [Address #3937] Add labels to template/metadata if includeTemplates is true Jan 31, 2022

natasha41575 mentioned this pull request Apr 13, 2022

Addition of pod labels without modifying selectors #4546

Closed

natasha41575 reviewed Apr 13, 2022

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 14, 2022

natasha41575 reviewed Apr 14, 2022

View reviewed changes

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 14, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 19, 2022

natasha41575 reviewed May 4, 2022

View reviewed changes

aibarbetta added 8 commits May 4, 2022 22:29

add labels in template/metadata by default

e04257f

update comment

f08744a

fix kustomization labels test

e854cd0

Add spec/template/metadata/labels when includeTemplate is true

8fcae57

remove unnecessary test changes

1f24a35

add error wrap

6908b57

Revert "add error wrap"

3080b32

This reverts commit 0a203df.

add error wrap at template fieldSpec merge

b6c41f6

aibarbetta force-pushed the fix-3937 branch from 0a203df to b6c41f6 Compare May 5, 2022 01:37

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 5, 2022

aibarbetta added a commit to aibarbetta/cli-experimental that referenced this pull request May 5, 2022

Add doc for includeSelectors flag

16fd92c

Documentation for changes introduced in kubernetes-sigs/kustomize#4209

aibarbetta mentioned this pull request May 5, 2022

Add doc for includeSelectors flag kubernetes-sigs/cli-experimental#268

Merged

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 5, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 5, 2022

k8s-ci-robot merged commit 5948f6a into kubernetes-sigs:master May 5, 2022

natasha41575 mentioned this pull request May 11, 2022

How to apply version? #4632

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Address #3937] Add labels to template/metadata if includeTemplates is true #4209

[Address #3937] Add labels to template/metadata if includeTemplates is true #4209

aibarbetta commented Sep 27, 2021 •

edited

Loading

k8s-ci-robot commented Sep 27, 2021

aibarbetta commented Sep 27, 2021

k8s-ci-robot commented Sep 27, 2021

natasha41575 commented Nov 1, 2021

natasha41575 commented Nov 1, 2021

natasha41575 Nov 1, 2021

aibarbetta Nov 15, 2021

aibarbetta Nov 15, 2021

k8s-ci-robot commented Nov 8, 2021

linux-foundation-easycla bot commented Nov 15, 2021 •

edited

Loading

natasha41575 commented Jan 21, 2022 •

edited

Loading

aibarbetta commented Jan 31, 2022

aibarbetta commented Mar 24, 2022

aibarbetta commented Apr 13, 2022

natasha41575 left a comment •

edited

Loading

natasha41575 commented Apr 14, 2022

natasha41575 Apr 14, 2022 •

edited

Loading

aibarbetta Apr 14, 2022

aibarbetta Apr 15, 2022

KnVerey May 4, 2022

aibarbetta May 5, 2022

natasha41575 commented Apr 19, 2022

natasha41575 commented Apr 19, 2022

aibarbetta commented Apr 19, 2022

natasha41575 May 4, 2022

KnVerey commented May 4, 2022

aibarbetta commented May 5, 2022

natasha41575 commented May 5, 2022

k8s-ci-robot commented May 5, 2022

[Address #3937] Add labels to template/metadata if includeTemplates is true #4209

[Address #3937] Add labels to template/metadata if includeTemplates is true #4209

Conversation

aibarbetta commented Sep 27, 2021 • edited Loading

k8s-ci-robot commented Sep 27, 2021

aibarbetta commented Sep 27, 2021

k8s-ci-robot commented Sep 27, 2021

natasha41575 commented Nov 1, 2021

natasha41575 commented Nov 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 8, 2021

linux-foundation-easycla bot commented Nov 15, 2021 • edited Loading

natasha41575 commented Jan 21, 2022 • edited Loading

aibarbetta commented Jan 31, 2022

aibarbetta commented Mar 24, 2022

aibarbetta commented Apr 13, 2022

natasha41575 left a comment • edited Loading

Choose a reason for hiding this comment

natasha41575 commented Apr 14, 2022

natasha41575 Apr 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natasha41575 commented Apr 19, 2022

natasha41575 commented Apr 19, 2022

aibarbetta commented Apr 19, 2022

Choose a reason for hiding this comment

KnVerey commented May 4, 2022

aibarbetta commented May 5, 2022

natasha41575 commented May 5, 2022

k8s-ci-robot commented May 5, 2022

aibarbetta commented Sep 27, 2021 •

edited

Loading

linux-foundation-easycla bot commented Nov 15, 2021 •

edited

Loading

natasha41575 commented Jan 21, 2022 •

edited

Loading

natasha41575 left a comment •

edited

Loading

natasha41575 Apr 14, 2022 •

edited

Loading