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.

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

Skip kustomize transformers for paths #1491

Closed
wants to merge 19 commits into from

Conversation

jbrette
Copy link
Contributor

@jbrette jbrette commented Aug 30, 2019

This PR attempts to fix: Skip kustomize transformers for paths #896

It leverage the work done in #1485

It aims to address the following issue:
Make cluster level kind configurable #617
Do not add namespace to cluster scoped CRDs #552
Skipping nameprefix for some resource kind #519
Adding labels only to metadata.labels #330
Skip adding labels to certain paths #519
Unable to disable commonLabels injection using transformer config: Error: conflicting fieldspecs #817

@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 Aug 30, 2019
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Aug 30, 2019
@jbrette
Copy link
Contributor Author

jbrette commented Aug 30, 2019

/assign @Liujingfang1

@Liujingfang1
Copy link
Contributor

@jbrette I don't think we agree on which solution kustomize should go with. This is a very important feature. Let's think about it carefully before putting any implementation for it. Would you like open an KEP for this? List and compare different solutions. Then we can bring it into discussion in a SIG-CLI meeting and have people agree on the best solution.

@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 liujingfang1
You can assign the PR to them by writing /assign @liujingfang1 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 fsskip branch 2 times, most recently from adf6ddd to 94ead03 Compare August 31, 2019 06:01
@jbrette
Copy link
Contributor Author

jbrette commented Aug 31, 2019

@Liujingfang1 @richardmarshall @sethp-nr I will open the KEP.

@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 Aug 31, 2019
@jbrette jbrette force-pushed the fsskip branch 2 times, most recently from 4cd6cb7 to d856191 Compare September 3, 2019 19:55
@jbrette
Copy link
Contributor Author

jbrette commented Sep 4, 2019

@monopole @Liujingfang1 @richardmarshall @sethp-nr As agreed, here is the corresponding KEP

@jbrette jbrette changed the title WIP: skip kustomize transformers for paths Skip kustomize transformers for paths Sep 4, 2019
@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 Sep 4, 2019
@Liujingfang1 Liujingfang1 added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 5, 2019
@jbrette jbrette force-pushed the fsskip branch 3 times, most recently from e8d0435 to 6118090 Compare September 10, 2019 13:21
@jbrette jbrette force-pushed the fsskip branch 4 times, most recently from 64bb752 to c1cf89c Compare October 11, 2019 00:29
@jbrette jbrette force-pushed the fsskip branch 2 times, most recently from 9fd7543 to a4f05bf Compare October 12, 2019 15:16
jbrette and others added 19 commits October 12, 2019 23:20
Add "add", "replace" and "remove" behavior to transformerconfig.
Introduces a new config field, `namePrefixSuffixSkip`, that takes a list
of gvk arguments. For example:

```
namePrefixSuffixSkip:
- apiVersion: storage.k8s.io/v1
  kind: StorageClass
```

For any matching group/version and kind, kustomize will preserve the
original resource name even if `namePrefix` and `nameSuffix` are
defined.
For types that give special meaning to an empty selector, having create=true overwrites the implicit default labels
@nlamirault
Copy link
Contributor

@jbrette why close this PR ?

@jbrette
Copy link
Contributor Author

jbrette commented Oct 13, 2019

@nlamirault I can't recollect a single "Feature PR" merged to kustomize even when we took the time to open KEP in kubernetes/sig-cli, it was addressing multiple projects needs and issues. We had quite a lot of "Bug Fixes PR" merged, but no feature one.

So, like the autovar feature #1217 , the inlining feature #1208 , the diamond import #1316 , the SMP for CRD feature, this feature PR was on course to rot. Hence it did not make any sense trying to keep it small to ease the reviewing. It was causing us internal merge issues with additional improvements and bug fixes waiting in line behind that PR.

Since our project needs those features, we will unfortunately have to maintain them until something equivalent is officially integrated Kustomize.

Meanwhile in order to reduce our workload, we squashed all the commits and divided by two the numbers of branches we have to maintain. The code for this particular PR has been merged with actual bug fixes here: transformers branch. We pull all those features together and the associated additional regressions tests in this branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants