[address #4717] Re-do create template/metadata when using includeTemplates if not present #4805

aibarbetta · 2022-09-20T16:41:36Z

Hi all, I'm attempting to re-do #4751 to fix #4717, I added a new test to reproduce the bug the old PR had.

It's not clear to me how can I add resource-specific fieldspecs for the includeTemplates flag in the labels plugin (as @KnVerey and @wolfmah mentioned here and here), can anyone provide some guidance?

Thanks!

k8s-ci-robot · 2022-09-20T16:41:37Z

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

k8s-ci-robot · 2022-09-20T16:41:45Z

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.

KnVerey · 2022-09-20T17:31:34Z

It's not clear to me how can I add resource-specific fieldspecs for the includeTemplates flag in the labels plugin (as @KnVerey and @wolfmah mentioned #4751 (comment) and #4751 (comment)), can anyone provide some guidance?

The full set of specs you'll need can be extracted from the CommonLabels config here. Look specifically at paths that end in /metadata/labels. I suggest trying to extract those ones into a separate constant, that then gets concatenated onto commonLabelFieldSpecs and a new templateLabelFieldSpecs for you to use. You'll then need to expose the templateLabelFieldSpecs through the functions in this file. And finally load them with fss.MergeAll like the function you're modifying is doing a couple lines earlier. Please include the various types this PR will support in your tests, in addition to Deployment.

aibarbetta · 2022-09-23T19:24:10Z

Thanks for the help @KnVerey! Changes are done, I added some tests for the rest of the types and they all pass. Can I get an ok-to-test to run the full suite here?

KnVerey · 2022-09-23T19:52:22Z

/ok-to-test

aibarbetta · 2022-09-23T21:59:08Z

@KnVerey make lint runs successfully now, can we retest? 🙏🏻

koba1t · 2022-09-24T02:59:38Z

api/internal/plugins/builtinconfig/transformerconfig.go

@@ -7,6 +7,7 @@ import (
 	"log"
 	"sort"

+	"github.com/pkg/errors"


"github.com/pkg/errors" looks like an already unmaintained and archived.

Could you think of using fmt.Errorf() instead of errors.Wrap() if you don't have any reason?

"github.com/pkg/errors" looks like an already unmaintained and archived.

Could you think of using fmt.Errorf() instead of errors.Wrap() if you don't have any reason?

done! thanks for making me aware of that

FYI the other errors package we often use in Kustomize is our own "sigs.k8s.io/kustomize/kyaml/errors", which provides errors.WrapPrefixf(err, msg, args...) for situations like these. We have plenty of fmt.Errorf usage as well though, so I don't feel strongly about this.

aibarbetta · 2022-09-29T21:16:12Z

@KnVerey linter fix is done, can we retest?

aibarbetta · 2022-09-30T18:48:57Z

/test all

KnVerey · 2022-10-04T20:34:01Z

Nice work! I have a couple very minor suggestions and will put a hold in case you want to take them (otherwise the bot will auto-merge immediately).

/hold
/lgtm
/approve

k8s-ci-robot · 2022-10-04T20:34:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aibarbetta, 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:

~~OWNERS~~ [KnVerey]

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

aibarbetta · 2022-10-07T16:38:39Z

Nice work! I have a couple very minor suggestions and will put a hold in case you want to take them (otherwise the bot will auto-merge immediately).

/hold /lgtm /approve

Hi @KnVerey sure, please send me the suggestions

KnVerey

Seems like I accidentally didn't click the submit button on this review. Sorry for the confusion!

api/krusty/inlinelabels_test.go

KnVerey · 2022-10-04T20:13:14Z

api/krusty/inlinelabels_test.go

+    app.kubernetes.io/component: a
+    app.kubernetes.io/instance: b
+    app.kubernetes.io/name: c
+    app.kubernetes.io/part-of: d


Suggestion: for these extra Kinds tests, start with fewer labels to make it easier to see the before/after. In particular, I was confused at first because this test is starting with the exact same labels that are added by the transformer in the other tests.

KnVerey · 2022-10-04T20:31:36Z

api/internal/plugins/builtinconfig/transformerconfig.go

@@ -7,6 +7,7 @@ import (
 	"log"
 	"sort"

+	"github.com/pkg/errors"


FYI the other errors package we often use in Kustomize is our own "sigs.k8s.io/kustomize/kyaml/errors", which provides errors.WrapPrefixf(err, msg, args...) for situations like these. We have plenty of fmt.Errorf usage as well though, so I don't feel strongly about this.

Co-authored-by: Katrina Verey <[email protected]>

aibarbetta · 2022-10-08T23:40:28Z

Thanks for the suggestions @KnVerey! all done :)

KnVerey · 2022-10-11T16:08:08Z

/unhold
/lgtm

Thank you for all your work on this one!

aibarbetta added 2 commits September 20, 2022 13:29

add test for resources with no template

662ccf1

create template/metadata in includeTemplates if not present

4dcc040

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 20, 2022

k8s-ci-robot requested review from natasha41575 and yuwenma September 20, 2022 16:41

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 20, 2022

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 20, 2022

aibarbetta added 2 commits September 23, 2022 15:11

separate template specific field specs and use in includeTemplates

e2196d9

add tests for other kinds

344b257

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 23, 2022

aibarbetta marked this pull request as ready for review September 23, 2022 18:55

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 23, 2022

k8s-ci-robot requested a review from KnVerey September 23, 2022 18:56

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 Sep 23, 2022

error wraps for linter

2b9ef61

koba1t reviewed Sep 24, 2022

View reviewed changes

aibarbetta added 2 commits September 25, 2022 09:07

use fmt instead of errors

c750c00

use %w to format errors

eb1529b

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 4, 2022

k8s-ci-robot assigned KnVerey Oct 4, 2022

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

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

KnVerey reviewed Oct 7, 2022

View reviewed changes

Update api/krusty/inlinelabels_test.go

acba8ff

Co-authored-by: Katrina Verey <[email protected]>

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

aibarbetta added 2 commits October 8, 2022 20:37

Use WrapPrefixf

d14dfb6

Use fewer labels to ease reading

7b84613

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Oct 11, 2022

k8s-ci-robot merged commit bf7a0f9 into kubernetes-sigs:master Oct 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[address #4717] Re-do create template/metadata when using includeTemplates if not present #4805

[address #4717] Re-do create template/metadata when using includeTemplates if not present #4805

aibarbetta commented Sep 20, 2022

k8s-ci-robot commented Sep 20, 2022

k8s-ci-robot commented Sep 20, 2022

KnVerey commented Sep 20, 2022

aibarbetta commented Sep 23, 2022

KnVerey commented Sep 23, 2022

aibarbetta commented Sep 23, 2022

koba1t Sep 24, 2022

aibarbetta Sep 25, 2022

KnVerey Oct 4, 2022

aibarbetta commented Sep 29, 2022

aibarbetta commented Sep 30, 2022

KnVerey commented Oct 4, 2022

k8s-ci-robot commented Oct 4, 2022

aibarbetta commented Oct 7, 2022

KnVerey left a comment

KnVerey Oct 4, 2022

KnVerey Oct 4, 2022

aibarbetta commented Oct 8, 2022

KnVerey commented Oct 11, 2022

[address #4717] Re-do create template/metadata when using includeTemplates if not present #4805

[address #4717] Re-do create template/metadata when using includeTemplates if not present #4805

Conversation

aibarbetta commented Sep 20, 2022

k8s-ci-robot commented Sep 20, 2022

k8s-ci-robot commented Sep 20, 2022

KnVerey commented Sep 20, 2022

aibarbetta commented Sep 23, 2022

KnVerey commented Sep 23, 2022

aibarbetta commented Sep 23, 2022

koba1t Sep 24, 2022

Choose a reason for hiding this comment

aibarbetta Sep 25, 2022

Choose a reason for hiding this comment

KnVerey Oct 4, 2022

Choose a reason for hiding this comment

aibarbetta commented Sep 29, 2022

aibarbetta commented Sep 30, 2022

KnVerey commented Oct 4, 2022

k8s-ci-robot commented Oct 4, 2022

aibarbetta commented Oct 7, 2022

KnVerey left a comment

Choose a reason for hiding this comment

KnVerey Oct 4, 2022

Choose a reason for hiding this comment

KnVerey Oct 4, 2022

Choose a reason for hiding this comment

aibarbetta commented Oct 8, 2022

KnVerey commented Oct 11, 2022