Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Feature: Improve robustness and capabilities of variable inlining #1208

Closed
wants to merge 2 commits into from
Closed

Feature: Improve robustness and capabilities of variable inlining #1208

wants to merge 2 commits into from

Conversation

jbrette
Copy link
Contributor

@jbrette jbrette commented Jun 19, 2019

This PR is addressing the following issue: #1190

It allows add the ability to inline/copy a tree first from one K8s object to another and let the standard kustomize transformation proceed.

This is really useful because it allows the reuse of complex tree structure (for instance PodTemplate) accross K8s object and does not force the user to create one variable per field.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 19, 2019
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 19, 2019
@jbrette jbrette force-pushed the inline branch 2 times, most recently from 427948a to b6bd23a Compare June 19, 2019 18:43
@jbrette
Copy link
Contributor Author

jbrette commented Jun 19, 2019

gentle quick to retriger build

@k8s-ci-robot 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 Jun 19, 2019
@ian-howell ian-howell force-pushed the inline branch 3 times, most recently from ccb9ce7 to b0f0a4f Compare June 19, 2019 19:58
monopole
monopole reviewed Jun 19, 2019
View reviewed changes
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.

Nice tests!

pkg/transformers/refvars.go Outdated Show resolved Hide resolved
pkg/transformers/refvars.go Outdated Show resolved Hide resolved
pkg/transformers/refvars.go Outdated Show resolved Hide resolved
pkg/transformers/refvars_helper.go Outdated Show resolved Hide resolved
pkg/transformers/refvars.go Outdated Show resolved Hide resolved
pkg/transformers/refvars.go Outdated Show resolved Hide resolved
pkg/transformers/refvars.go Outdated Show resolved Hide resolved
@jbrette
Copy link
Contributor Author

jbrette commented Jun 19, 2019 •
edited
Loading

@monopole We were really happy to be able to use kustomize as an improvement over one of the component of our product architecture. To give more better understanding of what we are trying to achieve, we joined three examples of the usage of that feature. In each case, in order to respect the yaml syntax, we could not do:

construct:
   parent-node: $(var-to-inline)
       child-field: child-value

we had to use a more convoluted struct

construct:
   parent-node: 
      parent-inline: $(var-to-inline)
      child-field: child-value

Here are the examples:

  1. Check out: https://github.com/keleustes/airship-treasuremap/tree/inline and in particular:
    https://github.com/keleustes/airship-treasuremap/blob/inline/global/config/endpoints.yaml#L132 which should be replaced by: https://github.com/keleustes/airship-treasuremap/blob/inline/global/config/service_accounts.yaml#L227

  2. More simple version of the same concept:
    https://github.com/keleustes/kustomize/blob/allinone/examples/allinone/base/wordpress/deployment.yaml#L15

  3. The example we added in the pkg/target:
    https://github.com/keleustes/kustomize/blob/inline/pkg/target/variableref_test.go#L130

@jbrette jbrette force-pushed the inline branch 3 times, most recently from 9d947fd to 43778e3 Compare June 20, 2019 03:45
@jbrette
Copy link
Contributor Author

jbrette commented Jun 20, 2019

/assign @monopole

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jbrette
To complete the pull request process, please assign monopole
You can assign the PR to them by writing /assign @monopole in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

@jbrette jbrette force-pushed the inline branch 3 times, most recently from 76f3d57 to 435cb7e Compare June 21, 2019 22:28
@jbrette jbrette force-pushed the inline branch 2 times, most recently from de0bb12 to 485fd21 Compare September 18, 2019 21:49
@jbrette jbrette mentioned this pull request Sep 22, 2019
Closed
@jbrette jbrette force-pushed the inline branch 6 times, most recently from 1e6a186 to 6541fb1 Compare September 27, 2019 18:00
@tkellen
Copy link
Contributor

tkellen commented Sep 30, 2019

@monopole / @Liujingfang1 Apologies for the repeated pings over the last few months but, is there any chance either of you will be giving this or #1217 a review before the end of the year?

@jbrette jbrette force-pushed the inline branch 4 times, most recently from 15ff784 to 71d0075 Compare October 8, 2019 01:59
@jbrette jbrette force-pushed the inline branch 7 times, most recently from de8337d to 960a05f Compare October 13, 2019 02:39
@jbrette jbrette mentioned this pull request Oct 13, 2019
Closed
@jbrette
Travis build cleanup
62f92a2 
- Makefile needs to be updated after change in kustomize organization
- Remove mdrip, blackfriday from kustomize dependencies
@jbrette
Inline: Streamline inlining and parent-inlining capabilities
5ff67d4 
- Streamline inline implementation, no need for new fields
- Upgrade replaceVars methods to deal with non primitive cases
- Add deepCopy when inlining non primitiv types
- Add tests showcasing usage of parent-inlining of variable

Inline: Add more tests for refvars
Inline: Update the recurse inlining test

- Seems that order of fieldspec and replacement algorithm is important
- Went up to four level4 of redirection, and algorithm seems to work.

Inline: Update to account for code reviews.

- rename methods.
- add more comments.
- return errors instead of logging statements.
- remove noops.

Inline: Add another enhanced inlining, test converage of issue 0952
Inline: Add composition example to inline test.
Inline: Update Deployment from extensions/v1beta1 to apps/v1
@jbrette jbrette changed the title Improve robustness and capabilities of variable inlining Inlining: Improve robustness and capabilities of variable inlining Oct 14, 2019
@jbrette jbrette changed the title Inlining: Improve robustness and capabilities of variable inlining Feature: Improve robustness and capabilities of variable inlining Oct 14, 2019
@jbrette
Copy link
Contributor Author

jbrette commented Oct 15, 2019

Seems that PR simplifying the usage of kustomize can't merge. Closing it.

@jbrette jbrette closed this Oct 15, 2019
@Datamance Datamance mentioned this pull request Feb 14, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@monopole monopole monopole left review comments

@ian-howell ian-howell ian-howell left review comments

Assignees

@monopole monopole

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jbrette @k8s-ci-robot @monopole @rdubya16 @tkellen @ian-howell