-
Notifications
You must be signed in to change notification settings - Fork 71
🌱 Add uninstall feature test #2453
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
🌱 Add uninstall feature test #2453
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
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.
Pull request overview
This PR adds an end-to-end test for verifying that bundle resources are properly removed when a ClusterExtension is uninstalled. The test uses the Gherkin BDD format to define the uninstall scenario.
Changes:
- Added new
uninstall.featurefile with a test scenario that verifies resource cleanup after ClusterExtension deletion
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2453 +/- ##
==========================================
- Coverage 74.02% 73.52% -0.50%
==========================================
Files 101 102 +1
Lines 7991 8231 +240
==========================================
+ Hits 5915 6052 +137
- Misses 1626 1718 +92
- Partials 450 461 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
d7180c9 to
255a7c0
Compare
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.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
255a7c0 to
1bd6544
Compare
camilamacedo86
left a comment
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.
Just I nit otherwise
/approved
|
/hold to add another test |
1bd6544 to
1b10dc6
Compare
|
/unhold |
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.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1b10dc6 to
63cb602
Compare
|
/lgtm cancel I think we need a second person doing reviews too :-) |
I was ok with it, but @pedjak needed to check his concerns were addressed. |
test/e2e/features/uninstall.feature
Outdated
|
|
||
| Scenario: Removing ClusterExtension triggers the extension uninstall, eventually removing all installed resources | ||
| When ClusterExtension is removed | ||
| Then ClusterExtension is uninstalled |
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.
we should be here more verbose, i.e. to replace "uninstalled" with what actually happens:
| Then ClusterExtension is uninstalled | |
| Then resources being part of ClusterExtension are removed |
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.
Would "ClusterExtension resources are removed" work as well?
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.
Would "ClusterExtension resources are removed" work as well?
hm, not sure. When I read it like that I would understand it as resources of type ClusterExtension are removed. What we actually assert that all resources owned by the removed ClusterExtension are removed too.
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.
What about "the ClusterExtension's constituent resources are removed"?
test/e2e/steps/steps.go
Outdated
| kind := obj.GetObjectKind().GroupVersionKind().Kind | ||
| clusterObj, err := getResource(kind, obj.GetName(), obj.GetNamespace()) | ||
| if err != nil { | ||
| godog.Logf(ctx, "error getting resource name=%q namespace=%q kind=%q: %v", obj.GetName(), obj.GetNamespace(), kind, err) |
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.
use logger instance.
test/e2e/steps/steps.go
Outdated
| if labels["olm.operatorframework.io/owner-kind"] != "ClusterExtension" { | ||
| godog.Logf(ctx, "resource name=%q namespace=%q kind=%q has bad owner-kind label %q (expected=ClusterExtension)", obj.GetName(), obj.GetNamespace(), kind, labels["olm.operatorframework.io/owner-kind"]) | ||
| return false | ||
| } | ||
| if labels["olm.operatorframework.io/owner-name"] != sc.clusterExtensionName { |
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.
nit: perhaps you can declare a map with labels you want to check and then have a for loop over them?
test/e2e/steps/steps.go
Outdated
| return fmt.Errorf("resource %s is not in the format <kind>/<name>", resource) | ||
| } | ||
|
|
||
| require.Eventually(godog.T(ctx), func() bool { |
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.
use waitFor method available in this file already.
| And resource "networkpolicy/test-operator-network-policy" is installed | ||
| And resource "configmap/test-configmap" is installed | ||
| And resource "deployment/test-operator" is installed |
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.
I would keep the verbose assertions here, because it can gives us better signal when the test fails.
| And resource "networkpolicy/test-operator-network-policy" is installed | ||
| And resource "configmap/test-configmap" is installed | ||
| And resource "deployment/test-operator" is installed |
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.
same as above.
test/e2e/steps/steps.go
Outdated
| "github.com/cucumber/godog" | ||
| jsonpatch "github.com/evanphx/json-patch" | ||
| "github.com/google/go-cmp/cmp" | ||
| diff "github.com/google/go-cmp/cmp" |
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.
not sure if we really need this diff alias, IMHO cmp is clear?
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.
good catch - I needed to make that change in an earlier iteration where we imported the standard cmp library!
89e8b11 to
8172d3a
Compare
8172d3a to
13de323
Compare
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.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ffbf321 to
acd8577
Compare
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.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
425545d to
34dcd55
Compare
Signed-off-by: Per Goncalves da Silva <[email protected]>
34dcd55 to
a3f4ae0
Compare
pedjak
left a comment
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
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86, pedjak, tmshort The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fbe909f
into
operator-framework:main
Description
Updates e2e suite to add the uninstall feature with two tests:
Updates the
ClusterExtension is rolled outstep to collect the cluster extension resources and attach them to the test's contextThe following steps are added:
ClusterExtension resources are created and labeled: checks that the extension resources in the context exist and carry the appropriate olm owner-kind and owner-name labelsClusterExtension is removed: deletes the ClusterExtension under testClusterExtension resources are removed: checks that the extension resources in the context (eventually) no longer exist on the clusterresource <some resource> is eventually not found: checks that the resource is eventually not on the clusterThe extension resources are acquired from either the Helm release secret or the latest active ClusterExtensionRevision for the ClusterExtension depending on the feature-gate settings.
Note: OLM's internal helm client uses a custom release backend that can handle arbitrarily large bundles. In the e2e implementation, getting the helm release resources does not yet support sharded releases because:
Reviewer Checklist