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: patch additions honor source key style #5196

Merged
merged 4 commits into from
Sep 1, 2023
Merged

fix: patch additions honor source key style #5196

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
691b7d1
fix: patch additions honor source key style
ephesused Jun 8, 2023
c76fd5e
test: update psm key style test
ephesused Jun 13, 2023
096b2c4
test: add psm test for different key types
ephesused Jun 15, 2023
78b8139
Merge branch 'master' into issue4928-append-honors-key-style
ephesused Aug 30, 2023
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
97 changes: 97 additions & 0 deletions api/filters/patchstrategicmerge/patchstrategicmerge_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -776,6 +776,103 @@ spec:
autoscaling: true
deepgram-api:
some: value
`,
},

// Issue #4928
"support numeric keys": {
input: `
apiVersion: v1
kind: ConfigMap
metadata:
name: blabla
namespace: blabla-ns
data:
"6443": "foobar"
Copy link
Member

Choose a reason for hiding this comment

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

Could you consider adding a test case when the key-value has "6443" and 6443?

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'm happy to extend or add other tests, but I'm not quite following the scenario you'd like me to test. Are you asking that this test to change its current input "6443" entry to:

  "6443": 6443

And leave the patch entry like so?

  "6443": "barfoo"

That would mean this test would be exercising different value types.

Copy link
Member

Choose a reason for hiding this comment

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

I'm sorry for writing a misreadable comment.
I just want to add to tests that mixed the same name string key and int key.

Ex.

  • base configMap has "6443": "foobar" and a patch has 6443: "barfoo_int"

Copy link
Contributor Author

@ephesused ephesused Jun 13, 2023 •
edited

Choose a reason for hiding this comment

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

Done - note that this scenario mirrors one that I discussed with @KnVerey in my earlier attempt.

A key point to make is that I'm intending this fix to be targeted at the specific issue in #4928: the key style of a node that's newly created in a merge.

This PR is not intended to alter the existing key style behavior for patches (which, I admit, is somewhat confusing). In that context, the adjusted test case we are discussing falls outside of the scope of what this PR changes. The current behavior for kustomize when input and patch have a matching key is to retain the key style of the input. This PR does not change that behavior.

Edited to fix a bad link.

Copy link
Member

Choose a reason for hiding this comment

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

In that context, the adjusted test case we are discussing falls outside of the scope of what this PR changes.

I understood this sentence. I want to keep until behavior with test cases.

Copy link
Member

Choose a reason for hiding this comment

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

So I want to say that we need to add test cases while keeping at the tests from 691b7d1.
Could you add test cases using the int key under your tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies - I believe I've got it right this time. Thanks for your patience.

`,
patch: yaml.MustParse(`
apiVersion: v1
kind: ConfigMap
metadata:
name: blabla
namespace: blabla-ns
data:
"6443": "barfoo"
"9110": "foo-foo"
`),
expected: `
apiVersion: v1
kind: ConfigMap
metadata:
name: blabla
namespace: blabla-ns
data:
"6443": "barfoo"
"9110": "foo-foo"
`,
},

"honor different key style one": {
input: `
apiVersion: v1
kind: ConfigMap
metadata:
name: blabla
namespace: blabla-ns
data:
'6443': "foobar"
`,
patch: yaml.MustParse(`
apiVersion: v1
kind: ConfigMap
metadata:
name: blabla
namespace: blabla-ns
data:
"6443": "barfoo"
9110: "foo-foo"
`),
expected: `
apiVersion: v1
kind: ConfigMap
metadata:
name: blabla
namespace: blabla-ns
data:
'6443': "barfoo"
9110: "foo-foo"
`,
},

"honor different key style two": {
input: `
apiVersion: v1
kind: ConfigMap
metadata:
name: blabla
namespace: blabla-ns
data:
"6443": "foobar"
`,
patch: yaml.MustParse(`
apiVersion: v1
kind: ConfigMap
metadata:
name: blabla
namespace: blabla-ns
data:
"6443": "barfoo"
'9110': "foo-foo"
`),
expected: `
apiVersion: v1
kind: ConfigMap
metadata:
name: blabla
namespace: blabla-ns
data:
"6443": "barfoo"
'9110': "foo-foo"
`,
},
}
Expand Down
5 changes: 5 additions & 0 deletions kyaml/yaml/fns.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -690,6 +690,10 @@ type FieldSetter struct {
// when setting it. Otherwise, if an existing node is found, the style is
// retained.
OverrideStyle bool `yaml:"overrideStyle,omitempty"`

// AppendKeyStyle defines the style of the key when no existing node is
// found, and a new node is appended.
AppendKeyStyle Style `yaml:"appendKeyStyle,omitempty"`
}

func (s FieldSetter) Filter(rn *RNode) (*RNode, error) {
Expand Down Expand Up @@ -747,6 +751,7 @@ func (s FieldSetter) Filter(rn *RNode) (*RNode, error) {
&yaml.Node{
Kind: yaml.ScalarNode,
Value: s.Name,
Style: s.AppendKeyStyle,
HeadComment: s.Comments.HeadComment,
LineComment: s.Comments.LineComment,
FootComment: s.Comments.FootComment,
Expand Down
41 changes: 41 additions & 0 deletions kyaml/yaml/merge2/map_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,47 @@ spec:
protocol: TCP
- port: 30901
targetPort: 30901
`,
mergeOptions: yaml.MergeOptions{
ListIncreaseDirection: yaml.MergeOptionsListAppend,
},
},

