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.

Sign up for GitHub

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

Jump to bottom

feat: added first draft of replacement between delimiter #5617

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

feat: added first draft of replacement between delimiter #5617

Show file tree
Hide file tree
Changes from 2 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 Mar 18, 2024
4210131
fix: added full string replacement
Yingrjimsch Mar 21, 2024
f4441a8
fix: added edge case tests for regex and reject for source
Yingrjimsch Jun 21, 2024
255e8d2
chore: fix simple bug
Yingrjimsch Jun 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
73 changes: 61 additions & 12 deletions api/filters/replacement/replacement.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package replacement
import (
"fmt"
"strings"
"regexp"

"sigs.k8s.io/kustomize/api/internal/utils"
"sigs.k8s.io/kustomize/api/resource"
Expand Down Expand Up @@ -39,6 +40,10 @@ func (f Filter) Filter(nodes []*yaml.RNode) ([]*yaml.RNode, error) {
}

func getReplacement(nodes []*yaml.RNode, r *types.Replacement) (*yaml.RNode, error) {
if r.Source.FullText != ""{
rn := yaml.NewScalarRNode(r.Source.FullText)
return rn, nil
}
source, err := selectSourceNode(nodes, r.Source)
if err != nil {
return nil, err
Expand Down Expand Up @@ -93,7 +98,24 @@ func getRefinedValue(options *types.FieldOptions, rn *yaml.RNode) (*yaml.RNode,
if rn.YNode().Kind != yaml.ScalarNode {
return nil, fmt.Errorf("delimiter option can only be used with scalar nodes")
}
value := strings.Split(yaml.GetValue(rn), options.Delimiter)
value := []string{}
if options.EndDelimiter == "" {
value = strings.Split(yaml.GetValue(rn), options.Delimiter)
} else {
mapper := func(s string) string {
s = strings.ReplaceAll(s, options.Delimiter, "")
s = strings.ReplaceAll(s, options.EndDelimiter, "")
return s
}
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))
Copy link
Member

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:

  • 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?

Copy link
Author

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.

  • 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?

Copy link
Member

@varshaprasad96 varshaprasad96 Jun 21, 2024 •
edited

Choose a reason for hiding this comment

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

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?

Copy link
Author

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.

Copy link
Author

@Yingrjimsch Yingrjimsch Jun 27, 2024 •
edited

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

dv := re.FindAllString(yaml.GetValue(rn), -1)
for _, s := range dv {
value = append(value, mapper(s))
}
}
if options.Index >= len(value) || options.Index < 0 {
return nil, fmt.Errorf("options.index %d is out of bounds for value %s", options.Index, yaml.GetValue(rn))
}
Expand Down Expand Up @@ -220,22 +242,20 @@ func fieldRetrievalError(fieldPath string, isCreate bool) string {

func setFieldValue(options *types.FieldOptions, targetField *yaml.RNode, value *yaml.RNode) error {
value = value.Copy()
if options != nil && options.Delimiter != "" {
if options != nil && (options.Delimiter != "" || options.FullText != "") {
if targetField.YNode().Kind != yaml.ScalarNode {
return fmt.Errorf("delimiter option can only be used with scalar nodes")
}
tv := strings.Split(targetField.YNode().Value, options.Delimiter)
v := yaml.GetValue(value)
// TODO: Add a way to remove an element
switch {
case options.Index < 0: // prefix
tv = append([]string{v}, tv...)
case options.Index >= len(tv): // suffix
tv = append(tv, v)
default: // replace an element
tv[options.Index] = v
if options.FullText != "" {
value.YNode().Value = getByRegex(options.FullText, targetField.YNode().Value, v, options.Index)
} else if options.Delimiter != "" && options.EndDelimiter != "" {
regex := regexp.QuoteMeta(options.Delimiter) + `(.*?)` + regexp.QuoteMeta(options.EndDelimiter)
source := options.Delimiter + v + options.EndDelimiter
value.YNode().Value = getByRegex(regex, targetField.YNode().Value, source, options.Index)
} else {
value.YNode().Value = getByDelimiter(options.Delimiter, targetField.YNode().Value, v, options.Index)
}
value.YNode().Value = strings.Join(tv, options.Delimiter)
}

if targetField.YNode().Kind == yaml.ScalarNode {
Expand All @@ -247,3 +267,32 @@ func setFieldValue(options *types.FieldOptions, targetField *yaml.RNode, value *

return nil
}

func getByDelimiter(delimiter string, target string, source string, index int) string {
tv := strings.Split(target, delimiter)
// TODO: Add a way to remove an element
switch {
case index < 0: // prefix
tv = append([]string{source}, tv...)
case index >= len(tv): // suffix
tv = append(tv, source)
default: // replace an element
tv[index] = source
}
return strings.Join(tv, delimiter)
}

func getByRegex(regex string, target string, source string, index int) string {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

@Yingrjimsch Yingrjimsch Jun 21, 2024 •
edited

Choose a reason for hiding this comment

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

@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

re := regexp.MustCompile(regex)
counter := 0
res := re.ReplaceAllStringFunc(target, func(str string) string {
if counter != index && index >= 0 {
return str
}

counter++
return re.ReplaceAllString(str, source)
})
return res
}