feat: added first draft of replacement between delimiter #5617

Yingrjimsch · 2024-03-18T11:36:12Z

linux-foundation-easycla · 2024-03-18T11:36:16Z

The committers listed above are authorized under a signed CLA.

✅ login: Yingrjimsch (45f492a, 255e8d2, 4210131, f4441a8)

k8s-ci-robot · 2024-03-18T11:36:21Z

Welcome @Yingrjimsch!

It looks like this is your first PR to kubernetes-sigs/kustomize 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kustomize has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-03-18T11:36:22Z

Hi @Yingrjimsch. 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.

k8s-ci-robot · 2024-03-21T09:26:44Z

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.

Yingrjimsch · 2024-03-21T09:53:27Z

Edit: I decided to add a fulltext replacement as well because it was very similar to the start and enddelimiter. Should not be too much to review and is pretty straightforward.

Yingrjimsch · 2024-03-21T10:02:55Z

/assign @ncapps

Yingrjimsch · 2024-03-26T09:02:32Z

/assign @varshaprasad96

olegmayko · 2024-04-10T08:47:58Z

hi, any ETA on the merge of this PR?

Yingrjimsch · 2024-04-10T08:55:56Z

hi, any ETA on the merge of this PR?

I tried via Slack and Assigning to find someone but until now I got sadly no reaction...

olegmayko · 2024-04-10T08:59:50Z

hi, any ETA on the merge of this PR?

I tried via Slack and Assigning to find someone but until now I got sadly no reaction...

Hi @Yingrjimsch thanks for your contribution, I have been checked how to migrate from vars to replacement recently and found it's not possible for our setup until this or similar feature is available. it's strange that there is not so much noise about this , but maybe noone have tried to migrate yet.

harshsingh32

lgtm

newtondev · 2024-05-15T11:33:51Z

This is a great feature and would love to start using it (kudos for implementing it). There any reviewers that can approve?

antoooks · 2024-06-20T17:10:23Z

/ok-to-test

varshaprasad96 · 2024-06-20T20:01:16Z

api/filters/replacement/replacement.go

+		if options.Delimiter == "" {
+			return nil, fmt.Errorf("delimiter needs to be set if enddelimiter is set")
+		}
+		re := regexp.MustCompile(regexp.QuoteMeta(options.Delimiter) + `(.*?)` + regexp.QuoteMeta(options.EndDelimiter))


This seems to be a bit fragile:

We assume that delimiter and end delimiter are in pairs, but this may not be the case always.

Also what happens when delimiters are not in expected order, or there are nested delimiters.

If not for nested delimiters, for the other cases, can we do any kind of pre-processing?

@varshaprasad96 thank you for the feedback.

The option is that Delimiter comes alone, in this case it stays like it is today, the EndDelimiter should not be used without a Delimiter 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.

The delimiters should always be in expected order because their key (delimiter or enddelimiter) allow us to define the order.

To make it less fragile we could do a preprocessing, if the EndDelimiter is set and the Delimiter is empty they swap values. Do you have another suggestion for preprocessing?

The option is that Delimiter comes alone, in this case it stays like it is today, the EndDelimiter should not be used without a Delimiter 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.

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?

To make it less fragile we could do a preprocessing, if the EndDelimiter is set and the Delimiter is empty they swap values. Do you have another suggestion for preprocessing?

I think this looks good the way it is for now, where we error out if delimiter is not set when end delimiter is.

The delimiters should always be in expected order because their key (delimiter or enddelimiter) allow us to define the order.

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?

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

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 one SourceSelector and multiple TargetSelector everyone of these types contain exactly one FieldOptions type.

If I am missing something just tell me and I will look into it.

varshaprasad96 · 2024-06-20T20:13:11Z

api/filters/replacement/replacement.go

