Remove workload, flavor, os labels from templates which have older version #50

ksimon1 · 2020-11-25T12:07:16Z

What this PR does / why we need it:
remove workload, flavor, os labels from templates which have older version

With labels removed, old templates will no longer be visible in UI.
Which issue(s) this PR fixes:
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1859235

Release note:

SSP operator now removes workload, flavor, os labels from templates which have older version

ksimon1 · 2020-11-25T12:08:50Z

internal/operands/common-templates/reconcile.go

+		}
+	}
+
+	return common.ResourceStatus{}, nil


@akrejcir, @vatsalparekh what should be returned in this case?

For other resources, this return value signals if the resource is still being updated, or if it is in the desired state. If any resource is still updating, the operator will not be in phase deployed.

In this case however, the operator does not own any of the old templates, so I think they should not block the operator from being in deployed state. The return value can be left as is now.

internal/operands/common-templates/reconcile.go

akrejcir · 2020-12-02T13:01:59Z

internal/operands/common-templates/reconcile.go

+	for _, template := range oldTemplates.Items {
+		for key := range template.Labels {
+			if strings.HasPrefix(key, "os.template.kubevirt.io/") || strings.HasPrefix(key, "flavor.template.kubevirt.io/") || strings.HasPrefix(key, "workload.template.kubevirt.io/") {
+				delete(template.Labels, key)


I didn't know go allows removing elements from map while iterating over it. Good to know it does, in other languages it is usually not a good idea.

This should be safe operation.
See https://golang.org/ref/spec#For_statements

The iteration order over maps is not specified and is not guaranteed to be the same from one iteration to the next. If a map entry that has not yet been reached is removed during iteration, the corresponding iteration value will not be produced. If a map entry is created during iteration, that entry may be produced during the iteration or may be skipped. The choice may vary for each entry created and from one iteration to the next. If the map is nil, the number of iterations is 0.

Yes, it is safe. I just didn't know that before reading this code. I always learn something new.

internal/operands/common-templates/reconcile.go

kubevirt-bot · 2020-12-03T07:48:38Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign akrejcir
You can assign the PR to them by writing /assign @akrejcir in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

akrejcir · 2020-12-03T08:12:00Z

Looks good. Do you think this needs a functional test?

ksimon1 · 2020-12-03T08:44:37Z

Looks good. Do you think this needs a functional test?

Yes, but there is already one https://github.com/kubevirt/ssp-operator/blob/master/tests/e2e-test-csv-generator.sh

akrejcir · 2020-12-03T08:58:43Z

That file only tests CSV generator, not old templates.

ksimon1 · 2020-12-03T09:02:49Z

ah you are right, sorry I posted wrong link

tests/tests_suite_test.go

akrejcir

Looks good, just a few minor comments.

tests/commonTemplates_test.go

tests/tests_suite_test.go

…rsion With labels removed, old templates will no longer be visible in UI. Signed-off-by: Karel Simon <ksimon@redhat.com>

openshift-merge-robot · 2020-12-07T20:24:22Z

@ksimon1: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-functests	`2e70ea2`	link	`/test e2e-functests`
ci/prow/e2e-upgrade-functests	`2e70ea2`	link	`/test e2e-upgrade-functests`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

akrejcir · 2020-12-08T07:52:24Z

tests/commonTemplates_test.go

@@ -122,4 +127,66 @@ var _ = Describe("Common templates", func() {
 			table.Entry("[test_id:4770]golden image NS", &goldenImageNS),
 		)
 	})
+
+	FContext("update old templates", func() {


You forgot to unfocus the test. The CI will fail if any tests are focused.

kubevirt-bot · 2020-12-17T13:45:30Z

@ksimon1: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

vatsalparekh · 2020-12-21T17:20:32Z

Cloned this PR to #71
/close

kubevirt-bot · 2020-12-21T17:20:35Z

@vatsalparekh: Closed this PR.

In response to this:

Cloned this PR to #71
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

kubevirt-bot requested review from shwetaap and vatsalparekh November 25, 2020 12:07

kubevirt-bot added the size/L label Nov 25, 2020

ksimon1 force-pushed the remove-duplicate-labels-templates branch from 84f61b8 to b6637cc Compare November 25, 2020 12:07

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Nov 25, 2020

ksimon1 commented Nov 25, 2020

View reviewed changes

ksimon1 changed the title ~~[WIP] remove workload, flavor, os labels from templates which have older version~~ Remove workload, flavor, os labels from templates which have older version Nov 26, 2020

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 26, 2020

akrejcir requested changes Dec 2, 2020

View reviewed changes

kubevirt-bot assigned akrejcir Dec 2, 2020

ksimon1 force-pushed the remove-duplicate-labels-templates branch from b6637cc to b00edaf Compare December 3, 2020 07:48

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 4, 2020

ksimon1 force-pushed the remove-duplicate-labels-templates branch from b00edaf to 68d7164 Compare December 4, 2020 08:10

kubevirt-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 4, 2020

ksimon1 force-pushed the remove-duplicate-labels-templates branch from 68d7164 to b0acf10 Compare December 7, 2020 12:00

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 7, 2020

akrejcir reviewed Dec 7, 2020

View reviewed changes

tests/tests_suite_test.go Show resolved Hide resolved

akrejcir requested changes Dec 7, 2020

View reviewed changes

tests/commonTemplates_test.go Outdated Show resolved Hide resolved

tests/commonTemplates_test.go Outdated Show resolved Hide resolved

tests/tests_suite_test.go Outdated Show resolved Hide resolved

remove workload, flavor, os labels from templates which have older ve…

2e70ea2

…rsion With labels removed, old templates will no longer be visible in UI. Signed-off-by: Karel Simon <ksimon@redhat.com>

ksimon1 force-pushed the remove-duplicate-labels-templates branch from b0acf10 to 2e70ea2 Compare December 7, 2020 18:32

akrejcir reviewed Dec 8, 2020

View reviewed changes

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 17, 2020

kubevirt-bot closed this Dec 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove workload, flavor, os labels from templates which have older version #50

Remove workload, flavor, os labels from templates which have older version #50

ksimon1 commented Nov 25, 2020

ksimon1 Nov 25, 2020

akrejcir Nov 25, 2020

akrejcir Dec 2, 2020

ksimon1 Dec 3, 2020

akrejcir Dec 3, 2020

kubevirt-bot commented Dec 3, 2020

akrejcir commented Dec 3, 2020

ksimon1 commented Dec 3, 2020

akrejcir commented Dec 3, 2020

ksimon1 commented Dec 3, 2020

akrejcir left a comment

openshift-merge-robot commented Dec 7, 2020

akrejcir Dec 8, 2020 •

edited

kubevirt-bot commented Dec 17, 2020

vatsalparekh commented Dec 21, 2020

kubevirt-bot commented Dec 21, 2020

Remove workload, flavor, os labels from templates which have older version #50

Remove workload, flavor, os labels from templates which have older version #50

Conversation

ksimon1 commented Nov 25, 2020

ksimon1 Nov 25, 2020

Choose a reason for hiding this comment

akrejcir Nov 25, 2020

Choose a reason for hiding this comment

akrejcir Dec 2, 2020

Choose a reason for hiding this comment

ksimon1 Dec 3, 2020

Choose a reason for hiding this comment

akrejcir Dec 3, 2020

Choose a reason for hiding this comment

kubevirt-bot commented Dec 3, 2020

akrejcir commented Dec 3, 2020

ksimon1 commented Dec 3, 2020

akrejcir commented Dec 3, 2020

ksimon1 commented Dec 3, 2020

akrejcir left a comment

Choose a reason for hiding this comment

openshift-merge-robot commented Dec 7, 2020

akrejcir Dec 8, 2020 • edited

Choose a reason for hiding this comment

kubevirt-bot commented Dec 17, 2020

vatsalparekh commented Dec 21, 2020

kubevirt-bot commented Dec 21, 2020

akrejcir Dec 8, 2020 •

edited