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.

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

Option to customize NamespaceTransformer overwrite behaviour #4708

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion .golangci.yml
Expand Up @@ -37,7 +37,7 @@ linters:
- gocyclo
# - godot
# - godox
- goerr113
# - goerr113
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm glad we're disabling this, it's been slightly annoying to work with

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feel free to question the set of linters I enabled when they really don't align with the style in this codebase and/or value is low. I erred on the side of enabling more of them, without necessarily having a clear picture of the implications.

- gofmt
# - gofumpt
- goheader
Expand Down
2 changes: 1 addition & 1 deletion Makefile-tools.mk
Expand Up @@ -28,7 +28,7 @@ uninstall-out-of-tree-tools:
rm -f $(MYGOBIN)/stringer

$(MYGOBIN)/golangci-lint:
go install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.45.2
go install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.46.2

$(MYGOBIN)/mdrip:
go install github.com/monopole/mdrip@v1.0.2
Expand Down
68 changes: 52 additions & 16 deletions api/filters/filtersutil/setters.go
Expand Up @@ -12,58 +12,94 @@ type SetFn func(*yaml.RNode) error

// SetScalar returns a SetFn to set a scalar value
func SetScalar(value string) SetFn {
return func(node *yaml.RNode) error {
return node.PipeE(yaml.FieldSetter{StringValue: value})
}
return SetEntry("", value, yaml.NodeTagEmpty)
}

