-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
feat: added first draft of replacement between delimiter #5617
Open
Yingrjimsch
wants to merge
4
commits into
kubernetes-sigs:master
Choose a base branch
from
Yingrjimsch:feat/replace-between-delimiters
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
45f492a
feat: added first draft of replacement between delimiter
Yingrjimsch 4210131
fix: added full string replacement
Yingrjimsch f4441a8
fix: added edge case tests for regex and reject for source
Yingrjimsch 255e8d2
chore: fix simple bug
Yingrjimsch File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be a bit fragile:
If not for nested delimiters, for the other cases, can we do any kind of pre-processing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@varshaprasad96 thank you for the feedback.
Delimiter
comes alone, in this case it stays like it is today, theEndDelimiter
should not be used without aDelimiter
if so we return an error and if they are used in combination, the value in between them is loaded. If I'm not mistaken all cases should be handled.EndDelimiter
is set and theDelimiter
is empty they swap values. Do you have another suggestion for preprocessing?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we do have a check for that. But what happens when there are unequal number of delimiters and end-delimiters? The regex would still compile, but I'm wondering that there would be some unexpected behaviours? Which is why would it be better if we count for each of those occurrences to ensure they work appear as pairs?
I think this looks good the way it is for now, where we error out if delimiter is not set when end delimiter is.
Can we have checks to ensure that they are in the order as expected, especially when there is a nested scenario - where we have multiple of delimiters and enddelimiters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@varshaprasad96 I will look into this this week and give you a feedback. If I see a possibility to ensure the order and the amount of delimiters I would implement it so you can review it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @varshaprasad96 I've looked into it and wanted to ask if you could provide a nested example for me. As I have seen the delimiters are used in
FieldOptions
https://github.com/kubernetes-sigs/kustomize/blob/master/api/types/replacement.go#L67 which consists of primitive datatypes variables, therefore I don't see how multiple delimiters or nesting could accure. The replacement consists of oneSourceSelector
and multipleTargetSelector
everyone of these types contain exactly oneFieldOptions
type.If I am missing something just tell me and I will look into it.