appendCsVAnnotation must mutate the annotations RNode in place

Otherwise, it is incompatible with filters like fieldSpec.Filter that recurse through a tree of RNodes. In the case of the bug this commit fixes, the leaf RNode is a metadata.annotation value field, and appendCsvAnnotation is called immediately before updating that leaf's value. If appendCsvAnnotation replaces the entire metadata.annotation RNode on the resource, the leaf RNode that gets updated is no longer attached to the resource.

Alternatively, `func (f Filter) setScalar` could be updated to change
the value of the leaf RNode BEFORE calling appendCsvAnnotation. However,
the modification of the entire metadata segment of the RNode in a
function that nominally just edits an annotation feels like a
side-effect prone to creating other difficult-to-debug situations,
beyond this ordering dependency.
This commit is contained in:
Katrina Verey
2022-07-08 22:20:47 -04:00
parent 77814ac12b
commit 04e1663b70
3 changed files with 13 additions and 18 deletions

View File

@@ -649,7 +649,6 @@ metadata:
name: oldNs
`)
m := th.Run(".", th.MakeDefaultOptions())
// Incorrect: the self-reference annotations in both resources were not updated!
th.AssertActualEqualsExpected(m, `
apiVersion: v1
data:
@@ -658,7 +657,7 @@ data:
kind: ConfigMap
metadata:
annotations:
theConfigMap: cm
theConfigMap: cm-updated
theNamespace: newNs
name: cm-updated
namespace: newNs
@@ -668,7 +667,7 @@ kind: Namespace
metadata:
annotations:
theConfigMap: cm-updated
theNamespace: oldNs
theNamespace: newNs
name: newNs
`)
}

View File

@@ -255,13 +255,9 @@ func (r *Resource) appendCsvAnnotation(name, value string) {
if value == "" {
return
}
annotations := r.GetAnnotations()
if existing, ok := annotations[name]; ok {
annotations[name] = existing + "," + value
} else {
annotations[name] = value
}
if err := r.SetAnnotations(annotations); err != nil {
currentValue := r.getCsvAnnotation(name)
newValue := strings.Join(append(currentValue, value), ",")
if err := r.RNode.PipeE(kyaml.SetAnnotation(name, newValue)); err != nil {
panic(err)
}
}

View File

@@ -1402,31 +1402,31 @@ spec:
`))
assert.NoError(t, err)
r.AppendRefBy(resid.FromString("knd1.ver1.gr1/name1.ns1"))
assert.Equal(t, r.RNode.MustString(), `apiVersion: v1
assert.Equal(t, `apiVersion: v1
kind: Deployment
metadata:
name: clown
annotations:
internal.config.kubernetes.io/refBy: knd1.ver1.gr1/name1.ns1
internal.config.kubernetes.io/refBy: 'knd1.ver1.gr1/name1.ns1'
spec:
numReplicas: 1
`)
`, r.RNode.MustString())
assert.Equal(t, r.GetRefBy(), []resid.ResId{resid.FromString("knd1.ver1.gr1/name1.ns1")})
r.AppendRefBy(resid.FromString("knd2.ver2.gr2/name2.ns2"))
assert.Equal(t, r.RNode.MustString(), `apiVersion: v1
assert.Equal(t, `apiVersion: v1
kind: Deployment
metadata:
name: clown
annotations:
internal.config.kubernetes.io/refBy: knd1.ver1.gr1/name1.ns1,knd2.ver2.gr2/name2.ns2
internal.config.kubernetes.io/refBy: 'knd1.ver1.gr1/name1.ns1,knd2.ver2.gr2/name2.ns2'
spec:
numReplicas: 1
`)
assert.Equal(t, r.GetRefBy(), []resid.ResId{
`, r.RNode.MustString())
assert.Equal(t, []resid.ResId{
resid.FromString("knd1.ver1.gr1/name1.ns1"),
resid.FromString("knd2.ver2.gr2/name2.ns2"),
})
}, r.GetRefBy())
}
func TestOrigin(t *testing.T) {