Skip to content

🌱 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.

Sign up for GitHub

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

Jump to bottom
Open
perdasilva wants to merge 1 commit into
base: main
Choose a base branch
from
Open

🌱 Add uninstall feature test #2453

perdasilva wants to merge 1 commit into from
+98 −14

Conversation

@perdasilva
Copy link
Contributor

@perdasilva perdasilva commented Jan 20, 2026 •
edited
Loading

Description

Adds initial uninstall feature with two tests:

  • cluster extension deletion removes bundle resources from the cluster
  • cluster extension deletion after service account deletion removed bundle resources from the cluster

Note: this PR also does some low level variable name refactoring to improve readability

Reviewer Checklist

  • API Go Documentation
  • Tests: Unit Tests (and E2E Tests, if appropriate)
  • Comprehensive Commit Messages
  • Links to related GitHub Issue(s)

Copilot AI review requested due to automatic review settings January 20, 2026 17:16
@openshift-ci openshift-ci bot requested review from bentito and pedjak January 20, 2026 17:16
@openshift-ci
Copy link

openshift-ci bot commented Jan 20, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign joelanford for approval. For more information see the Code Review Process.

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

Details Needs approval from an approver in each of these files:

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

@netlify
Copy link

netlify bot commented Jan 20, 2026 •
edited
Loading

✅ Deploy Preview for olmv1 ready!

Name Link
🔨 Latest commit 876fc6b
🔍 Latest deploy log https://app.netlify.com/projects/olmv1/deploys/6971054e126e0a0008dac4da
😎 Deploy Preview https://deploy-preview-2453--olmv1.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Copilot AI reviewed Jan 20, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a 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.feature file 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
Copy link

codecov bot commented Jan 20, 2026 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.50%. Comparing base (8167ff8) to head (876fc6b).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff            @@
##             main    #2453    +/-   ##
========================================
  Coverage   69.49%   69.50%            
========================================
  Files         101      101            
  Lines        7754     7891   +137     
========================================
+ Hits         5389     5485    +96     
- Misses       1930     1968    +38     
- Partials      435      438     +3
Flag Coverage Δ
e2e 46.94% <ø> (+0.83%) ⬆️
experimental-e2e 13.71% <ø> (+0.25%) ⬆️
unit 57.07% <ø> (-0.11%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@perdasilva perdasilva marked this pull request as draft January 20, 2026 18:27
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 20, 2026
@perdasilva perdasilva marked this pull request as ready for review January 21, 2026 07:28
Copilot AI review requested due to automatic review settings January 21, 2026 07:28
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 21, 2026
@openshift-ci openshift-ci bot requested review from grokspawn and trgeiger January 21, 2026 07:28
Copilot AI reviewed Jan 21, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a 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.

@perdasilva perdasilva changed the title 🌱 Add unintall feature test 🌱 Add uninstall feature test Jan 21, 2026
camilamacedo86
camilamacedo86 reviewed Jan 21, 2026
View reviewed changes
camilamacedo86
camilamacedo86 reviewed Jan 21, 2026
View reviewed changes
Copy link
Contributor

@camilamacedo86 camilamacedo86 left a 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

@perdasilva
Copy link
Contributor Author

perdasilva commented Jan 21, 2026

/hold to add another test

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 21, 2026
Copilot AI review requested due to automatic review settings January 21, 2026 14:08
@perdasilva
Copy link
Contributor Author

perdasilva commented Jan 21, 2026

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 21, 2026
Copilot AI reviewed Jan 21, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a 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.

camilamacedo86
camilamacedo86 reviewed Jan 21, 2026
View reviewed changes
camilamacedo86
camilamacedo86 reviewed Jan 21, 2026
View reviewed changes
test/e2e/features/uninstall.feature Outdated
And resource "clusterrole/testoperator.v1.2.-37mym6pni2xxmai9n7fmhtbn9i348lx7o619rmf3ypio" is eventually not found
And resource "clusterrole/testoperator.v1.2.0-t88i5epjh8oxp4klplhjyrsekwcp92b27w03ayr1ku5" is eventually not found
And resource "clusterrolebinding/testoperator.v1.2.-37mym6pni2xxmai9n7fmhtbn9i348lx7o619rmf3ypio" is eventually not found
And resource "clusterrolebinding/testoperator.v1.2.0-t88i5epjh8oxp4klplhjyrsekwcp92b27w03ayr1ku5" is eventually not found No newline at end of file
Copy link
Contributor

@camilamacedo86 camilamacedo86 Jan 21, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think we can have those fixed. Can we not improve that and search by label something like that?

/approved cancel (due new additionals)

Copy link
Contributor Author

@perdasilva perdasilva Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't see a way around it. If we use a label, all we assert is that there are no clusterroles owned by the ClusterExtension. But, they could still exist on the cluster =/

Copy link
Contributor Author

@perdasilva perdasilva Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This brought up something I hadn't thought about: if the names change, then we'll get a false positive. I've updated the Background to ensure the resources exist with those names.

Copy link
Contributor

@camilamacedo86 camilamacedo86 Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep—I don’t think we can do that.

Also, my understanding is that e2e tests should reflect what an end user expects. As a user, I’d expect all owned files to be deleted, while keeping the CRDs to avoid data loss.

Since this is all managed by OLM, do we have any label/annotation we can rely on to identify everything OLM manages for a given CE?

Copilot AI review requested due to automatic review settings January 21, 2026 15:52
Copilot AI reviewed Jan 21, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a 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.

@perdasilva perdasilva force-pushed the uninstall-e2e-test branch 2 times, most recently from 34df40f to 20cf71d Compare January 21, 2026 16:19
Copilot AI review requested due to automatic review settings January 21, 2026 16:19
Copilot AI reviewed Jan 21, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a 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 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot AI review requested due to automatic review settings January 21, 2026 16:37
Copilot AI reviewed Jan 21, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a 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 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Add unintall feature test
876fc6b 
Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@camilamacedo86 camilamacedo86 camilamacedo86 left review comments

Copilot code review Copilot Copilot left review comments

@bentito bentito Awaiting requested review from bentito

@pedjak pedjak Awaiting requested review from pedjak

@grokspawn grokspawn Awaiting requested review from grokspawn

@trgeiger trgeiger Awaiting requested review from trgeiger

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@perdasilva @camilamacedo86