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

fix error message using replacement wildcard and create option #4577

Merged
merged 6 commits into from Apr 13, 2022
Merged

fix error message using replacement wildcard and create option #4577

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5e84de2
fix errror message using replacement wildcard and create option
koba1t Apr 11, 2022
cdc4a50
fix lint error
koba1t Apr 12, 2022
01ab069
fix error message
koba1t Apr 12, 2022
02e0b38
fix lint error
koba1t Apr 12, 2022
ed72bb0
fix lint error
koba1t Apr 12, 2022
2f2e14e
fix lint error to nolint:goerr113
koba1t Apr 13, 2022
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
3 changes: 0 additions & 3 deletions api/filters/replacement/replacement.go
View file
Open in desktop
Expand Up @@ -142,9 +142,6 @@ func applyToOneNode(options *types.FieldOptions, t *yaml.RNode, value *yaml.RNod
}

for _, scalarNode := range t.YNode().Content {
if options != nil && options.Create {
return fmt.Errorf("cannot use create option in a multi-value target")
}
rn := yaml.NewRNode(scalarNode)
if err := setTargetValue(options, rn, value); err != nil {
return err
Expand Down
39 changes: 39 additions & 0 deletions api/filters/replacement/replacement_test.go
View file
Open in desktop
Expand Up @@ -1634,6 +1634,45 @@ spec:
- name: deployment-name
value: sample-deploy`,
},
"index contains '*' character and create options": {
input: `apiVersion: apps/v1
kind: Deployment
metadata:
labels:
app: sample-deploy
name: sample-deploy
spec:
replicas: 1
selector:
matchLabels:
app: sample-deploy
template:
metadata:
labels:
app: sample-deploy
spec:
containers:
- image: nginx
name: main
env:
- name: other-env
value: YYYYY
`,
replacements: `replacements:
- source:
kind: Deployment
name: sample-deploy
fieldPath: metadata.name
targets:
- select:
kind: Deployment
fieldPaths:
- spec.template.spec.containers.*.env.[name=deployment-name].value
options:
create: true
`,
expectedErr: "cannot support create option in a multi-value target now",
},
"multiple field paths in target": {
Copy link
Contributor

Choose a reason for hiding this comment

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

could you please also add the test from https://github.com/kubernetes-sigs/kustomize/pull/4574/files that I added to replacement_test.go, called "create Job spec"?

I pulled your branch and it looks like something funny is going on when running with that test, it would be great if you could help fix that.

Copy link
Contributor

Choose a reason for hiding this comment

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

For that case, I think we can do something in the code that's here:

kustomize/api/filters/replacement/replacement.go

Lines 119 to 123 in e5041ba

if target.Options != nil && target.Options.Create {
t, err = node.Pipe(yaml.LookupCreate(value.YNode().Kind, fieldPath...))
} else {
t, err = node.Pipe(&yaml.PathMatcher{Path: fieldPath})
}

We can potentially try something like this:

		if target.Options != nil && target.Options.Create && !fieldPathAlreadyExistsInTarget {
			t, err = node.Pipe(yaml.LookupCreate(value.YNode().Kind, fieldPath...))
		} else {
			t, err = node.Pipe(&yaml.PathMatcher{Path: fieldPath})
		}

to see if it fixes the problem.

input: `apiVersion: v1
kind: ConfigMap
Expand Down
3 changes: 3 additions & 0 deletions kyaml/yaml/fns.go
View file
Open in desktop
Expand Up @@ -541,6 +541,9 @@ func (l PathGetter) getFilter(part, nextPart string, fieldPath *[]string) (Filte
case part == "-":
// part is a hyphen
return GetElementByIndex(-1), nil
case part == "*":
// part is a asterisk
return nil, errors.Errorf("cannot support create option in a multi-value target now")
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't it be possible for someone to be attempting to use PathGetter with an asterisk but without the create option? I think this error message needs to be about wildcard support rather than create mode specifically because of that (and replacements may need to augment the error to clarify what the problem is in that specific flow).

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

The other option is to move the check into the replacement filter, in replacements.go you can throw this error if create is true and the path contains *.

Also suggest removing the word "now"

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I fix it to 01ab069

case IsListIndex(part):
// part is surrounded by brackets
return l.elemFilter(part)
Expand Down
11 changes: 9 additions & 2 deletions kyaml/yaml/fns_test.go
View file
Open in desktop
Expand Up @@ -142,7 +142,7 @@ func TestElementSetter(t *testing.T) {

node = MustParse(`
- a: b
- c: d
- c: d
`)
// If given a key and no values, ElementSetter will
// change node to be an empty list
Expand All @@ -154,7 +154,7 @@ func TestElementSetter(t *testing.T) {

node = MustParse(`
- a: b
- c: d
- c: d
`)
// Return error because ElementSetter will assume all elements are scalar when
// there is only value provided.
Expand Down Expand Up @@ -580,6 +580,13 @@ a: {}
assert.Equal(t, "h\n", assertNoErrorString(t)(rn.String()))
}

func TestLookup_Fn_create_with_wildcard_error(t *testing.T) {
node, err := Parse(s)
assert.NoError(t, err)
_, err = node.Pipe(LookupCreate(yaml.MappingNode, "a", "b", "*", "t"))
assert.Error(t, err, "cannot support create option in a multi-value target now")
}

func TestLookup(t *testing.T) {
s := `n: o
a:
Expand Down