Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

convert vars to replacements #3926

Merged

Conversation

natasha41575
Copy link
Contributor

@natasha41575 natasha41575 commented May 26, 2021

addresses #3849

Would appreciate any suggestions for tests. I also was struggling to think of a good way to output the diff. It seems like a bit much to output the entire old build output and the new build output.

Sample output:

$ kustomize edit fix
Fixed fields:
  patchesJson6902 -> patches
  commonLabels -> labels

To convert vars -> replacements, run the command `kustomize edit fix --vars`

WARNING: Converting vars to replacements will potentially overwrite many resource files 
and in some rare scenarios the resulting files may not produce the same output when `kustomize build` is run. 
We recommend doing this in a clean git repository where the change is easy to undo.
$ kustomize edit fix --vars
Fixed fields:
  patchesJson6902 -> patches
  commonLabels -> labels
  vars -> replacements

If there is a difference in the output of kustomize build before and after the change:

$ kustomize edit fix --vars
Fixed fields:
  patchesJson6902 -> patches
  commonLabels -> labels
  vars -> replacements

Warning: 'Fixed' kustomization now produces a different output when running `kustomize build`:
...
<new kustomize build output>
...

ALLOW_MODULE_SPAN

@natasha41575 natasha41575 requested a review from monopole May 26, 2021 17:13
@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 26, 2021
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label May 26, 2021
@natasha41575
Copy link
Contributor Author

/test all

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 26, 2021
@natasha41575 natasha41575 force-pushed the ConvertVarsToReplacements branch from 362dedb to 421f4ff Compare May 26, 2021 17:23
@natasha41575 natasha41575 force-pushed the ConvertVarsToReplacements branch from 421f4ff to 17e56e5 Compare June 3, 2021 00:00
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 3, 2021
@natasha41575 natasha41575 force-pushed the ConvertVarsToReplacements branch 3 times, most recently from f55cc7f to 9e6e1bc Compare June 9, 2021 00:24
@natasha41575 natasha41575 marked this pull request as ready for review June 9, 2021 00:24
@natasha41575 natasha41575 force-pushed the ConvertVarsToReplacements branch 3 times, most recently from fc96cbb to a9ca134 Compare June 9, 2021 00:46
@natasha41575 natasha41575 marked this pull request as draft June 9, 2021 00:49
@natasha41575 natasha41575 force-pushed the ConvertVarsToReplacements branch from a9ca134 to 7d8661d Compare June 9, 2021 00:52
@natasha41575 natasha41575 marked this pull request as ready for review June 9, 2021 00:53
@natasha41575
Copy link
Contributor Author

@monopole would appreciate some initial feedback on this, thanks!

@natasha41575 natasha41575 changed the title WIP: convert vars to replacements convert vars to replacements Jun 9, 2021
@k8s-ci-robot 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 Jun 9, 2021
@natasha41575 natasha41575 changed the title convert vars to replacements WIP: convert vars to replacements Jun 9, 2021
@k8s-ci-robot 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 Jun 9, 2021
@natasha41575 natasha41575 force-pushed the ConvertVarsToReplacements branch from 7d8661d to eb0c3d5 Compare June 9, 2021 22:16
@natasha41575 natasha41575 changed the title WIP: convert vars to replacements convert vars to replacements Jun 9, 2021
@k8s-ci-robot 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 Jun 9, 2021
@natasha41575 natasha41575 requested a review from KnVerey June 9, 2021 22:17
@natasha41575 natasha41575 force-pushed the ConvertVarsToReplacements branch from eb0c3d5 to 46a0144 Compare June 10, 2021 16:17
@monopole monopole self-assigned this Jun 10, 2021
Copy link
Contributor

@monopole monopole left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

Just an ask to move some code around

@@ -263,3 +578,21 @@ func (k *Kustomization) Unmarshal(y []byte) error {
*k = nk
return nil
}

func stringInSlice(elem string, slice []string) bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

api/internal/utils.StringSliceContains

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't be able to use this after moving the code to kustomize/kustomize/commands/edit/fix because it's an internal package

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol, true

return false
}

func indexOf(varName string, slice []string) int {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

api/internal/utils.StringSliceIndex

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

@natasha41575 natasha41575 force-pushed the ConvertVarsToReplacements branch from 46a0144 to 81a755d Compare June 10, 2021 22:21
@natasha41575 natasha41575 requested a review from monopole June 10, 2021 22:23
@natasha41575 natasha41575 force-pushed the ConvertVarsToReplacements branch from 81a755d to f121e74 Compare June 10, 2021 22:29
Copy link
Contributor

@monopole monopole left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 11, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: monopole, 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 [monopole,natasha41575]

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

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants