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

use field options to refine replacements #3773

Merged

Conversation

natasha41575
Copy link
Contributor

@natasha41575 natasha41575 commented Apr 2, 2021

#3492, #3772

Use delimiter, index, and create options.

@monopole I was hoping you could clarify what you intended for the encoding option to do (from this comment).

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 2, 2021
@natasha41575 natasha41575 linked an issue Apr 5, 2021 that may be closed by this pull request
@natasha41575 natasha41575 force-pushed the ReplacementFieldOptions branch 2 times, most recently from 9282e4d to 4f8425c Compare April 7, 2021 21:03
@natasha41575
Copy link
Contributor Author

@monopole updated to allow prefixing/suffixing and added a test with a path

@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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 8, 2021
@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 Apr 8, 2021
api/filters/replacement/replacement.go Outdated Show resolved Hide resolved
}
value := strings.Split(yaml.GetValue(rn), options.Delimiter)
var index int
if options.Index > len(value) || options.Index < 0 { // take the last element
Copy link
Contributor

@KnVerey KnVerey Apr 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also seems like invalid configuration to me that should result in an error. It makes sense to provide a way to target the last element regardless of how many there are, but I'd say that should be done with a specific value, i.e. -1, or for negative indexes to be supported in general. But I feel like > current length and < 0 both meaning "last" could get confusing, especially given that current length can change independently of the Kustomization this is written in.

Edit: making negative indexing select the end when specified in source and indicate a prefix in target is also confusing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated it to throw an error if the source index is out of bounds

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good idea to be relaxed about inputs (and strict on outputs).

However, I cannot think of a no-surprises default behavior for an out-of-range index after the split when trying to get the source value.

So - seems that it should error (fail build). Meaning that a split resulting in an empty array is always an error.

@monopole
Copy link
Contributor

monopole commented Apr 9, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 9, 2021
@k8s-ci-robot k8s-ci-robot merged commit 264b3ff into kubernetes-sigs:master Apr 9, 2021
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/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.

Implement use of reject and options fields in replacement filter
4 participants