// SetEntry returns a SetFn to set an entry in a map
func SetEntry(key, value, tag string) SetFn {
// SetEntry returns a SetFn to set a field or a map entry to a value.
// It can be used with an empty name to set both a value and a tag on a scalar node.
// When setting only a value on a scalar node, use SetScalar instead.
func SetEntry(name, value, tag string) SetFn {
n := &yaml.Node{
Kind: yaml.ScalarNode,
Value: value,
Tag: tag,
}
return func(node *yaml.RNode) error {
return node.PipeE(yaml.FieldSetter{
Name: key,
Name: name,
Value: yaml.NewRNode(n),
})
}
}

type TrackableSetter struct {
// SetValueCallback will be invoked each time a field is set
setValueCallback func(key, value, tag string, node *yaml.RNode)
setValueCallback func(name, value, tag string, node *yaml.RNode)
}

// WithMutationTracker registers a callback which will be invoked each time a field is mutated
func (s *TrackableSetter) WithMutationTracker(callback func(key, value, tag string, node *yaml.RNode)) {
func (s *TrackableSetter) WithMutationTracker(callback func(key, value, tag string, node *yaml.RNode)) *TrackableSetter {
s.setValueCallback = callback
return s
}

// SetScalar returns a SetFn to set a scalar value
// SetScalar returns a SetFn to set a scalar value.
// if a mutation tracker has been registered, the tracker will be invoked each
// time a scalar is set
func (s TrackableSetter) SetScalar(value string) SetFn {
origSetScalar := SetScalar(value)
return s.SetEntry("", value, yaml.NodeTagEmpty)
}

// SetScalarIfEmpty returns a SetFn to set a scalar value only if it isn't already set.
// If a mutation tracker has been registered, the tracker will be invoked each
// time a scalar is actually set.
func (s TrackableSetter) SetScalarIfEmpty(value string) SetFn {
return s.SetEntryIfEmpty("", value, yaml.NodeTagEmpty)
}

// SetEntry returns a SetFn to set a field or a map entry to a value.
// It can be used with an empty name to set both a value and a tag on a scalar node.
// When setting only a value on a scalar node, use SetScalar instead.
// If a mutation tracker has been registered, the tracker will be invoked each
// time an entry is set.
func (s TrackableSetter) SetEntry(name, value, tag string) SetFn {
origSetEntry := SetEntry(name, value, tag)
return func(node *yaml.RNode) error {
if s.setValueCallback != nil {
s.setValueCallback("", value, "", node)
s.setValueCallback(name, value, tag, node)
}
return origSetScalar(node)
return origSetEntry(node)
}
}

// SetEntry returns a SetFn to set an entry in a map
// if a mutation tracker has been registered, the tracker will be invoked each
// time an entry is set
func (s TrackableSetter) SetEntry(key, value, tag string) SetFn {
// SetEntryIfEmpty returns a SetFn to set a field or a map entry to a value only if it isn't already set.
// It can be used with an empty name to set both a value and a tag on a scalar node.
// When setting only a value on a scalar node, use SetScalar instead.
// If a mutation tracker has been registered, the tracker will be invoked each
// time an entry is actually set.
func (s TrackableSetter) SetEntryIfEmpty(key, value, tag string) SetFn {
origSetEntry := SetEntry(key, value, tag)
return func(node *yaml.RNode) error {
if hasExistingValue(node, key) {
return nil
}
if s.setValueCallback != nil {
s.setValueCallback(key, value, tag, node)
}
return origSetEntry(node)
}
}

func hasExistingValue(node *yaml.RNode, key string) bool {
if node.IsNilOrEmpty() {
return false
}
if err := yaml.ErrorIfInvalid(node, yaml.ScalarNode); err == nil {
return yaml.GetValue(node) != ""
}
entry := node.Field(key)
if entry.IsNilOrEmpty() {
return false
}
return yaml.GetValue(entry.Value) != ""
}
106 changes: 106 additions & 0 deletions api/filters/filtersutil/setters_test.go
@@ -0,0 +1,106 @@
// Copyright 2022 The Kubernetes Authors.
// SPDX-License-Identifier: Apache-2.0

package filtersutil_test

import (
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"sigs.k8s.io/kustomize/api/filters/filtersutil"
"sigs.k8s.io/kustomize/kyaml/yaml"
)

func TestTrackableSetter_SetScalarIfEmpty(t *testing.T) {
tests := []struct {
name string
input *yaml.RNode
value string
want string
}{
{
name: "sets null values",
input: yaml.MakeNullNode(),
value: "foo",
want: "foo",
},
{
name: "sets empty values",
input: yaml.NewScalarRNode(""),
value: "foo",
want: "foo",
},
{
name: "does not overwrite values",
input: yaml.NewStringRNode("a"),
value: "foo",
want: "a",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
wasSet := false
s := (&filtersutil.TrackableSetter{}).WithMutationTracker(func(_, _, _ string, _ *yaml.RNode) {
wasSet = true
})
wantSet := tt.value == tt.want
fn := s.SetScalarIfEmpty(tt.value)
require.NoError(t, fn(tt.input))
assert.Equal(t, tt.want, yaml.GetValue(tt.input))
assert.Equal(t, wantSet, wasSet, "tracker invoked even though value was not changed")
})
}
}

func TestTrackableSetter_SetEntryIfEmpty(t *testing.T) {
tests := []struct {
name string
input *yaml.RNode
key string
value string
want string
}{
{
name: "sets empty values",
input: yaml.NewMapRNode(&map[string]string{"setMe": ""}),
key: "setMe",
value: "foo",
want: "foo",
},
{
name: "sets missing keys",
input: yaml.NewMapRNode(&map[string]string{}),
key: "setMe",
value: "foo",
want: "foo",
},
{
name: "does not overwrite values",
input: yaml.NewMapRNode(&map[string]string{"existing": "original"}),
key: "existing",
value: "foo",
want: "original",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
wasSet := false
s := (&filtersutil.TrackableSetter{}).WithMutationTracker(func(_, _, _ string, _ *yaml.RNode) {
wasSet = true
})
wantSet := tt.value == tt.want
fn := s.SetEntryIfEmpty(tt.key, tt.value, "")
require.NoError(t, fn(tt.input))
assert.Equal(t, tt.want, yaml.GetValue(tt.input.Field(tt.key).Value))
assert.Equal(t, wantSet, wasSet, "tracker invoked even though value was not changed")
})
}
}

func TestTrackableSetter_SetEntryIfEmpty_BadInputNodeKind(t *testing.T) {
fn := filtersutil.TrackableSetter{}.SetEntryIfEmpty("foo", "false", yaml.NodeTagBool)
rn := yaml.NewListRNode("nope")
rn.AppendToFieldPath("dummy", "path")
assert.EqualError(t, fn(rn), "wrong Node Kind for dummy.path expected: MappingNode was SequenceNode: value: {- nope}")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is outside the scope of this PR, but I hope we can improve this error message some day

}
15 changes: 6 additions & 9 deletions api/filters/nameref/nameref.go
Expand Up @@ -337,19 +337,16 @@ func (f Filter) selectReferral(
return candidates[0], nil
}
ids := getIds(candidates)
f.failureDetails(candidates)
return nil, fmt.Errorf(" found multiple possible referrals: %s", ids)
return nil, fmt.Errorf("found multiple possible referrals: %s\n%s", ids, f.failureDetails(candidates))
}

func (f Filter) failureDetails(resources []*resource.Resource) {
fmt.Printf(
"\n**** Too many possible referral targets to referrer:\n%s\n",
f.Referrer.MustYaml())
func (f Filter) failureDetails(resources []*resource.Resource) string {
msg := strings.Builder{}
msg.WriteString(fmt.Sprintf("\n**** Too many possible referral targets to referrer:\n%s\n", f.Referrer.MustYaml()))
for i, r := range resources {
fmt.Printf(
"--- possible referral %d:\n%s", i, r.MustYaml())
fmt.Println("------")
msg.WriteString(fmt.Sprintf("--- possible referral %d:\n%s\n", i, r.MustYaml()))
}
return msg.String()
}

func allNamesAreTheSame(resources []*resource.Resource) bool {
Expand Down