Allow setting every array element in replacements #4424

koba1t · 2022-01-26T21:19:39Z

Implement to #4053.
And resolve #4414 (comment) and #4391.

k8s-ci-robot · 2022-01-26T21:19:47Z

Hi @koba1t. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

koba1t · 2022-01-28T20:11:46Z

/assign @phanimarupaka

natasha41575 · 2022-01-28T20:17:40Z

/ok-to-test
/assign

natasha41575 · 2022-01-28T20:20:31Z

Would you mind also adding a test to https://github.com/kubernetes-sigs/kustomize/blob/master/api/filters/replacement/replacement_test.go to add some e2e coverage? Thank you for working on this

k8s-ci-robot · 2022-01-30T17:59:49Z

@koba1t: 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.

natasha41575 · 2022-02-01T18:16:00Z

api/filters/replacement/replacement_test.go

+        - name: deployment-name
+          value: sample-deploy`,
+		},
+		"list index contains '*' character": {


what is the difference between this test and the above test?

Also, the test seems not to be passing.

It tests to replace multi values in a single '*' character.

https://github.com/kubernetes-sigs/kustomize/pull/4424/files#diff-909c0819f13e268a60555f0aa78bd8acdbb42c10f3e2a906d62ba4b9d1722947R1515-R1522

https://github.com/kubernetes-sigs/kustomize/pull/4424/files#diff-909c0819f13e268a60555f0aa78bd8acdbb42c10f3e2a906d62ba4b9d1722947R1556-R1563

koba1t · 2022-02-02T18:26:35Z

Sorry for the delay to reply.
But, I found a problem with this code when writing test code.

I think "replacements" couldn't replace for the target to multi values.

e.g.

source

# deploy.yaml
apiVersion: apps/v1
kind: Deployment
metadata:
  labels:
    app: sample-deploy
  name: sample-deploy
spec:
  replicas: 1
  selector:
    matchLabels:
      app: sample-deploy
  template:
    metadata:
      labels:
        app: sample-deploy
    spec:
      containers:
      - image: nginx
        name: main
        env:
        - name: deployment-name
          value: XXXXX
        - name: foo
          value: bar
      - image: nginx
        name: sidecar
        env:
        - name: deployment-name
          value: YYYYY

# kustomization.yaml
resources:
  - deploy.yaml

replacements:
- source:
    kind: Deployment
    name: sample-deploy
    fieldPath: metadata.name
  targets:
  - select:
      kind: Deployment
    fieldPaths:
    - spec.template.spec.containers.[image=nginx].env.[name=deployment-name].value

result

apiVersion: apps/v1
kind: Deployment
metadata:
  labels:
    app: sample-deploy
  name: sample-deploy
spec:
  replicas: 1
  selector:
    matchLabels:
      app: sample-deploy
  template:
    metadata:
      labels:
        app: sample-deploy
    spec:
      containers:
      - env:
        - name: deployment-name
          value: sample-deploy
        - name: foo
          value: bar
        image: nginx
        name: main
      - env:
        - name: deployment-name
          value: YYYYY
        image: nginx
        name: sidecar

It's only to replace the first element in deploy.yaml.

I think it's needed to fix when using this function. And I'm trying it now.

koba1t · 2022-02-03T19:30:11Z

Hello @natasha41575.
Every test case was passing. I think this work is complete.

Sorry for to delay.
If you have time, please re-check this PR.

natasha41575 · 2022-02-07T19:34:45Z

If you have time, please re-check this PR.

Thanks for the update! I have been out for a few days, will try to take a look soon.

natasha41575

Some very minor nits, apart from that LGTM.

natasha41575 · 2022-02-08T22:18:20Z

api/filters/replacement/replacement.go

@@ -119,20 +119,42 @@ func applyToNode(node *yaml.RNode, value *yaml.RNode, target *types.TargetSelect
 		if target.Options != nil && target.Options.Create {
 			t, err = node.Pipe(yaml.LookupCreate(value.YNode().Kind, fieldPath...))
 		} else {
-			t, err = node.Pipe(yaml.Lookup(fieldPath...))
+			// t, err = node.Pipe(yaml.Lookup(fieldPath...))


Did you forget to remove this?

Yes. I'll delete it.

natasha41575 · 2022-02-08T22:20:21Z

kyaml/yaml/fns.go

@@ -796,6 +803,12 @@ func SplitIndexNameValue(p string) (string, string, error) {
 	return parts[0], parts[1], nil
 }

+// IsMatchEveryIndex returns true if p is matching every elements.
+// e.g. "*"
+func IsMatchEveryIndex(p string) bool {


Suggest changing the name here to IsWildcard and moving this up to line 791 to keep the IsXXXX functions together

I agree with you. I fix it.

koba1t · 2022-02-09T20:28:20Z

I fix any mistake from your review and add the above testcase(#4424 (comment)).

natasha41575 · 2022-02-10T00:52:15Z

/lgtm
/approve

Thanks!

k8s-ci-robot · 2022-02-10T00:52:39Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

nprokopic · 2022-02-14T08:31:23Z

So nice to see this implemented, thanks @koba1t! 🎉

afirth · 2022-02-17T18:51:40Z

you rock @koba1t

Allow setting every array element in replacements

26b9af0

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 26, 2022

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 26, 2022

k8s-ci-robot requested review from mengqiy and phanimarupaka January 26, 2022 21:20

k8s-ci-robot assigned phanimarupaka Jan 28, 2022

k8s-ci-robot assigned natasha41575 Jan 28, 2022

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 28, 2022

add replacements every element match test

b79d77a

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 30, 2022

natasha41575 reviewed Feb 1, 2022

View reviewed changes

replacements allow to replace multi values

22f9daa

natasha41575 mentioned this pull request Feb 7, 2022

Extend API to support wildcard match/replace on FieldSpec level. #4456

Closed

natasha41575 reviewed Feb 8, 2022

View reviewed changes

rename IsMatchEveryIndex to IsWildcard

5ed96a3

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

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

natasha41575 mentioned this pull request Feb 10, 2022

Allow setting every array element in replacements #4053

Closed

k8s-ci-robot merged commit ff40460 into kubernetes-sigs:master Feb 10, 2022

koba1t deleted the feature/allow_setting_every_array_element_in_replacements branch February 10, 2022 02:23

koba1t mentioned this pull request Feb 13, 2022

docs: replacements to all array elements kubernetes-sigs/cli-experimental#240

Merged

natasha41575 mentioned this pull request Apr 7, 2022

Relax "create: true" constraints #4574

Closed

natasha41575 mentioned this pull request Jun 7, 2022

replacements: fix issue with create: true option when there is an existing field #4667

Merged

koba1t mentioned this pull request Jul 12, 2022

REQUEST: New membership for koba1t kubernetes/org#3554

Closed

9 tasks

macetw mentioned this pull request Oct 11, 2023

Documented wildcard matching for replacement is broken #5375

Closed

natasha41575 mentioned this pull request Jan 17, 2024

Replacements doc for key-value pair indexes doesn't mention regex and has misleading examples #5349

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow setting every array element in replacements #4424

Allow setting every array element in replacements #4424

koba1t commented Jan 26, 2022 •

edited

Loading

k8s-ci-robot commented Jan 26, 2022

koba1t commented Jan 28, 2022

natasha41575 commented Jan 28, 2022

natasha41575 commented Jan 28, 2022

k8s-ci-robot commented Jan 30, 2022

natasha41575 Feb 1, 2022

koba1t Feb 2, 2022 •

edited

Loading

koba1t commented Feb 2, 2022 •

edited

Loading

koba1t commented Feb 3, 2022

natasha41575 commented Feb 7, 2022

natasha41575 left a comment

natasha41575 Feb 8, 2022

koba1t Feb 9, 2022

natasha41575 Feb 8, 2022

koba1t Feb 9, 2022

koba1t commented Feb 9, 2022

natasha41575 commented Feb 10, 2022

k8s-ci-robot commented Feb 10, 2022

nprokopic commented Feb 14, 2022

afirth commented Feb 17, 2022

Allow setting every array element in replacements #4424

Allow setting every array element in replacements #4424

Conversation

koba1t commented Jan 26, 2022 • edited Loading

k8s-ci-robot commented Jan 26, 2022

koba1t commented Jan 28, 2022

natasha41575 commented Jan 28, 2022

natasha41575 commented Jan 28, 2022

k8s-ci-robot commented Jan 30, 2022

natasha41575 Feb 1, 2022

Choose a reason for hiding this comment

koba1t Feb 2, 2022 • edited Loading

Choose a reason for hiding this comment

koba1t commented Feb 2, 2022 • edited Loading

source

result

koba1t commented Feb 3, 2022

natasha41575 commented Feb 7, 2022

natasha41575 left a comment

Choose a reason for hiding this comment

natasha41575 Feb 8, 2022

Choose a reason for hiding this comment

koba1t Feb 9, 2022

Choose a reason for hiding this comment

natasha41575 Feb 8, 2022

Choose a reason for hiding this comment

koba1t Feb 9, 2022

Choose a reason for hiding this comment

koba1t commented Feb 9, 2022

natasha41575 commented Feb 10, 2022

k8s-ci-robot commented Feb 10, 2022

nprokopic commented Feb 14, 2022

afirth commented Feb 17, 2022

koba1t commented Jan 26, 2022 •

edited

Loading

koba1t Feb 2, 2022 •

edited

Loading

koba1t commented Feb 2, 2022 •

edited

Loading