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

Keep empty map in kustomize output #2805

Merged
merged 1 commit into from
Aug 7, 2020
Merged

Keep empty map in kustomize output #2805

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 20 additions & 10 deletions api/krusty/extendedpatch_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,8 @@ spec:
- mountPath: /tmp/ps
name: busybox-persistent-storage
volumes:
- name: busybox-persistent-storage
- emptyDir: {}
name: busybox-persistent-storage
- configMap:
name: configmap-in-base
name: configmap-in-base
Expand Down Expand Up @@ -232,7 +233,8 @@ spec:
- mountPath: /tmp/ps
name: nginx-persistent-storage
volumes:
- name: nginx-persistent-storage
- emptyDir: {}
name: nginx-persistent-storage
- configMap:
name: configmap-in-base
name: configmap-in-base
Expand All @@ -258,7 +260,8 @@ spec:
- mountPath: /tmp/ps
name: busybox-persistent-storage
volumes:
- name: busybox-persistent-storage
- emptyDir: {}
name: busybox-persistent-storage
- configMap:
name: configmap-in-base
name: configmap-in-base
Expand Down Expand Up @@ -332,7 +335,8 @@ spec:
- mountPath: /tmp/ps
name: nginx-persistent-storage
volumes:
- name: nginx-persistent-storage
- emptyDir: {}
name: nginx-persistent-storage
- configMap:
name: configmap-in-base
name: configmap-in-base
Expand Down Expand Up @@ -459,7 +463,8 @@ spec:
- mountPath: /tmp/ps
name: busybox-persistent-storage
volumes:
- name: busybox-persistent-storage
- emptyDir: {}
name: busybox-persistent-storage
- configMap:
name: configmap-in-base
name: configmap-in-base
Expand Down Expand Up @@ -559,7 +564,8 @@ spec:
- mountPath: /tmp/ps
name: busybox-persistent-storage
volumes:
- name: busybox-persistent-storage
- emptyDir: {}
name: busybox-persistent-storage
- configMap:
name: configmap-in-base
name: configmap-in-base
Expand Down Expand Up @@ -661,7 +667,8 @@ spec:
- mountPath: /tmp/ps
name: busybox-persistent-storage
volumes:
- name: busybox-persistent-storage
- emptyDir: {}
name: busybox-persistent-storage
- configMap:
name: configmap-in-base
name: configmap-in-base
Expand Down Expand Up @@ -762,7 +769,8 @@ spec:
- mountPath: /tmp/ps
name: busybox-persistent-storage
volumes:
- name: busybox-persistent-storage
- emptyDir: {}
name: busybox-persistent-storage
- configMap:
name: configmap-in-base
name: configmap-in-base
Expand Down Expand Up @@ -957,7 +965,8 @@ spec:
- mountPath: /tmp/ps
name: busybox-persistent-storage
volumes:
- name: busybox-persistent-storage
- emptyDir: {}
name: busybox-persistent-storage
- configMap:
name: configmap-in-base
name: configmap-in-base
Expand Down Expand Up @@ -1171,7 +1180,8 @@ spec:
- mountPath: /tmp/ps
name: busybox-persistent-storage
volumes:
- name: busybox-persistent-storage
- emptyDir: {}
name: busybox-persistent-storage
- configMap:
name: configmap-in-base
name: configmap-in-base
Expand Down
6 changes: 4 additions & 2 deletions api/krusty/inlinepatch_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,8 @@ spec:
- mountPath: /tmp/ps
name: nginx-persistent-storage
volumes:
- name: nginx-persistent-storage
- emptyDir: {}
name: nginx-persistent-storage
- configMap:
name: configmap-in-base
name: configmap-in-base
Expand Down Expand Up @@ -222,7 +223,8 @@ spec:
- mountPath: /tmp/ps
name: nginx-persistent-storage
volumes:
- name: nginx-persistent-storage
- emptyDir: {}
name: nginx-persistent-storage
- configMap:
name: configmap-in-base
name: configmap-in-base
Expand Down
3 changes: 2 additions & 1 deletion api/krusty/multiplepatch_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -540,7 +540,8 @@ spec:
- mountPath: /tmp/ps
name: nginx-persistent-storage
volumes:
- name: nginx-persistent-storage
- emptyDir: {}
name: nginx-persistent-storage
- configMap:
name: staging-team-foo-configmap-in-base-798k5k7g9f
name: configmap-in-base
Expand Down
5 changes: 3 additions & 2 deletions kyaml/yaml/fns.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,14 +67,15 @@ func (e ElementSetter) Filter(rn *RNode) (*RNode, error) {
matchingElementFound := false
for i := range rn.YNode().Content {
elem := rn.Content()[i]
newNode := NewRNode(elem)

// empty elements are not valid -- they at least need an associative key
if IsEmpty(NewRNode(elem)) {
if IsEmpty(newNode) || IsEmptyMap(newNode) {
continue
}

// check if this is the element we are matching
val, err := NewRNode(elem).Pipe(FieldMatcher{Name: e.Key, StringValue: e.Value})
val, err := newNode.Pipe(FieldMatcher{Name: e.Key, StringValue: e.Value})
if err != nil {
return nil, err
}
Expand Down
2 changes: 1 addition & 1 deletion kyaml/yaml/merge2/element_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ kind: Deployment
spec:
template:
spec:
containers: {}
containers: []
`,
dest: `
apiVersion: apps/v1
Expand Down
4 changes: 2 additions & 2 deletions kyaml/yaml/merge2/list_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ kind: Deployment
{description: `remove list -- empty in src`,
source: `
kind: Deployment
items: {}
items: []
`,
dest: `
kind: Deployment
Expand All @@ -273,7 +273,7 @@ items:
`,
expected: `
kind: Deployment
items: {}
items: []
`,
},
}
2 changes: 1 addition & 1 deletion kyaml/yaml/merge2/map_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,8 @@ spec: {}
expected: `
kind: Deployment
spec:
foo: bar1
baz: buz
foo: bar1
`,
},

Expand Down
3 changes: 1 addition & 2 deletions kyaml/yaml/merge2/scalar_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,15 +91,14 @@ kind: Deployment
{description: `remove scalar -- empty in src`,
source: `
kind: Deployment
field: {}
Copy link
Contributor

Choose a reason for hiding this comment

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

So this PR also retains scalar nulls?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This null is in input. Because {} is not considered as empty so only null can be used as empty in input.

field: null
`,
dest: `
kind: Deployment
field: value1
`,
expected: `
kind: Deployment
field: {}
`,
},

Expand Down
6 changes: 3 additions & 3 deletions kyaml/yaml/merge3/element_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,15 +371,15 @@ kind: Deployment
spec:
template:
spec:
containers: {}
containers: null
`,
update: `
apiVersion: apps/v1
kind: Deployment
spec:
template:
spec:
containers: {}
containers: null
`,
local: `
apiVersion: apps/v1
Expand Down Expand Up @@ -584,7 +584,7 @@ kind: Deployment
spec:
template:
spec:
containers: {}
containers: null
`,
local: `
apiVersion: apps/v1
Expand Down
13 changes: 7 additions & 6 deletions kyaml/yaml/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,16 +44,17 @@ func IsMissingOrNull(node *RNode) bool {
return node == nil || node.YNode() == nil || node.YNode().Tag == NullNodeTag
}

// IsEmpty returns true if the RNode is MissingOrNull, or is either a MappingNode with
// no fields.
// IsEmpty returns true if the RNode is MissingOrNull
func IsEmpty(node *RNode) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

It now seems we have:

IsMissingOrNull(n) => n == nil || n.YNode() == nil || n.YNode().Tag == NullNodeTag
        IsEmpty(n) => IsMissingOrNull(n)
         IsNull(n) => n != nil && n.YNode() != nil && n.YNode().Tag == NullNodeTag
     IsEmptyMap(n) => IsEmpty(n) ||
                          (n.YNode().Kind == yaml.MappingNode && len(n.YNode().Content) == 0)

This feels random. And we have this strange contest between
Kind and Tag, which i'm not sure how to resolve yet.

We've not hit 1.0 yet, so we can change the API.

How about we try:

  IsNil(n)        => n == nil || n.YNode() == nil
  IsEmptyDoc(n)   => !IsNodeNil(n) && n.YNode().Tag == NodeTagNull
  IsEmptyMap(n)   => !IsNodeNil(n) &&
                        n.YNode().Kind == yaml.MappingNode &&
                        len(n.YNode().Content) == 0

with this, the tags are hidden, and we start a pattern where
IsEmptyFoo means the object is non-nil with a defined type -
so empty docs, lists and maps are different things.

Translation:

  • IsNull becomes IsEmptyDoc
  • IsEmptyand IsMissingOrNull are written as IsNil || IsEmptyDoc

The compiler can optimize away redundancy.

So L54 in kyaml/yaml/walk/associative_sequence.go which currently reads

if yaml.IsEmpty(val) || yaml.IsEmptyMap(val) {

becomes

if yaml.IsNil(val) || yaml.IsEmptyDoc(val) || yaml.IsEmptyMap(val) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds good. But because these methods are fundamental and widely used so if we change them, we need to update a lot of files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about create another issue about this refactoring?

if IsMissingOrNull(node) {
return IsMissingOrNull(node)
}

// IsEmptyMap returns true if the RNode is an empty node or an empty map
func IsEmptyMap(node *RNode) bool {
if IsEmpty(node) {
return true
}

// Empty sequence is a special case and temporarily not considered as empty here.
// Some users may want to keep empty sequence for compatibility reason.
// For example, use JSON 6902 patch.
return node.YNode().Kind == yaml.MappingNode && len(node.YNode().Content) == 0
}

Expand Down
6 changes: 3 additions & 3 deletions kyaml/yaml/types_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,19 +221,19 @@ func TestIsEmpty_Arrays(t *testing.T) {
func TestIsEmpty_Maps(t *testing.T) {
node := NewMapRNode(nil)
// empty map
if !IsEmpty(node) {
if !IsEmptyMap(node) {
t.Fatalf("input: empty map")
}
// map with 1 item
node = NewMapRNode(&map[string]string{
"foo": "bar",
})
if IsEmpty(node) {
if IsEmptyMap(node) {
t.Fatalf("input: map with 1 item")
}
// delete the item in map
node.value.Content = nil
if !IsEmpty(node) {
if !IsEmptyMap(node) {
t.Fatalf("input: empty map")
}
}
2 changes: 1 addition & 1 deletion kyaml/yaml/walk/associative_sequence.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func (l *Walker) walkAssociativeSequence() (*yaml.RNode, error) {
if err != nil {
return nil, err
}
if yaml.IsEmpty(val) {
if yaml.IsEmpty(val) || yaml.IsEmptyMap(val) {
_, err = dest.Pipe(yaml.ElementSetter{Key: key, Value: value})
if err != nil {
return nil, err
Expand Down
42 changes: 1 addition & 41 deletions plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -467,7 +467,6 @@ spec:
replicas: 1
template:
metadata:
creationTimestamp: null
labels:
workload.sas.com/class: stateless
spec:
Expand Down Expand Up @@ -498,45 +497,6 @@ spec:
name: tmp
`

// This is the current (incorrect) result we get with kustomize 3.8.1
const currentCleanedDeployment = `apiVersion: apps/v1
kind: Deployment
metadata:
labels:
workload.sas.com/class: stateless
name: sas-crunchy-data-postgres-operator
spec:
replicas: 1
template:
metadata:
labels:
workload.sas.com/class: stateless
spec:
containers:
- envFrom: []
image: sas-crunchy-data-operator-api-server
imagePullPolicy: IfNotPresent
name: apiserver
ports:
- containerPort: 8443
volumeMounts:
- mountPath: /security-ssh
name: security-ssh
- mountPath: /tmp
name: tmp
imagePullSecrets: []
initContainers: []
serviceAccountName: postgres-operator
tolerations:
- effect: NoSchedule
key: workload.sas.com/class
operator: Equal
value: stateful
volumes:
- name: security-ssh
- name: tmp
`

func TestPatchStrategicMergeTransformerCleanupItems(t *testing.T) {
th := kusttest_test.MakeEnhancedHarness(t).
PrepBuiltin("PatchStrategicMergeTransformer")
Expand Down Expand Up @@ -571,7 +531,7 @@ paths:
- patch.yaml
`,
anUncleanDeploymentResource,
currentCleanedDeployment) // prefer expectedCleanedDeployment
expectedCleanedDeployment) // prefer expectedCleanedDeployment
}

func TestStrategicMergeTransformerNoSchema(t *testing.T) {
Expand Down
5 changes: 4 additions & 1 deletion plugin/builtin/patchstrategicmergetransformer/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,7 @@ require (
sigs.k8s.io/yaml v1.2.0
)

replace sigs.k8s.io/kustomize/api v0.5.1 => ../../../api
replace (
sigs.k8s.io/kustomize/api v0.5.1 => ../../../api
sigs.k8s.io/kustomize/kyaml v0.4.2 => ../../../kyaml
)