OLS-3265: Add Konflux integration test pipeline and e2e test improvements#113
OLS-3265: Add Konflux integration test pipeline and e2e test improvements#113blublinsky wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@blublinsky: This pull request references OLS-3265 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
Warning Review limit reached
More reviews will be available in 44 minutes and 44 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughPR adds sandbox-mode configuration to the Makefile, an installer script, a Tekton pipeline + IntegrationTestScenario for ephemeral-cluster e2e runs, manager arg placeholders, and e2e test/helper updates that assert per-proposal ServiceAccount/RBAC and require explicit verification approval sequencing. ChangesEnd-to-end test infrastructure and CI/CD
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.tekton/integration-tests/pipelines/agentic-operator-e2e-pipeline.yaml:
- Line 148: The pipeline is silently ignoring checkout failures because the
command 'git fetch --depth=1 origin "${COMMIT}" && git checkout "${COMMIT}" ||
true' swallows errors; remove the trailing '|| true' so the shell returns a
non‑zero exit and the Tekton task fails on fetch/checkout errors, or
alternatively replace it with an explicit validation step after checkout that
verifies the current HEAD matches "${COMMIT}" and exits non‑zero if not (update
the existing command string referenced above).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: db93b893-0f21-4d09-8d7f-2c94fe7a2e1b
📒 Files selected for processing (6)
.tekton/integration-tests/integration-test-scenarios.yaml.tekton/integration-tests/pipelines/agentic-operator-e2e-pipeline.yamlMakefiletest/e2e/execution_test.gotest/e2e/helpers_test.gotest/e2e/verification_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/lightspeed-agentic-sandbox(manual)
|
[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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/helpers_test.go`:
- Around line 118-127: The deleteBarePod helper defaults OPERATOR_NAMESPACE to
"openshift-lightspeed", which mismatches the Makefile default of "default";
update deleteBarePod (and its OPERATOR_NAMESPACE fallback logic) to use the same
default used when running locally—either change the fallback to "default" or,
better, use the existing test namespace variable (testNS) used elsewhere in
tests so cleanup targets the same namespace the operator creates bare pods in;
adjust only the fallback assignment inside deleteBarePod to reference testNS (or
"default") so deletion succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5811c707-782d-406d-bc3d-609591c934a6
📒 Files selected for processing (6)
.tekton/integration-tests/integration-test-scenarios.yaml.tekton/integration-tests/pipelines/agentic-operator-e2e-pipeline.yamlMakefiletest/e2e/execution_test.gotest/e2e/helpers_test.gotest/e2e/verification_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/lightspeed-agentic-sandbox(manual)
✅ Files skipped from review due to trivial changes (1)
- .tekton/integration-tests/integration-test-scenarios.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e/execution_test.go
- .tekton/integration-tests/pipelines/agentic-operator-e2e-pipeline.yaml
| // deleteBarePod removes a bare pod by name from the operator namespace. | ||
| func deleteBarePod(t *testing.T, c client.Client, name string) { | ||
| t.Helper() | ||
| operatorNS := os.Getenv("OPERATOR_NAMESPACE") | ||
| if operatorNS == "" { | ||
| operatorNS = "openshift-lightspeed" | ||
| } | ||
| pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: operatorNS}} | ||
| _ = c.Delete(context.Background(), pod) | ||
| } |
There was a problem hiding this comment.
OPERATOR_NAMESPACE default mismatch across files.
deleteBarePod defaults OPERATOR_NAMESPACE to "openshift-lightspeed" (line 123), but Makefile line 50 defaults it to "default". When running the operator locally via make run without explicitly setting OPERATOR_NAMESPACE, the operator creates bare pods in namespace "default", but this cleanup helper will attempt to delete them from "openshift-lightspeed", causing cleanup to fail and leaving orphaned pods.
Align the defaults or document that OPERATOR_NAMESPACE must be set consistently when running local e2e flows.
🔧 Suggested fix
Change the default to match Makefile, or use testNS as the default since bare pods are proposal-scoped:
func deleteBarePod(t *testing.T, c client.Client, name string) {
t.Helper()
operatorNS := os.Getenv("OPERATOR_NAMESPACE")
if operatorNS == "" {
- operatorNS = "openshift-lightspeed"
+ operatorNS = "default" // Match Makefile default
}
pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: operatorNS}}
_ = c.Delete(context.Background(), pod)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // deleteBarePod removes a bare pod by name from the operator namespace. | |
| func deleteBarePod(t *testing.T, c client.Client, name string) { | |
| t.Helper() | |
| operatorNS := os.Getenv("OPERATOR_NAMESPACE") | |
| if operatorNS == "" { | |
| operatorNS = "openshift-lightspeed" | |
| } | |
| pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: operatorNS}} | |
| _ = c.Delete(context.Background(), pod) | |
| } | |
| // deleteBarePod removes a bare pod by name from the operator namespace. | |
| func deleteBarePod(t *testing.T, c client.Client, name string) { | |
| t.Helper() | |
| operatorNS := os.Getenv("OPERATOR_NAMESPACE") | |
| if operatorNS == "" { | |
| operatorNS = "default" // Match Makefile default | |
| } | |
| pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: operatorNS}} | |
| _ = c.Delete(context.Background(), pod) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/helpers_test.go` around lines 118 - 127, The deleteBarePod helper
defaults OPERATOR_NAMESPACE to "openshift-lightspeed", which mismatches the
Makefile default of "default"; update deleteBarePod (and its OPERATOR_NAMESPACE
fallback logic) to use the same default used when running locally—either change
the fallback to "default" or, better, use the existing test namespace variable
(testNS) used elsewhere in tests so cleanup targets the same namespace the
operator creates bare pods in; adjust only the fallback assignment inside
deleteBarePod to reference testNS (or "default") so deletion succeeds.
openshift-ci
Bot
added
the
needs-rebase
openshift-ci
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Makefile (1)
160-163: 💤 Low valueSed delimiter collision risk if
SANDBOX_IMAGEcontains|.The sed command uses
|as a delimiter. IfSANDBOX_IMAGEever contains a literal|(unlikely for image refs, but possible), the substitution will fail silently or produce malformed YAML.Consider escaping or using a less common delimiter if this needs to be more robust.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` around lines 160 - 163, The sed substitution in the Makefile uses | as the delimiter and can break if $(SANDBOX_IMAGE) contains |; update the sed invocation to use a less-common delimiter (for example @) for all three substitutions so variables OPERATOR_NAMESPACE, SANDBOX_MODE and SANDBOX_IMAGE are substituted safely (e.g. replace -e 's|__OPERATOR_NAMESPACE__|$(OPERATOR_NAMESPACE)|g' with -e 's@__OPERATOR_NAMESPACE__@$(OPERATOR_NAMESPACE)`@g`' and likewise for the other two) or alternatively switch to a quoting/escaping approach (perl -pe or awk) that properly handles arbitrary characters in $(SANDBOX_IMAGE).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Makefile`:
- Around line 160-163: The sed substitution in the Makefile uses | as the
delimiter and can break if $(SANDBOX_IMAGE) contains |; update the sed
invocation to use a less-common delimiter (for example @) for all three
substitutions so variables OPERATOR_NAMESPACE, SANDBOX_MODE and SANDBOX_IMAGE
are substituted safely (e.g. replace -e
's|__OPERATOR_NAMESPACE__|$(OPERATOR_NAMESPACE)|g' with -e
's@__OPERATOR_NAMESPACE__@$(OPERATOR_NAMESPACE)`@g`' and likewise for the other
two) or alternatively switch to a quoting/escaping approach (perl -pe or awk)
that properly handles arbitrary characters in $(SANDBOX_IMAGE).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c7f9cca9-fe2c-4d0b-b4f6-80684f074eda
📒 Files selected for processing (10)
.tekton/integration-tests/integration-test-scenarios.yaml.tekton/integration-tests/pipelines/agentic-operator-e2e-pipeline.yaml.tekton/integration-tests/scripts/install-operator.shMakefileconfig/manager/manager.yamltest/e2e/analysis_execution_test.gotest/e2e/denial_test.gotest/e2e/execution_test.gotest/e2e/helpers_test.gotest/e2e/verification_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/lightspeed-agentic-sandbox(manual)
✅ Files skipped from review due to trivial changes (1)
- test/e2e/analysis_execution_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- test/e2e/verification_test.go
- .tekton/integration-tests/integration-test-scenarios.yaml
- test/e2e/execution_test.go
- test/e2e/helpers_test.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/e2e/helpers_test.go (1)
118-127:⚠️ Potential issue | 🟡 MinorOPERATOR_NAMESPACE default mismatch across files.
deleteBarePoddefaultsOPERATOR_NAMESPACEto"openshift-lightspeed"(line 123), butMakefileline 50 defaults it to"default". When running the operator locally viamake runwithout explicitly settingOPERATOR_NAMESPACE, the operator creates bare pods in namespace"default", but this cleanup helper attempts to delete them from"openshift-lightspeed", causing cleanup to fail and leaving orphaned pods.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/helpers_test.go` around lines 118 - 127, The deleteBarePod helper uses OPERATOR_NAMESPACE default "openshift-lightspeed" which mismatches the Makefile default "default"; update the deleteBarePod function (and any similar test helpers) to use the same default namespace as the Makefile (e.g., default to "default" or read from a shared constant/env helper) so the cleanup targets the same namespace the operator creates pods in; specifically, change the fallback value in deleteBarePod (function name: deleteBarePod, variable: operatorNS / OPERATOR_NAMESPACE) to match the Makefile's default or centralize the namespace default into a shared symbol and reference it from the test helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@test/e2e/helpers_test.go`:
- Around line 118-127: The deleteBarePod helper uses OPERATOR_NAMESPACE default
"openshift-lightspeed" which mismatches the Makefile default "default"; update
the deleteBarePod function (and any similar test helpers) to use the same
default namespace as the Makefile (e.g., default to "default" or read from a
shared constant/env helper) so the cleanup targets the same namespace the
operator creates pods in; specifically, change the fallback value in
deleteBarePod (function name: deleteBarePod, variable: operatorNS /
OPERATOR_NAMESPACE) to match the Makefile's default or centralize the namespace
default into a shared symbol and reference it from the test helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3a7ed3bc-07ba-4352-bc5b-997af8607f37
📒 Files selected for processing (10)
.tekton/integration-tests/integration-test-scenarios.yaml.tekton/integration-tests/pipelines/agentic-operator-e2e-pipeline.yaml.tekton/integration-tests/scripts/install-operator.shMakefileconfig/manager/manager.yamltest/e2e/analysis_execution_test.gotest/e2e/denial_test.gotest/e2e/execution_test.gotest/e2e/helpers_test.gotest/e2e/verification_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/lightspeed-agentic-sandbox(manual)
🚧 Files skipped from review as they are similar to previous changes (8)
- test/e2e/denial_test.go
- .tekton/integration-tests/integration-test-scenarios.yaml
- config/manager/manager.yaml
- .tekton/integration-tests/scripts/install-operator.sh
- test/e2e/analysis_execution_test.go
- test/e2e/execution_test.go
- test/e2e/verification_test.go
- .tekton/integration-tests/pipelines/agentic-operator-e2e-pipeline.yaml
|
@blublinsky: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
|
/retest |
3 participants
Summary
Add a Konflux integration test pipeline that runs e2e tests against an ephemeral Hypershift cluster on every operator image build. Also improve the e2e tests for bare-pod mode and per-proposal SA verification.
Konflux pipeline
Pipeline deploys the freshly-built operator image from SNAPSHOT with a fixed mock agent (quay.io/openshift-lightspeed/ols-qe:lightspeed-mock-agent). No OLM bundle, no real LLM secrets — tests are self-contained.
Design decisions
Fixed mock agent image: Not built from SNAPSHOT — it's a stable test fixture that changes rarely. Avoids coupling two component builds.
No auto-approval for verification: Each test explicitly approves only the steps it needs. Prevents the execution test from accidentally spawning a verification pod (wasting 60s+ per run).
TEST_NAMESPACE env var: Allows the same tests to run in any namespace — openshift-lightspeed in Konflux/production, overridable for local dev.
validate-agent-sandbox (check, not install): The Sandbox operator install method is transitioning (raw manifests → OLM). The Makefile checks CRDs exist but doesn't install them — that's the user's/OLM's responsibility.