🌱 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
✅ 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)" |
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 68.61% 68.63% +0.02%
==========================================
Files 131 131
Lines 9333 9333
==========================================
+ Hits 6404 6406 +2
+ Misses 2438 2437 -1
+ Partials 491 490 -1
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
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.
test/e2e/steps/steps.go
Outdated
| // 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.
| sc.Step(`^(?i)ServiceAccount "([^"]*)" with needed permissions is available in \${TEST_NAMESPACE}$`, ServiceAccountWithNeededPermissionsIsAvailableInNamespace) | ||
| sc.Step(`^(?i)ServiceAccount "([^"]*)" with permissions to install extensions is available in "([^"]*)" namespace$`, ServiceAccountWithNeededPermissionsIsAvailableInGivenNamespace) | ||
| sc.Step(`^(?i)ServiceAccount "([^"]*)" with needed permissions is available in test namespace$`, ServiceAccountWithNeededPermissionsIsAvailableInTestNamespace) | ||
| sc.Step(`^(?i)ServiceAccount "([^"]*)" with needed permissions is available in \${TEST_NAMESPACE}$`, ServiceAccountWithNeededPermissionsIsAvailableInTestNamespace) |
There was a problem hiding this comment.
we may need to rebase as there's been a recent change affecting ServiceAccountWithNeededPermissionsIsAvailableInNamespace
| olmNamespace = "olmv1-system" | ||
| kubeconfigPath string | ||
| k8sCli string | ||
| deployImageRegistry = sync.OnceValue(func() error { |
There was a problem hiding this comment.
Any reason not to make this just part of BeforeSuite?
There was a problem hiding this comment.
Any reason not to make this just part of BeforeSuite?
Currently, image-registry depends on the installation of OLM - it uses a CA certificate to create its own. It is a bit of unfortunate, but in these sort of tests, we would like to install and upgrade OLM as a part of test scenario. Thus, we cannot put this logic in BeforeSuite, because OLM is not installed at that point. Perhaps we could relax OLM installation a bit so that it could use already installed certificate, but that requires moving us to Helm installation method. Anyhow, that would make this PR a way larger.
There was a problem hiding this comment.
makes sense! thank you =D
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
36ad16e to
56dddb8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| if err := applyServiceAccount(ctx, serviceAccount, "TEST_NAMESPACE", ns); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
ServiceAccountWithNeededPermissionsIsAvailableInGivenNamespace applies the ServiceAccount in the provided ns, but then calls applyPermissionsToServiceAccount, which (currently) also calls applyServiceAccount using the scenario’s default sc.namespace. This creates an extra ServiceAccount/Namespace unrelated to the provided namespace. Consider adjusting applyPermissionsToServiceAccount to pass the same TEST_NAMESPACE override through to applyServiceAccount (or skipping the duplicate call here) so the ServiceAccount is only created in the intended namespace.
| if err := applyServiceAccount(ctx, serviceAccount, "TEST_NAMESPACE", ns); err != nil { | |
| return err | |
| } |
| test-st2ex-e2e: export MANIFEST := $(STANDARD_RELEASE_MANIFEST) | ||
| test-st2ex-e2e: export TEST_CLUSTER_CATALOG_NAME := test-catalog | ||
| test-st2ex-e2e: export TEST_CLUSTER_EXTENSION_NAME := test-package |
There was a problem hiding this comment.
test-st2ex-e2e still exports MANIFEST, TEST_CLUSTER_CATALOG_NAME, and TEST_CLUSTER_EXTENSION_NAME, but the upgrade test flow no longer consumes these variables (the old pre/post upgrade scripts were removed and the new Godog feature hard-codes the resource names). These exports are now dead configuration and can be removed to avoid confusion about what the upgrade tests actually use.
| test-st2ex-e2e: export MANIFEST := $(STANDARD_RELEASE_MANIFEST) | |
| test-st2ex-e2e: export TEST_CLUSTER_CATALOG_NAME := test-catalog | |
| test-st2ex-e2e: export TEST_CLUSTER_EXTENSION_NAME := test-package |
| test-st2ex-e2e: export TEST_CLUSTER_CATALOG_NAME := test-catalog | ||
| test-st2ex-e2e: export TEST_CLUSTER_EXTENSION_NAME := test-package | ||
| test-st2ex-e2e: run-internal image-registry pre-upgrade-setup kind-deploy-experimental post-upgrade-checks kind-clean #HELP Run swichover (standard -> experimental) e2e tests on a local kind cluster | ||
| test-st2ex-e2e: experimental/install.sh standard/install.sh $(TEST_UPGRADE_E2E_TASKS) #HELP Run swichover (standard -> experimental) e2e tests on a local kind cluster |
There was a problem hiding this comment.
Typo in help text: swichover should be switchover.
| test-st2ex-e2e: experimental/install.sh standard/install.sh $(TEST_UPGRADE_E2E_TASKS) #HELP Run swichover (standard -> experimental) e2e tests on a local kind cluster | |
| test-st2ex-e2e: experimental/install.sh standard/install.sh $(TEST_UPGRADE_E2E_TASKS) #HELP Run switchover (standard -> experimental) e2e tests on a local kind cluster |
|
/override go-apidiff this PR does not touch any of API |
|
@pedjak: Overrode contexts on behalf of pedjak: go-apidiff DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rashmigottipati, tmshort 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
|
/override go-apidiff |
|
@tmshort: Overrode contexts on behalf of tmshort: go-apidiff DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
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