🌱 Align ClusterObjectSet e2e cleanup with addedResources pattern#2799
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.
Pull request overview
This PR updates the e2e test harness cleanup flow so ClusterObjectSet resources follow the same addedResources-based tracking/cleanup pattern already used for ClusterExtension, ensuring scenarios that create multiple ClusterObjectSets reliably delete all of them during ScenarioCleanup.
Changes:
- Track applied
ClusterObjectSetresources insc.addedResourceswithinResourceIsApplied. - Remove the legacy/special-case
clusterObjectSetName-based cleanup append inScenarioCleanup, relying onaddedResourcesinstead.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| test/e2e/steps/steps.go | Adds ClusterObjectSet to addedResources when applied so cleanup can delete all COS created in a scenario. |
| test/e2e/steps/hooks.go | Removes the special-case COS cleanup block, consolidating deletion behavior around addedResources. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pedjak
left a comment
pedjak
left a comment
There was a problem hiding this comment.
suggestion: Improve PR title and description for clarity
The current title describes the mechanism ("addedResources pattern") but not the problem being solved. Reviewers who haven't read #2783 won't know why alignment matters.
Suggested title:
🌱 Fix e2e cleanup to handle multiple ClusterObjectSets per scenario
Suggested description:
Follow-up to #2783, which consolidated ClusterExtension cleanup into
addedResources
but left ClusterObjectSets on the legacy single-field path.The old cleanup tracked only
sc.clusterObjectSetName— a scalar that gets overwritten
on each apply. Scenarios that create multiple ClusterObjectSets would only clean up the
last one, leaking the rest. This PR applies the same fix #2783 used for ClusterExtensions:
append each applied COS toaddedResourcesat apply time and drop the special-case
cleanup block.What changed:
ResourceIsAppliednow appends every ClusterObjectSet toaddedResources
(theclusterObjectSetNamefield is still set for${COS_NAME}substitution)ScenarioCleanupdrops the single-COS cleanup block —addedResourceshandles it
| sc.addedResources = append(sc.addedResources, resource{name: res.GetName(), kind: "clusterextension"}) | ||
| } else if res.GetKind() == "ClusterObjectSet" { | ||
| sc.clusterObjectSetName = res.GetName() | ||
| sc.addedResources = append(sc.addedResources, resource{name: res.GetName(), kind: "clusterobjectset"}) |
There was a problem hiding this comment.
nitpick (non-blocking): clusterObjectSetName is still assigned here but cleanup has moved to addedResources. A short inline comment noting this field now only serves ${COS_NAME} substitution would help future readers understand why it is still set.
|
thought (non-blocking): If a scenario applies the same COS name twice (e.g., updating it), /lgtm |
Track ClusterObjectSets in addedResources when applied, matching the pattern established for ClusterExtensions in operator-framework#2783. This removes the special-case cleanup block in ScenarioCleanup and ensures multiple ClusterObjectSets per scenario are all cleaned up correctly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
perdasilva
force-pushed
the
e2e-cos-cleanup-alignment
branch
from
bc8c575 to
2e972e6
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pedjak, rashmigottipati 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 |
openshift-ci
Bot
added
the
approved
openshift-merge-bot
Bot
merged commit be16a0a
into
operator-framework:main
…rator-framework#2799) Track ClusterObjectSets in addedResources when applied, matching the pattern established for ClusterExtensions in operator-framework#2783. This removes the special-case cleanup block in ScenarioCleanup and ensures multiple ClusterObjectSets per scenario are all cleaned up correctly. Co-authored-by: Per G. da Silva <pegoncal@redhat.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
addedResources-based cleanup pattern to ClusterObjectSets in the e2e test harnessResourceIsAppliednow appends ClusterObjectSets toaddedResources(alongside settingclusterObjectSetNamefor${COS_NAME}substitution)clusterObjectSetNamecleanup block fromScenarioCleanup, so multiple ClusterObjectSets per scenario are all cleaned up correctlyTest plan
go build ./test/e2e/...)🤖 Generated with Claude Code