🌱 Align ClusterObjectSet e2e cleanup with addedResources pattern#2799
🌱 Align ClusterObjectSet e2e cleanup with addedResources pattern#2799perdasilva wants to merge 1 commit into
Conversation
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>
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
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 |
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