enable extended patch transformer and add tests #1355
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Liujingfang1 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Liujingfang1
requested review from
damienr74
and removed request for
mengqiy and
monopole
k8s-ci-robot
added
the
size/XL
| @@ -200,7 +200,6 @@ func (k *Kustomization) EnforceFields() []string { | |||
| // new field names. | |||
| func FixKustomizationPreUnmarshalling(data []byte) []byte { | |||
| deprecateFieldsMap := map[string]string{ | |||
| "patches:": "patchesStrategicMerge:", | |||
There was a problem hiding this comment.
Hmm... It seems patches field was deprecated and patchesStrategicMerge is the replacement for it.
But now we want to reuse patches field as a different meaning.
There was a problem hiding this comment.
It might be a breaking change for "lazy" people that haven't migrated yet.
There was a problem hiding this comment.
I've asked about this as well. The patches keyword has been removed since kustomize v2.0.0, so I think this is fine.
There was a problem hiding this comment.
As I've seen, many users already changed patches to patchesStrategicMerge. Maybe @damienr74 can provide some data for how many patches are used in kustomization.yaml file.
There was a problem hiding this comment.
On public github it appears there's still about 1k of them out of ~6k. But in any case, they would already be broken for kustomize v2+, so I'm not sure there's much of a concern for this.
There was a problem hiding this comment.
@damienr74 @mengqiy We can address this issue by kustomize edit fix command to convert the old patches to the new patches format. I will have a separate PR for it.
| name: busybox | ||
| `) | ||
| th.WriteF("/app/base/patch.yaml", ` | ||
| apiVersion: apps/v1beta2 |
There was a problem hiding this comment.
Can we shrink this patch to
metadata:
name: busybox
annotations:
new-key: new-value
or
metadata:
annotations:
new-key: new-value
There was a problem hiding this comment.
We couldn't do that. If we remove name, then it's not a valid strategic merge patch.
| kind: Deployment | ||
| `) | ||
| th.WriteF("/app/base/patch.yaml", ` | ||
| apiVersion: apps/v1beta2 |
k8s-ci-robot
added
size/XXL
k8s-ci-robot
added
the
lgtm
It turns out we (kubebuilder) are the "lazy" people, we are still using the deprecates
This seems to be not true. Deprecated |
Updates the example for patching multiple objects to match the implementation in kubernetes-sigs#1355, which supports name as a regular expression (not wildcard pattern). Fixes kubernetes-sigs#4258.
Updates the example for patching multiple objects to match the implementation in kubernetes-sigs#1355, which supports name as a regular expression (not wildcard pattern).
part of #720
enable extended patch transformer in kustomization by adding and triggering the config function
add unit tests for extended transformer to select targets