//
// Test Case
//
{description: `Verify key style behavior`,
source: `
kind: Deployment
spec:
source-and-dest: source-and-dest
source-only: source-only
"source-only-key-double-quoted": source-only
source-and-dest-key-style-diff-1: source-and-dest
'source-and-dest-key-style-diff-2': source-and-dest
"source-and-dest-key-style-diff-3": source-and-dest
`,
dest: `
kind: Deployment
spec:
source-and-dest: source-and-dest
'source-and-dest-key-style-diff-1': source-and-dest
"source-and-dest-key-style-diff-2": source-and-dest
source-and-dest-key-style-diff-3: source-and-dest
dest-only: dest-only
'dest-only-key-single-quoted': dest-only
`,
expected: `
kind: Deployment
spec:
source-and-dest: source-and-dest
'source-and-dest-key-style-diff-1': source-and-dest
"source-and-dest-key-style-diff-2": source-and-dest
source-and-dest-key-style-diff-3: source-and-dest
dest-only: dest-only
'dest-only-key-single-quoted': dest-only
source-only: source-only
"source-only-key-double-quoted": source-only
`,
mergeOptions: yaml.MergeOptions{
ListIncreaseDirection: yaml.MergeOptionsListAppend,
Expand Down
57 changes: 57 additions & 0 deletions kyaml/yaml/merge3/map_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,4 +306,61 @@ metadata:
name: foo
annotations: {}
`},

//
// Test Case
//
{
description: `Verify key style behavior`,
origin: `
apiVersion: v1
kind: ConfigMap
metadata:
name: foo
data:
unchanged: origin
unchanged-varying-key-style: origin
deleted-in-update: origin
deleted-in-local: origin
`,
update: `
apiVersion: v1
kind: ConfigMap
metadata:
name: foo
data:
unchanged: origin
'unchanged-varying-key-style': origin
deleted-in-local: origin
'added-in-update': 'update'
'added-in-update-and-local-same-value': 'update-and-local'
'added-in-update-and-local-diff-value': 'update'
`,
local: `
apiVersion: v1
kind: ConfigMap
metadata:
name: foo
data:
unchanged: origin
"unchanged-varying-key-style": origin
deleted-in-update: origin
"added-in-local": "local"
"added-in-update-and-local-same-value": "update-and-local"
"added-in-update-and-local-diff-value": "local"
`,
expected: `
apiVersion: v1
kind: ConfigMap
metadata:
name: foo
data:
unchanged: origin
"unchanged-varying-key-style": origin
"added-in-local": "local"
"added-in-update-and-local-same-value": "update-and-local"
"added-in-update-and-local-diff-value": "update"
'added-in-update': 'update'
`,
},
}
21 changes: 16 additions & 5 deletions kyaml/yaml/walk/map.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ func (l Walker) walkMap() (*yaml.RNode, error) {
if l.Schema != nil {
s = l.Schema.Field(key)
}
fv, commentSch := l.fieldValue(key)
fv, commentSch, keyStyles := l.fieldValue(key)
if commentSch != nil {
s = commentSch
}
Expand Down Expand Up @@ -90,7 +90,13 @@ func (l Walker) walkMap() (*yaml.RNode, error) {
}

// this handles empty and non-empty values
_, err = dest.Pipe(yaml.FieldSetter{Name: key, Comments: comments, Value: val})
fieldSetter := yaml.FieldSetter{
Name: key,
Comments: comments,
AppendKeyStyle: keyStyles[val],
Value: val,
}
_, err = dest.Pipe(fieldSetter)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -153,10 +159,12 @@ func (l Walker) fieldNames() []string {
return result
}

// fieldValue returns a slice containing each source's value for fieldName
func (l Walker) fieldValue(fieldName string) ([]*yaml.RNode, *openapi.ResourceSchema) {
// fieldValue returns a slice containing each source's value for fieldName, the
// schema, and a map of each source's value to the style for the source's key.
func (l Walker) fieldValue(fieldName string) ([]*yaml.RNode, *openapi.ResourceSchema, map[*yaml.RNode]yaml.Style) {
var fields []*yaml.RNode
var sch *openapi.ResourceSchema
keyStyles := make(map[*yaml.RNode]yaml.Style, len(l.Sources))
for i := range l.Sources {
if l.Sources[i] == nil {
fields = append(fields, nil)
Expand All @@ -165,9 +173,12 @@ func (l Walker) fieldValue(fieldName string) ([]*yaml.RNode, *openapi.ResourceSc
field := l.Sources[i].Field(fieldName)
f, s := l.valueIfPresent(field)
fields = append(fields, f)
if field != nil && field.Key != nil && field.Key.YNode() != nil {
keyStyles[f] = field.Key.YNode().Style
}
if sch == nil && !s.IsMissingOrNull() {
sch = s
}
}
return fields, sch
return fields, sch, keyStyles
}