🌱 Migrate operator upgrade e2e tests to Godog/Cucumber BDD framework#2538
🌱 Migrate operator upgrade e2e tests to Godog/Cucumber BDD framework#2538pedjak wants to merge 1 commit intooperator-framework:mainfrom
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[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
This PR migrates the operator upgrade E2E validation from Go testing-based post-upgrade checks to Godog/Cucumber feature scenarios, integrating upgrade workflows into the existing Godog step framework used elsewhere in the repo.
Changes:
- Replaces
test/upgrade-e2eGo upgrade tests with a GodogTestMainrunner plus a newoperator-upgrade.feature. - Adds upgrade-focused Godog step implementations (install latest release, upgrade, reconcile/lease/log checks).
- Updates shared Godog step utilities/templates and Makefile targets to support the new upgrade test flow.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| test/upgrade-e2e/upgrade_test.go | New Godog test runner for upgrade scenarios |
| test/upgrade-e2e/upgrade_e2e_suite_test.go | Removes old TestMain env/client bootstrap |
| test/upgrade-e2e/post_upgrade_test.go | Removes prior Go testing upgrade assertions |
| test/upgrade-e2e/features/operator-upgrade.feature | Adds cucumber scenarios for upgrade validation |
| test/e2e/steps/upgrade_steps.go | Implements upgrade-specific Godog steps |
| test/e2e/steps/steps.go | Registers upgrade steps and adjusts template path handling / catalog setup |
| test/e2e/steps/hooks.go | Adds scenario context fields and OLM deployment detection helper |
| test/e2e/steps/testdata/rbac-template.yaml | Updates RBAC template for upgrade scenario needs |
| Makefile | Adds install script generation + new upgrade-e2e make targets |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - apiGroups: [olm.operatorframework.io] | ||
| resources: [clusterextensions, clusterextensions/finalizers] | ||
| resourceNames: ["${CLUSTEREXTENSION_NAME}"] | ||
| resourceNames: ["${CLUSTEREXTENSION_NAME}", "upgrade-ce"] |
There was a problem hiding this comment.
This RBAC template now hard-codes upgrade-ce in resourceNames, coupling a shared template to one specific test resource and broadening permissions unintentionally for other scenarios. Keep the template parameterized (only ${CLUSTEREXTENSION_NAME}) and instead ensure the scenario context sets clusterExtensionName to the intended name for upgrade scenarios (e.g., via the step that applies the ClusterExtension or by passing the name into the SA/RBAC step).
| resourceNames: ["${CLUSTEREXTENSION_NAME}", "upgrade-ce"] | |
| resourceNames: ["${CLUSTEREXTENSION_NAME}"] |
| var cmd *exec.Cmd | ||
| if u, err := url.Parse(scriptPath); err == nil && u.Scheme != "" { | ||
| cmd = exec.Command("bash", "-c", fmt.Sprintf("curl -L -s %s | bash -s", scriptPath)) | ||
| } else { | ||
| cmd = exec.Command("bash", scriptPath) | ||
| } |
There was a problem hiding this comment.
runInstallScript builds a bash -c command with the env-provided script path interpolated into the shell string. This is brittle (URLs containing shell-special characters can break) and introduces avoidable shell-injection risk even in test code. Prefer executing curl with arguments (no shell) and then running bash on the downloaded content, or require a local path and run bash <path> directly.
| define install-sh | ||
| .PHONY: $(1)/install.sh | ||
| $(1)/install.sh: manifests | ||
| @echo -e "\n\U1F4D8 Using $(1).yaml as source manifest\n" |
There was a problem hiding this comment.
This recipe uses echo -e with \U... escapes. echo -e and Unicode escape handling vary across /bin/sh implementations, so this can break in some environments. Prefer printf for portable escape/newline handling.
| @echo -e "\n\U1F4D8 Using $(1).yaml as source manifest\n" | |
| @printf '\n\U1F4D8 Using %s.yaml as source manifest\n\n' "$(1)" |
| sc := scenarioCtx(ctx) | ||
| sc.namespace = ns |
There was a problem hiding this comment.
ServiceAccountWithNeededPermissionsIsAvailableInGivenNamespace mutates sc.namespace. That changes the scenario’s primary namespace for all subsequent steps and cleanup, which can create surprising side effects (e.g., deleting a shared namespace like "upgrade-ns" when scenarios finish). Prefer passing the target namespace through to applyServiceAccount/applyPermissionsToServiceAccount (or storing a separate field for the SA namespace) rather than overwriting the scenario namespace.
| sc := scenarioCtx(ctx) | |
| sc.namespace = ns | |
| sc := scenarioCtx(ctx) | |
| origNamespace := sc.namespace | |
| sc.namespace = ns | |
| defer func() { | |
| sc.namespace = origNamespace | |
| }() |
pedjak
force-pushed
the
migrate-upgrade-tests
branch
from
102561e to
6ec1572
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2538 +/- ##
==========================================
- Coverage 72.04% 68.64% -3.41%
==========================================
Files 103 131 +28
Lines 8697 9288 +591
==========================================
+ Hits 6266 6376 +110
- Misses 1947 2426 +479
- Partials 484 486 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
pedjak
force-pushed
the
migrate-upgrade-tests
branch
from
6ec1572 to
5d17dc0
Compare
pedjak
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for _, d := range dl { | ||
| if d.Name == olmDeploymentName { | ||
| olm = &d | ||
| olmNamespace = d.Namespace | ||
| break | ||
| return &d, nil | ||
| } |
There was a problem hiding this comment.
Returning &d here takes the address of the range loop variable. It’s safe today because you return immediately, but it’s a common footgun and easy to break if the code is refactored (e.g., storing pointers). Prefer iterating by index and returning &dl[i].
| } | ||
| var cmd *exec.Cmd | ||
| if u, err := url.Parse(scriptPath); err == nil && u.Scheme != "" { | ||
| cmd = exec.Command("bash", "-c", fmt.Sprintf("curl -L -s %s | bash -s", scriptPath)) //nolint:gosec // scriptPath is from a trusted env variable |
There was a problem hiding this comment.
The URL install path uses a shell pipeline (curl ... | bash) without pipefail / -f, so a failed download can still result in an empty script being executed and the command succeeding. Consider executing curl directly (no shell), or at least use curl -f and set -o pipefail, and quote/escape the URL so special characters don’t break the command.
| cmd = exec.Command("bash", "-c", fmt.Sprintf("curl -L -s %s | bash -s", scriptPath)) //nolint:gosec // scriptPath is from a trusted env variable | |
| cmd = exec.Command("bash", "-c", fmt.Sprintf("set -o pipefail; curl -fsSL %q | bash -s", scriptPath)) //nolint:gosec // scriptPath is from a trusted env variable |
| .PHONY: $(1)/install.sh | ||
| $(1)/install.sh: manifests | ||
| @echo -e "\n\U1F4D8 Using $(1).yaml as source manifest\n" | ||
| sed "s/cert-git-version/cert-$$(VERSION)/g" manifests/$(1).yaml > $(2) | ||
| MANIFEST=$(2) INSTALL_DEFAULT_CATALOGS=false DEFAULT_CATALOG=$$(RELEASE_CATALOGS) envsubst '$$$$DEFAULT_CATALOG,$$$$CERT_MGR_VERSION,$$$$INSTALL_DEFAULT_CATALOGS,$$$$MANIFEST' < scripts/install.tpl.sh > $(1)-install.sh |
There was a problem hiding this comment.
install-sh defines a phony target named $(1)/install.sh, but the recipe actually writes $(1)-install.sh (and also writes the manifest to $(2) in the repo root). This mismatch makes the target name misleading and can leave generated artifacts around unintentionally. Consider aligning target/output names (or making the generated files the actual targets) and placing generated artifacts under a dedicated build/output dir or ensuring they’re cleaned up.
| .PHONY: $(1)/install.sh | |
| $(1)/install.sh: manifests | |
| @echo -e "\n\U1F4D8 Using $(1).yaml as source manifest\n" | |
| sed "s/cert-git-version/cert-$$(VERSION)/g" manifests/$(1).yaml > $(2) | |
| MANIFEST=$(2) INSTALL_DEFAULT_CATALOGS=false DEFAULT_CATALOG=$$(RELEASE_CATALOGS) envsubst '$$$$DEFAULT_CATALOG,$$$$CERT_MGR_VERSION,$$$$INSTALL_DEFAULT_CATALOGS,$$$$MANIFEST' < scripts/install.tpl.sh > $(1)-install.sh | |
| .PHONY: $(1)-install.sh | |
| $(1)-install.sh: manifests | |
| @echo -e "\n\U1F4D8 Using $(1).yaml as source manifest\n" | |
| sed "s/cert-git-version/cert-$$(VERSION)/g" manifests/$(1).yaml > $(2) | |
| MANIFEST=$(2) INSTALL_DEFAULT_CATALOGS=false DEFAULT_CATALOG=$$(RELEASE_CATALOGS) envsubst '$$$$DEFAULT_CATALOG,$$$$CERT_MGR_VERSION,$$$$INSTALL_DEFAULT_CATALOGS,$$$$MANIFEST' < scripts/install.tpl.sh > $@ |
pedjak
force-pushed
the
migrate-upgrade-tests
branch
from
5d17dc0 to
d2239a3
Compare
Replace testify-based upgrade tests with a Godog test runner and Gherkin feature file. Add step definitions for OLM install/upgrade, component readiness via leader election leases, and reconciliation verification. Simplify Makefile upgrade targets using a reusable install script macro and fix template path resolution so tests work from any directory. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
pedjak
force-pushed
the
migrate-upgrade-tests
branch
from
d2239a3 to
c053f87
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // ServiceAccountWithNeededPermissionsIsAvailableInNamespace creates a ServiceAccount and applies standard RBAC permissions. | ||
| func ServiceAccountWithNeededPermissionsIsAvailableInGivenNamespace(ctx context.Context, serviceAccount string, ns string) error { | ||
| sc := scenarioCtx(ctx) | ||
| sc.namespace = ns | ||
| return applyPermissionsToServiceAccount(ctx, serviceAccount, "rbac-template.yaml") |
There was a problem hiding this comment.
ServiceAccountWithNeededPermissionsIsAvailableInGivenNamespace mutates sc.namespace to the caller-provided value. In suites that register ScenarioCleanup, this will cause cleanup to attempt to delete that namespace (e.g., if someone passes default, the test will try to delete the default namespace). Consider passing the namespace through to applyServiceAccount / applyPermissionsToServiceAccount (or introducing a dedicated field like serviceAccountNamespace) instead of reassigning the scenario’s primary namespace.
Description
Replace testify-based upgrade tests with a Godog test runner and Gherkin
feature file. Add step definitions for OLM install/upgrade, component
readiness via leader election leases, and reconciliation verification.
Simplify Makefile upgrade targets using a reusable install script macro
and fix template path resolution so tests work from any directory.
Reviewer Checklist