+	return strings.Join(tv, delimiter)
+}
+
+func getByRegex(regex string, target string, source string, index int) string {


Since both these are fragile string manipulation methods, can we have separate tests to verify these and ensure we are capturing the edge cases?

@varshaprasad96 I've written some edge case tests but if you want I can add some tests that handle these functions in specific

EDIT: I've added some tests for the regex function. I hope this is what you imagined. Let me know if you want/need more tests

k8s-ci-robot · 2024-06-21T07:57:02Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Yingrjimsch
Once this PR has been reviewed and has the lgtm label, please ask for approval from varshaprasad96. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

koba1t · 2024-06-30T18:43:55Z

/hold
Sorry, but I add a comment to the feature request issue.

k8s-ci-robot requested review from ncapps and varshaprasad96 March 18, 2024 11:36

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Mar 18, 2024

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 18, 2024

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 18, 2024

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 Mar 21, 2024

Yingrjimsch added 2 commits March 21, 2024 10:30

feat: added first draft of replacement between delimiter

45f492a

fix: added full string replacement

4210131

Yingrjimsch force-pushed the feat/replace-between-delimiters branch from 758a50c to 4210131 Compare March 21, 2024 09:33

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Mar 21, 2024

k8s-ci-robot assigned ncapps Mar 21, 2024

k8s-ci-robot assigned varshaprasad96 Mar 26, 2024

harshsingh32 reviewed May 5, 2024

View reviewed changes

Yingrjimsch requested a review from harshsingh32 June 20, 2024 06:06

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 Jun 20, 2024

varshaprasad96 reviewed Jun 20, 2024

View reviewed changes

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 21, 2024

fix: added edge case tests for regex and reject for source

f4441a8

Yingrjimsch force-pushed the feat/replace-between-delimiters branch from fbd7efe to f4441a8 Compare June 21, 2024 07:59

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jun 21, 2024

chore: fix simple bug

255e8d2

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: added first draft of replacement between delimiter #5617

feat: added first draft of replacement between delimiter #5617

Yingrjimsch commented Mar 18, 2024

linux-foundation-easycla bot commented Mar 18, 2024 •

edited

Loading

k8s-ci-robot commented Mar 18, 2024

k8s-ci-robot commented Mar 18, 2024

k8s-ci-robot commented Mar 21, 2024

Yingrjimsch commented Mar 21, 2024

Yingrjimsch commented Mar 21, 2024

Yingrjimsch commented Mar 26, 2024

olegmayko commented Apr 10, 2024

Yingrjimsch commented Apr 10, 2024

olegmayko commented Apr 10, 2024

harshsingh32 left a comment

newtondev commented May 15, 2024 •

edited

Loading

antoooks commented Jun 20, 2024

varshaprasad96 Jun 20, 2024

Yingrjimsch Jun 21, 2024

varshaprasad96 Jun 21, 2024 •

edited

Loading

Yingrjimsch Jun 25, 2024

Yingrjimsch Jun 27, 2024 •

edited

Loading

varshaprasad96 Jun 20, 2024

Yingrjimsch Jun 21, 2024 •

edited

Loading

k8s-ci-robot commented Jun 21, 2024

koba1t commented Jun 30, 2024

feat: added first draft of replacement between delimiter #5617

Are you sure you want to change the base?

feat: added first draft of replacement between delimiter #5617

Conversation

Yingrjimsch commented Mar 18, 2024

linux-foundation-easycla bot commented Mar 18, 2024 • edited Loading

k8s-ci-robot commented Mar 18, 2024

k8s-ci-robot commented Mar 18, 2024

k8s-ci-robot commented Mar 21, 2024

Yingrjimsch commented Mar 21, 2024

Yingrjimsch commented Mar 21, 2024

Yingrjimsch commented Mar 26, 2024

olegmayko commented Apr 10, 2024

Yingrjimsch commented Apr 10, 2024

olegmayko commented Apr 10, 2024

harshsingh32 left a comment

Choose a reason for hiding this comment

newtondev commented May 15, 2024 • edited Loading

antoooks commented Jun 20, 2024

varshaprasad96 Jun 20, 2024

Choose a reason for hiding this comment

Yingrjimsch Jun 21, 2024

Choose a reason for hiding this comment

varshaprasad96 Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Yingrjimsch Jun 25, 2024

Choose a reason for hiding this comment

Yingrjimsch Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

varshaprasad96 Jun 20, 2024

Choose a reason for hiding this comment

Yingrjimsch Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 21, 2024

koba1t commented Jun 30, 2024

linux-foundation-easycla bot commented Mar 18, 2024 •

edited

Loading

newtondev commented May 15, 2024 •

edited

Loading

varshaprasad96 Jun 21, 2024 •

edited

Loading

Yingrjimsch Jun 27, 2024 •

edited

Loading

Yingrjimsch Jun 21, 2024 •

edited

Loading