Namespace created by Filter(...).Apply() can not be delete automatic

[vincent-pli]

I found Namespace created by Filter(...).Apply() can not be delete automatic.

Base on the implements, when delete cluster level resource, we will check if it has the Annotation: manifestival: new,
The resource is come from m.resources when Delete().

When adopt Filter(...).Apply(), the Filter() will make new instance deep copy from m.resources, that's means the Annotation mark on the new instance rather than m.resources.

Check here:

manifestival/filter.go

Line 27 in 3577f8e

for _, spec := range m.Resources() {

I have no idea if the deep-copy is on purpose or not, but I found we has UT test, so it must be on purpose, can anyone clarify it, thanks.

[jcrossley3]

Forgive me, but I'm having a little trouble understanding the problem. The deep-copy is definitely intentional, in order to maintain the manifest's immutable semantics. Would it be possible for you to create a failing unit test demonstrating the problem?

[vincent-pli]

@jcrossley3
I do have a UT test, should add to manifestival_test.go

func TestFilterNamespaceDeletion(t *testing.T) {
	ns := v1.Namespace{
		ObjectMeta: metav1.ObjectMeta{
			Name: "test",
		},
	}
	u := unstructured.Unstructured{}
	scheme.Scheme.Convert(&ns, &u, nil)
	// First verify existing namespaces are *NOT* deleted
	client := fake.New(&ns)
	manifest, _ := ManifestFrom(Slice([]unstructured.Unstructured{u}), UseClient(client))
	namespace := Any(ByKind("Namespace"))
	err := manifest.Filter(namespace).Apply()
	assert(t, err, nil)
	err = manifest.Delete()
	assert(t, err, nil)
	_, err = client.Get(&u)
	assert(t, err, nil)
	// Now verify that newly-created namespaces *ARE* deleted
	err = client.Delete(&u)
	assert(t, err, nil)
	err = manifest.Filter(namespace).Apply()
	assert(t, err, nil)
	err = manifest.Delete()
	assert(t, err, nil)
	_, err = client.Get(&u)
	assert(t, errors.IsNotFound(err), true)
}

[vincent-pli]

manifest's immutable semantics

but we add Annotation to manifest.resources when do Apply()

[jcrossley3]

Ok, I see what you're saying now. Chaining the Apply call to the Filter means you lose the slice of annotated resources in the manifest. Obviously, the workaround is to save any manifest you Apply in a variable to be used later when you Delete. If that's not possible, I'm not sure I see a solution other than to Get the namespace in the Delete and check for the annotation instead of relying on the annotation in memory.

[vincent-pli]

@jcrossley3
Could we not do the deep-copy, I means for _, spec := range m.resources { replace below line?

manifestival/filter.go

Line 27 in 3577f8e

for _, spec := range m.Resources() {

[jcrossley3]

Well, we could do something like the following, but it would break some tests, since we currently allow the Predicate to mutate the passed resource. We even encourage it.

modified   filter.go
@@ -24,8 +24,8 @@ func (m Manifest) Filter(preds ...Predicate) Manifest {
 	result := m
 	result.resources = []unstructured.Unstructured{}
 	pred := All(preds...)
-	for _, spec := range m.Resources() {
-		if !pred(&spec) {
+	for _, spec := range m.resources {
+		if !pred(spec.DeepCopy()) {
 			continue
 		}
 		result.resources = append(result.resources, spec)

I'm not saying we shouldn't do it, but it would be a fairly significant, breaking change.

[jcrossley3]

The more I think about it, the more I like it. It's probably not great that a Predicate can mutate the resource. It's obvious that Transform will mutate, but that Filter could mutate, too, might violate the "Principle of Least Surprise".

[vincent-pli]

@jcrossley3
I agree with you, Predicate should not mutate the resource, it's server to Filter, should not change original resource.
Just let Transform do the mutate job, I think it's better.

[jcrossley3]

Yeah, and Transform's multi-value return ensures it can't be chained to an Apply. I'll work up a PR.

[jcrossley3]

@vincent-pli if you could go get github.com/manifestival/manifestival@master in your project and verify that everything works as you expect, i'd appreciate it!

[vincent-pli]

@jcrossley3
I have tested with our project, everything works as expect, thanks.