resourceapply: Allow removing run-level labels from namespaces by stlaz · Pull Request #727 · openshift/library-go

stlaz · 2020-03-06T12:07:34Z

A couple of components were using run-level labels on their
namespaces even though they did not have to. Make it possible
to remove this label with the library-go functions while not
changing the other labels/annotations in case these are maintained
automatically by a different controller.

sttts · 2020-03-06T12:13:34Z

+
+	// allow removing runlevels since the metadata are otherwise merged
+	_, requiredRunLevelPresent := required.ObjectMeta.Labels["run-level"]
+	if _, ok := existingCopy.ObjectMeta.Labels["run-level"]; ok != requiredRunLevelPresent && !requiredRunLevelPresent {


the add case works?

ok && !requiredRunLevelPresent is simpler

The add case is handled by the EnsureObjectMeta() function above.

Good simplification, I modified the condition in latest revision.

sttts · 2020-03-06T14:59:06Z

 func ApplyConfigMap(client coreclientv1.ConfigMapsGetter, recorder events.Recorder, required *corev1.ConfigMap) (*corev1.ConfigMap, bool, error) {
 	existing, err := client.ConfigMaps(required.Namespace).Get(required.Name, metav1.GetOptions{})
 	if apierrors.IsNotFound(err) {
+		resourcemerge.CleanMetaLabelsAnnotations(&required.ObjectMeta)


mutating the input is bad. We need a deep copy (or shallow copy of at least the root and ObjectMeta).

I went with the DeepCopy() should we ever want to mutate any other fields of the actual object as well

performance here does not matter anyway

sttts · 2020-03-06T15:45:14Z

-		actual, err := client.Namespaces().Create(required)
-		reportCreateEvent(recorder, required, err)
+		requiredCopy := required.DeepCopy()
+		resourcemerge.CleanMetaLabelsAnnotations(&requiredCopy.ObjectMeta)


how about:

actual, err := client.Namespaces().Create(WithCleanLabelsAndAnnotations(requiredCopy))`

If an annotation/label key ends with "-", remove that that key (ending "-"s are trimmed) from the existing map instead.

sttts · 2020-03-09T12:06:42Z

+		requiredCopy := required.DeepCopy()
+		actual, err := client.Namespaces().
+			Create(resourcemerge.WithCleanLabelsAndAnnotations(requiredCopy).(*corev1.Namespace))
+		reportCreateEvent(recorder, requiredCopy, err)


report requiredCopy or actual or cleaned version?

actual could be nil here in case of an error, so I guess the cleaned version should be reported. In C I would be sure that requiredCopy == cleaned, not sure if Go does additional magic to the pointer being passed to WithCleanLabelsAndAnnotations 🤔

Yes, they are equal

I don't care much which one you print, just decide conciously :)

sttts · 2020-03-09T12:09:03Z

+			// if "required" map contains a key with "-" as suffix, remove that
+			// key from the existing map instead of replacing the value
+			if strings.HasSuffix(k, "-") {
+				delete(*existing, strings.TrimRight(k, "-"))


Intentional to remove arbitrary many - here?

yes, you need to remove the trailing - to get the actual name of the field you're trying to remove:

original: run-level: 0 required: run-level-:

This is different in case of https://github.com/openshift/library-go/pull/727/files#diff-3567eb2cd10c84b30387be04408ab7b2R27 where the object being cleaned is the object that's actually getting created.

I meant that TrimRight also removes --- from foo---.

sttts · 2020-03-09T15:15:35Z

/lgtm
/approve

openshift-ci-robot · 2020-03-09T15:17:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: stlaz, sttts

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [sttts]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

resourceapply: Allow removing run-level labels from namespaces

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 6, 2020

openshift-ci-robot requested review from smarterclayton and sttts March 6, 2020 12:09

sttts reviewed Mar 6, 2020

View reviewed changes

stlaz force-pushed the ns_apply_runlevels branch 3 times, most recently from 57bdb65 to 148d711 Compare March 6, 2020 14:03

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 6, 2020

stlaz force-pushed the ns_apply_runlevels branch from 148d711 to 57bdb65 Compare March 6, 2020 14:10

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 6, 2020

stlaz force-pushed the ns_apply_runlevels branch from 57bdb65 to 1f5fc32 Compare March 6, 2020 14:34

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 6, 2020

sttts reviewed Mar 6, 2020

View reviewed changes

stlaz force-pushed the ns_apply_runlevels branch from 1f5fc32 to d477ea7 Compare March 6, 2020 15:39

sttts reviewed Mar 6, 2020

View reviewed changes

stlaz force-pushed the ns_apply_runlevels branch from d477ea7 to c6fa45c Compare March 6, 2020 16:01

resourcemerge: Allow removing annotations/labels

3a6456f

If an annotation/label key ends with "-", remove that that key (ending "-"s are trimmed) from the existing map instead.

stlaz force-pushed the ns_apply_runlevels branch from c6fa45c to 3a6456f Compare March 6, 2020 16:08

sttts reviewed Mar 9, 2020

View reviewed changes

openshift-ci-robot assigned sttts Mar 9, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 9, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 9, 2020

openshift-merge-robot merged commit d18a5ca into openshift:master Mar 9, 2020

stlaz mentioned this pull request Mar 10, 2020

Bug 1805919: Insights operator does not require being in an openshift run-level to function openshift/insights-operator#80

Merged

bertinatto pushed a commit to bertinatto/library-go that referenced this pull request Jul 2, 2020

Merge pull request openshift#727 from stlaz/ns_apply_runlevels

9585e4f

resourceapply: Allow removing run-level labels from namespaces

mcoops mentioned this pull request Nov 29, 2021

Bug 2020107: Remove run-level label openshift/cluster-version-operator#623

Merged

Conversation

stlaz commented Mar 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sttts Mar 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sttts Mar 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sttts commented Mar 9, 2020

Uh oh!

openshift-ci-robot commented Mar 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sttts Mar 9, 2020 •

edited

Loading

sttts Mar 9, 2020 •

edited

Loading