Skip to content

Conversation

@weshayutin
Copy link
Contributor

Why the changes were made

testing out changes to ci params

@coderabbitai
Copy link

coderabbitai bot commented Nov 6, 2025

Walkthrough

Makefile: adds public variable FAIL_FAST (default true), changes default OPENSHIFT_CI to false, disables FAIL_FAST when OPENSHIFT_CI is true, and appends --fail-fast=$(FAIL_FAST) into GINKGO_FLAGS. tests/e2e/lib/k8s_common_helpers.go: imports ptr and deletes PVCs with DeleteOptions{GracePeriodSeconds: ptr.To(int64(0))}.

Changes

Cohort / File(s) Summary
Test execution / CI flags
Makefile
Adds public FAIL_FAST (default true); sets OPENSHIFT_CI default to false; when OPENSHIFT_CI is true overrides FAIL_FAST=false; injects --fail-fast=$(FAIL_FAST) into GINKGO_FLAGS; includes OpenShift CI-specific conditional.
PVC cleanup logic
tests/e2e/lib/k8s_common_helpers.go
Adds import k8s.io/utils/pointer (ptr); changes DeleteAllPVCsInNamespace to call Delete with DeleteOptions{GracePeriodSeconds: ptr.To(int64(0))} for immediate PVC deletion.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Check Makefile: conditional override of FAIL_FAST when OPENSHIFT_CI=true, and that GINKGO_FLAGS string interpolation is correct.
  • Check Go change: ensure ptr.To(int64(0)) import path/name matches project conventions and immediate deletion is acceptable for test cleanup.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 716d231 and 8c8f29d.

📒 Files selected for processing (1)
  • Makefile (3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

  • Makefile
🔇 Additional comments (4)
Makefile (4)

787-787: LGTM – OPENSHIFT_CI default now false.

Changing the default from true to false is appropriate; the variable will be set to true in OpenShift CI environments via pipeline configuration.


819-819: LGTM – FAIL_FAST public variable added with sensible default.

Introducing FAIL_FAST with default true allows fail-on-first semantics in local development, while the conditional override (lines 850–852) disables it in OpenShift CI to report all test failures per the PR objective.


849-852: ✓ Previous critical issue resolved – Makefile comment syntax now correct.

Line 849 now correctly uses # (Makefile comment syntax) instead of the previously flagged // (invalid). The conditional override of FAIL_FAST is properly sequenced after the default definition on line 819, ensuring Make evaluates assignments in the correct order.


853-858: LGTM – FAIL_FAST correctly propagated into GINKGO_FLAGS.

The --fail-fast=$(FAIL_FAST) flag (line 857) is properly inserted into the test configuration. In OpenShift CI, FAIL_FAST will be false, allowing all tests to run and report results, while in local development it defaults to true for faster feedback.


Comment @coderabbitai help to get the list of available commands and usage tips.

@kaovilai
Copy link
Member

kaovilai commented Nov 7, 2025

/retest

@kaovilai
Copy link
Member

kaovilai commented Nov 7, 2025

/retest

ai-retester: The e2e-test-cli-aws-e2e step failed due to the test suite failing. Specifically, the "MySQL application two Vol CSI via CLI" test case failed, indicating a problem with the backup and restore functionality related to CSI volumes and the OADP CLI. The error message indicates inability to connect the deployed application via Route.

The e2e-test-aws-e2e step failed because the Ginkgo test suite timed out after an hour, with two tests failing or timing out. The test container exited with code 2.

@kaovilai
Copy link
Member

kaovilai commented Nov 7, 2025

/retest

ai-retester: The e2e-test-aws test failed because the "MySQL application two Vol CSI " test failed. The errors indicate a failure to connect to the "todolist" service via the external route and via the proxy pod's curl command. Specifically, it could not connect to the server running on port 8000 "PodInitializing".

@kaovilai
Copy link
Member

kaovilai commented Nov 8, 2025

/retest

ai-retester: The "e2e-test-aws" step timed out, and the "MySQL application DATAMOVER" test within it failed. It also seems like there maybe be some other test and deployment related issues within that step.

The e2e tests failed. Specifically, the "MySQL application KOPIA" test timed out after 540 seconds when a container in a MySQL pod repeatedly failed its ready check, likely preventing the tests from completing.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 778e057 and 716d231.

📒 Files selected for processing (1)
  • Makefile (3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

  • Makefile
🔇 Additional comments (2)
Makefile (2)

787-857: Logic for conditional FAIL_FAST override is sound.

The approach—setting FAIL_FAST ?= true as a public parameter, then overriding it to false when OPENSHIFT_CI=true—aligns well with the PR objective to disable fail-fast in CI runs while allowing local/manual overrides. The use of --fail-fast=$(FAIL_FAST) in GINKGO_FLAGS correctly propagates the decision to Ginkgo. Once the comment syntax is fixed, this change should achieve the intended behavior: faster test feedback locally, comprehensive test results in CI.


787-857: Verify related PVC deletion changes are present and consistent.

The PR objective mentions "set 0 timeout for pvc delete," but the provided code only shows the Makefile changes for fail-fast. Confirm that corresponding changes to tests/e2e/lib/k8s_common_helpers.go (as mentioned in the AI summary) are included in this PR and working as intended.

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 18, 2025
@kaovilai
Copy link
Member

ERRO[2025-11-17T23:15:56Z] Timed out waiting for RBAC to initialize in the test namespace. 

unrelated

/retest

@openshift-ci
Copy link

openshift-ci bot commented Nov 18, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kaovilai, shubham-pampattiwar, weshayutin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [kaovilai,shubham-pampattiwar]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@weshayutin
Copy link
Contributor Author

/LGTM

@openshift-ci
Copy link

openshift-ci bot commented Nov 21, 2025

@weshayutin: you cannot LGTM your own PR.

In response to this:

/LGTM

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.

@weshayutin weshayutin added the lgtm Indicates that a PR is ready to be merged. label Nov 21, 2025
@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 5d074fc and 2 for PR HEAD 8c8f29d in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 0e62027 and 1 for PR HEAD 8c8f29d in total

@openshift-ci
Copy link

openshift-ci bot commented Nov 21, 2025

@weshayutin: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/4.19-e2e-test-aws 778e057 link true /test 4.19-e2e-test-aws
ci/prow/4.19-e2e-test-kubevirt-aws 778e057 link true /test 4.19-e2e-test-kubevirt-aws
ci/prow/4.20-e2e-test-aws 8c8f29d link true /test 4.20-e2e-test-aws

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants