Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@anik120: This pull request references OPRUN-4228 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.21.0" version, but no target version was set. DetailsIn response to this: 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 openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: anik120 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/hold for openshift/cluster-olm-operator#151 to merge first |
openshift-ci
bot
added
the
do-not-merge/hold
|
@anik120: This pull request references OPRUN-4228 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.21.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
| mkdir -p $(@D) | ||
| $(GO_BINDATA) -pkg boxcuttercatalog -o $@ -prefix "testdata/boxcutter/catalog" testdata/boxcutter/catalog/... | ||
| go fmt ./$(@D)/... | ||
|
|
There was a problem hiding this comment.
I can see that we have a lot of it already :-(
Not a great solution
@tmshort I think we will need to look for a way to improve it at some stage
it is not maintainable.
openshift/tests-extension/testdata/boxcutter/bundle/tests/scorecard/config.yaml
Outdated
Show resolved
Hide resolved
openshift/tests-extension/testdata/boxcutter/bundle/metadata/properties.yaml
Outdated
Show resolved
Hide resolved
anik120
force-pushed
the
add-boxcutter-ote-tests
branch
3 times, most recently
from
72637ae to
7d4cb15
Compare
| Expect(foundOwnerRef).To(BeTrue(), "ClusterExtension should be in owner references") | ||
|
|
||
| By("verifying the revision has proper labels") | ||
| Expect(revision.Labels).To(HaveKeyWithValue("olm.operatorframework.io/owner", name), |
There was a problem hiding this comment.
That will be owner-name and can we have a const on top of the file.
I think we will use more than once
| err = k8sClient.Get(ctx, client.ObjectKey{Name: revision2Name}, &revision2) | ||
| Expect(err).ToNot(HaveOccurred(), "revision-2 should exist") | ||
|
|
||
| By("verifying revision-2 is Active") |
There was a problem hiding this comment.
Shoild we not check that the revision-1 is Archived ?
There was a problem hiding this comment.
From what I can see in the code that's in main, the apply() function creates a new revision when upgrading, but doesn't modify the lifecycle state of previous revisions. Unit tests also show that revisions must be manually set to Archived state. And then GC logic (setPreviousRevisions) only deletes revisions that are already in Archived state.
There was a problem hiding this comment.
Can you please check with the main changes? See the demo: #560 (comment)
| // NOTE: GC only deletes ARCHIVED revisions beyond the limit of 5. | ||
| // Active revisions are NEVER deleted regardless of count. | ||
| // We archive 6 revisions here, so when we later trigger a new revision, | ||
| // the oldest archived revision (revision-1) should be deleted, | ||
| // keeping 5 archived revisions (2-6) + active revisions (7-9). |
There was a problem hiding this comment.
All of this workaround should not be required after we get sync with downstream; operator-framework/operator-controller#2315
(see the demo in the PR)
| err := k8sClient.Get(ctx, client.ObjectKey{Name: revisionName}, &revision) | ||
| g.Expect(err).ToNot(HaveOccurred()) | ||
|
|
||
| revision.Spec.LifecycleState = olmv1.ClusterExtensionRevisionLifecycleStateArchived |
There was a problem hiding this comment.
We shouldn’t archive it manually — that should be handled by OLM, right?
So we need to test the expected behavior, and if it’s failing, then we’ll know it needs to be fixed. I believe operator-framework/operator-controller#2315 will address it.
There was a problem hiding this comment.
Oh so looks like this isn't in downstream yet. However, based on a quick look, looks like the archiving behavior is unchanged - ie there's still no automatic archiving behavior - @perdasilva is this intended?
Ack that once this PR downstreams I need to change incorporate the switch from spec.previous field to label-based discovery using olm.operatorframework.io/owner label
There was a problem hiding this comment.
ie there's still no automatic archiving behavior
Can you please test it out?
See the demo: https://asciinema.org/a/qupccwlIdeprj8VvjF7OOko9t
It is fixed with: operator-framework/operator-controller#2315
| ext.Annotations["test.boxcutter/trigger-gc"] = "true" | ||
| err = k8sClient.Update(ctx, &ext) | ||
| g.Expect(err).ToNot(HaveOccurred()) | ||
| }).WithTimeout(helpers.DefaultTimeout).WithPolling(helpers.DefaultPolling).Should(Succeed()) |
There was a problem hiding this comment.
I do not think that we should test it.
What IHMO we need to check is when we uninstall a ClusterExtension all revisions are not left orphoned and are either removed.
Signed-off-by: Anik Bhattacharjee <anbhatta@redhat.com>
Signed-off-by: Anik Bhattacharjee <anbhatta@redhat.com>
anik120
force-pushed
the
add-boxcutter-ote-tests
branch
from
7d4cb15 to
9dac49c
Compare
|
@anik120: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
3 participants
For EP: openshift/enhancements#1890