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

Fix issues with replacements #3931

Merged

Conversation

natasha41575
Copy link
Contributor

@natasha41575 natasha41575 commented May 27, 2021

First commit demonstrates #3927, second demonstrates #3928.

Third commit fixes #3927, fourth fixes #3928.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 27, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

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

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 27, 2021
@natasha41575 natasha41575 changed the title test to demonstrate multiple fieldpaths issue in replacements Fix multiple fieldpaths issue in replacements May 27, 2021
@natasha41575 natasha41575 requested a review from KnVerey May 27, 2021 19:12
@natasha41575 natasha41575 force-pushed the ReplacementOverwritesSource branch from b9074d1 to f0fbcb2 Compare May 27, 2021 19:13
@k8s-ci-robot 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 May 27, 2021
@natasha41575 natasha41575 force-pushed the ReplacementOverwritesSource branch from c9528ca to d555ce1 Compare May 27, 2021 20:56
@natasha41575 natasha41575 changed the title Fix multiple fieldpaths issue in replacements Fix issues with replacements May 27, 2021
@natasha41575 natasha41575 force-pushed the ReplacementOverwritesSource branch from d555ce1 to 81a5602 Compare May 27, 2021 20:59
@monopole
Copy link
Contributor

@natasha41575 Excellent job in the first commit establishing issue behavior, and then fixing it in a subsequent commit.

@natasha41575 natasha41575 force-pushed the ReplacementOverwritesSource branch 2 times, most recently from 5959e90 to 059c8bb Compare June 1, 2021 20:43
@natasha41575 natasha41575 requested review from monopole and KnVerey June 1, 2021 20:48
@natasha41575 natasha41575 force-pushed the ReplacementOverwritesSource branch from 059c8bb to f8707b3 Compare June 1, 2021 21:02
@natasha41575 natasha41575 force-pushed the ReplacementOverwritesSource branch from f8707b3 to 84724a3 Compare June 2, 2021 00:16
@monopole
Copy link
Contributor

monopole commented Jun 3, 2021

/lgtm
Thanks!

@monopole monopole merged commit 636b9c7 into kubernetes-sigs:master Jun 3, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 3, 2021
@616b2f
Copy link

616b2f commented Jun 10, 2021

Does it also fixes this kind of issues?

source:
  kind: ConfigMap
  name: my-config
  fieldPath: data.my-var
targets:
- select:
    name: my-service
    kind: Service
  fieldPaths:
  - metadata.annotations.["service.beta.kubernetes.io/load-balancer-abc"]

Because this also does not seem to work with kustomize v4.1.3

@natasha41575
Copy link
Contributor Author

@616b2f No it does not. I will work on that. Filed a tracking issue #3983, thank you for using and trying out the feature!

@natasha41575 natasha41575 deleted the ReplacementOverwritesSource branch June 10, 2021 17:47
@natasha41575
Copy link
Contributor Author

natasha41575 commented Jun 10, 2021

@616b2f just as an FYI this PR does provide functionality to escape your delimiters, e.g. service\.beta\.kubernetes\.io/load-balancer-abc, but I have also submitted another PR so that you can use surrounding brackets too.

@616b2f
Copy link

616b2f commented Jun 11, 2021

@natasha41575 thank you for clarification and opening the new bug.

Btw. I love the new feature you implemented, it's really saved us a lot of pain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
5 participants