Skip to content

🐛 Teardown revision on archival #2502

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

🐛 Teardown revision on archival #2502

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

Conversation

@perdasilva
Copy link
Contributor

@perdasilva perdasilva commented Feb 11, 2026 •
edited
Loading

Description

  • Call revisionEngine.Teardown() when a ClusterExtensionRevision enters the Archived lifecycle state, ensuring managed resources removed between bundle versions are cleaned up from the cluster
  • Move RevisionEngine creation before the teardown/reconcile branch so it is available for both paths
  • Handle incomplete teardown (requeue after 5s) and teardown errors (propagate for controller retry) — previously errors were silently swallowed
  • Add dummy-configmap to the v1.0.0 test bundle (absent in v1.2.0) and assert its removal in the "Each update creates a new revision" e2e test

Test plan

  • Unit tests pass (Test_ClusterExtensionRevisionReconciler_Reconcile_Deletion)
  • New unit test: requeue when archived revision teardown is incomplete
  • New unit test: error propagation when archived revision teardown fails
  • teardown() coverage improved from 68.8% to 93.8%
  • E2e: "Each update creates a new revision" verifies dummy-configmap is removed after archival

🤖 Generated with Claude Code

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 February 11, 2026 13:49
@netlify
Copy link

netlify bot commented Feb 11, 2026 •
edited
Loading

✅ Deploy Preview for olmv1 ready!

Name Link
🔨 Latest commit 5ca3f6e
🔍 Latest deploy log https://app.netlify.com/projects/olmv1/deploys/698c99d2e4b4aa00081b2f27
😎 Deploy Preview https://deploy-preview-2502--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.

@openshift-ci
Copy link

openshift-ci bot commented Feb 11, 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 perdasilva 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

@perdasilva perdasilva force-pushed the fix-cer-archival-teardown branch from 01760b0 to 06afd4a Compare February 11, 2026 13:52
Copilot AI reviewed Feb 11, 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

Updates ClusterExtensionRevision teardown behavior to actively tear down Boxcutter-managed resources when a revision is archived, and extends test coverage/data to validate resource removal across bundle versions.

Changes:

  • Invoke RevisionEngine.Teardown during teardown for archived revisions, with retry + requeue when teardown is incomplete.
  • Expand deletion/archival reconciliation tests to cover incomplete teardown and teardown failures (including expected requeue behavior).
  • Add a dummy ConfigMap manifest to the v1.0.0 test bundle to validate removal of resources that disappear in later bundle versions.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
internal/operator-controller/controllers/clusterextensionrevision_controller.go Calls Boxcutter teardown for archived revisions and requeues while teardown is incomplete.
internal/operator-controller/controllers/clusterextensionrevision_controller_test.go Adds test cases for archived teardown retry/error paths and asserts reconcile results.
testdata/images/bundles/test-operator/v1.0.0/manifests/dummy.configmap.yaml Introduces a version-specific manifest used to test removal of resources across upgrades.

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

@codecov
Copy link

codecov bot commented Feb 11, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 95.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 69.86%. Comparing base (e17412a) to head (5ca3f6e).

Files with missing lines Patch % Lines
...controllers/clusterextensionrevision_controller.go 95.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2502      +/-   ##
==========================================
+ Coverage   69.84%   69.86%   +0.01%     
==========================================
  Files         102      102              
  Lines        8496     8508      +12     
==========================================
+ Hits         5934     5944      +10     
- Misses       2090     2091       +1     
- Partials      472      473       +1
Flag Coverage Δ
e2e 46.09% <0.00%> (-0.08%) ⬇️
experimental-e2e 12.87% <0.00%> (-0.08%) ⬇️
unit 58.03% <95.00%> (+0.05%) ⬆️

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.

Call RevisionEngine.Teardown when CER is archived
5ca3f6e 
When a ClusterExtensionRevision transitions to the Archived lifecycle
state, invoke the revision engine's Teardown method to clean up managed
resources that are no longer part of the active revision. This ensures
resources removed between bundle versions (e.g. a ConfigMap present in
v1.0.0 but absent in v1.2.0) are deleted from the cluster.

Changes:
- Move RevisionEngine creation before the teardown check so it is
  available for both teardown and reconcile paths
- Pass RevisionEngine and boxcutter Revision into teardown()
- Call revisionEngine.Teardown() for archived revisions and handle
  incomplete teardown (requeue after 5s) and errors (return error
  for controller retry)
- Remove redundant lifecycle state check and fix swallowed teardown
  error
- Add unit tests for incomplete teardown requeue and teardown error
  propagation (teardown coverage: 68.8% -> 93.8%)
- Add dummy-configmap to v1.0.0 test bundle (absent in v1.2.0) and
  assert it is removed in the "Each update creates a new revision"
  e2e test

Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>
@perdasilva perdasilva force-pushed the fix-cer-archival-teardown branch from 06afd4a to 5ca3f6e Compare February 11, 2026 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@camilamacedo86 camilamacedo86 Awaiting requested review from camilamacedo86

@grokspawn grokspawn Awaiting requested review from grokspawn

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@perdasilva