-
Notifications
You must be signed in to change notification settings - Fork 104
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
Apply labels and annotations only in the correct paths #1499
Conversation
…n the GVK Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One nit but otherwise 👍
@@ -107,6 +107,9 @@ func TestSetDependenciesHash(t *testing.T) { | |||
assert.NilError(t, err) | |||
assert.DeepEqual(t, pod("somename", "namespace"), p) | |||
}}, | |||
{name: "no change in unstructured CRD", obj: unstructuredCrd("crd", "namespace"), assert: func(us *unstructured.Unstructured) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So there is no easy way to collect the CRDs as part of the diagnostics (or cleanup for that matter) 😞
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean because it didn't change? That's not correct, this test is specifically about the dependencies hash - no dependencies for CRDs, therefore no change.
I've extended one test in enhancer_test.go
to verify that and make it more obvious that the top-level annotations in metadata/annotations
and metadata/labels
are always added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
Adding labels/annotations in all possible paths and relying on conversion to typed objects to remove invalid paths obviously only works for resources where we have the typed objects available. For almost all CRDs this leaves the resources with unwanted labels and annotations in multiple locations.
We now have custom lists for locations based on the G(V)K.
Signed-off-by: Andreas Neumann aneumann@mesosphere.com
Fixes #1498