🌱 Add e2e test for direct ClusterObjectSet creation with Secret-stored objects#2624
🌱 Add e2e test for direct ClusterObjectSet creation with Secret-stored objects#2624pedjak wants to merge 1 commit intooperator-framework:mainfrom
Conversation
…d objects Adds a scenario verifying that users can create a ClusterObjectSet with phase objects externalized to Secrets via ref, rather than inline. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
[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 |
✅ 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
Adds an e2e scenario to validate direct ClusterObjectSet creation where phase objects are externalized in a Secret via ref, and improves e2e cleanup to handle additional (non-CE/COS) resources and namespaced deletions.
Changes:
- Add
revision.featurescenario forClusterObjectSetreferencing Secret-stored objects viaref. - Track additional applied resources for cleanup in
ResourceIsApplied. - Extend cleanup deletion to include
-n <namespace>when applicable.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/e2e/steps/steps.go | Tracks non-CE/COS applied resources (incl. namespace) for cleanup. |
| test/e2e/steps/hooks.go | Adds namespace field to tracked resources and uses it when deleting. |
| test/e2e/features/revision.feature | Adds new e2e scenario for COS creation via Secret ref objects. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| args = append(args, "-n", res.namespace) | ||
| } | ||
| if _, err := k8sClient(args...); err != nil { | ||
| logger.Info("Error deleting resource", "name", res.name, "namespace", sc.namespace, "stderr", stderrOutput(err)) |
There was a problem hiding this comment.
The log field namespace uses sc.namespace, but the deletion is performed with res.namespace (and some resources may be cluster-scoped / other namespaces). This makes failure logs misleading. Log res.namespace (and optionally also log sc.namespace under a different key if you need scenario context).
| logger.Info("Error deleting resource", "name", res.name, "namespace", sc.namespace, "stderr", stderrOutput(err)) | |
| logger.Info("Error deleting resource", "name", res.name, "namespace", res.namespace, "scenarioNamespace", sc.namespace, "stderr", stderrOutput(err)) |
| sc.addedResources = append(sc.addedResources, resource{ | ||
| name: res.GetName(), | ||
| kind: strings.ToLower(res.GetKind()), | ||
| namespace: res.GetNamespace(), |
There was a problem hiding this comment.
For namespaced resources applied without an explicit metadata.namespace in the YAML, res.GetNamespace() will be empty (because this is parsed from the manifest). With the new cleanup logic, those resources would be deleted without -n, likely leaving them behind. Consider defaulting namespace to the scenario/test namespace when res.GetNamespace() is empty (at least for this e2e harness), so cleanup reliably targets the namespace resources were applied into.
| sc.addedResources = append(sc.addedResources, resource{ | |
| name: res.GetName(), | |
| kind: strings.ToLower(res.GetKind()), | |
| namespace: res.GetNamespace(), | |
| namespace := res.GetNamespace() | |
| if namespace == "" { | |
| namespace = sc.namespace | |
| } | |
| sc.addedResources = append(sc.addedResources, resource{ | |
| name: res.GetName(), | |
| kind: strings.ToLower(res.GetKind()), | |
| namespace: namespace, |
| go func(res resource) { | ||
| if _, err := k8sClient("delete", res.kind, res.name, "--ignore-not-found=true"); err != nil { | ||
| args := []string{"delete", res.kind, res.name, "--ignore-not-found=true"} | ||
| if res.namespace != "" { | ||
| args = append(args, "-n", res.namespace) | ||
| } | ||
| if _, err := k8sClient(args...); err != nil { | ||
| logger.Info("Error deleting resource", "name", res.name, "namespace", sc.namespace, "stderr", stderrOutput(err)) | ||
| } | ||
| }(r) |
There was a problem hiding this comment.
Cleanup deletes all resources concurrently. With the PR tracking more resources, it becomes more likely that namespace deletion races with deletions of namespaced resources, creating intermittent cleanup failures/noise. Consider deleting in a deterministic order (e.g., delete tracked resources first, then delete the namespace last) and/or avoiding goroutines here so the namespace isn’t removed while other deletes are still in-flight.
Description
Adds an e2e scenario to
revision.featureverifying that users can directly create aClusterObjectSetwith phase objects externalized to Secrets viaref, rather than inlineobjectentries.Previously,
revision.featureonly tested inline objects, and the Secret externalization test ininstall.featureonly covered theClusterExtensionpath. This fills that gap.Changes
stringDataentriesClusterObjectSetreferencing those objects viarefResourceIsAppliednow tracks non-CE/COS resources inaddedResourcesfor cleanupScenarioCleanupsupports namespace-aware resource deletionTest plan
make lintpassesmake test-e2epasses (46 scenarios, 45 passed, 1 pre-existing undefined inuser-managed-fields.feature)🤖 Generated with Claude Code