fix: ensure at least one nested pipeline succeeds for main pipeline s… #1236
Conversation
WalkthroughRemoves the dedicated SOME_PIPELINE_FAILED exit branch and adds a final fallback: if no nested pipelines succeeded (and none triggered the early success), the main pipeline prints a message and exits with status 1; the early exit on success (exit 0) is unchanged. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Trigger
participant Main as Main Pipeline
participant Nested as Nested Pipelines
Dev->>Main: Start e2e main pipeline
Main->>Nested: Dispatch nested pipelines
Note over Nested,Main: Collect results (succeeded flags per nested pipeline)
alt Any succeeded
Main-->>Dev: Exit 0 (success)
else No successes detected
Note right of Main: New fallback path (no SOME_PIPELINE_FAILED check)
Main-->>Dev: Print message and Exit 1
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches🧪 Generate unit tests
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: 0
🧹 Nitpick comments (1)
integration-tests/pipelines/e2e-main-pipeline.yaml (1)
216-224: Confirm success-on-any-success policy (success wins over failures).Current ordering returns exit 0 if any nested pipeline succeeded, even when some failed. If that’s intended, ignore. If you want “fail on any failure unless all succeeded” or to fail on mixed outcomes, consider:
- if $SOME_PIPELINE_SUCCEEDED ; then - exit 0 - fi - if $SOME_PIPELINE_FAILED ; then - exit 1 - fi - # If no pipelines succeeded (including case where none started), fail the pipeline - echo "No nested pipelines succeeded. Main pipeline must fail." - exit 1 + # Pass only if at least one succeeded and none failed + if $SOME_PIPELINE_SUCCEEDED && ! $SOME_PIPELINE_FAILED ; then + exit 0 + fi + # Optionally, make mixed outcomes fail (uncomment echo for clarity) + if $SOME_PIPELINE_FAILED ; then + echo "At least one nested pipeline failed." + exit 1 + fi + # No successes (including none started) -> fail + echo "No nested pipelines succeeded. Main pipeline must fail." + exit 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
integration-tests/pipelines/e2e-main-pipeline.yaml(1 hunks)
🔇 Additional comments (1)
integration-tests/pipelines/e2e-main-pipeline.yaml (1)
222-224: Sane fallback: main pipeline now fails when zero nested pipelines succeed.This closes the gap where no PLRs started or none reached a terminal state of True/False. Looks good.
rhopp
force-pushed
the
fix/nested-pipeline-success-condition
branch
from
4f5da04 to
df59449
Compare
…uccess 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
rhopp
force-pushed
the
fix/nested-pipeline-success-condition
branch
from
df59449 to
9677f80
Compare
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jsmid1, rhopp 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
integration-tests/pipelines/e2e-main-pipeline.yaml (4)
219-220: Add an explicit error message before exiting.Right now the failure path is silent (e.g., when zero pipelines start). Emit a clear message to aid triage.
- # If we reach here, no pipelines succeeded - fail the main pipeline - exit 1 + # If we reach here, no pipelines succeeded - fail the main pipeline + echo "No nested pipelines succeeded (total started: ${#PIPELINERUNS_ARRAY[@]}). Failing main pipeline." >&2 + exit 1
202-215: Rename the boolean to reflect its purpose or drop it.
SOME_PIPELINE_FAILEDis no longer part of the exit condition; it’s used only to gate printing the header. Rename to avoid confusion, or remove if not needed.- SOME_PIPELINE_FAILED=false + PRINTED_FAILED_HEADER=false SOME_PIPELINE_SUCCEEDED=false for PIPELINE_RUN in "${PIPELINERUNS_ARRAY[@]}"; do if [[ $(oc get pipelinerun/$PIPELINE_RUN -n ${KONFLUX_NAMESPACE} -o jsonpath="{.status.conditions[?(@.type==\"Succeeded\")].status}") == "False" ]]; then - if ! $SOME_PIPELINE_FAILED ; then + if ! $PRINTED_FAILED_HEADER ; then echo "List of failed PLRs:" fi echo "${KONFLUX_URL}/ns/${KONFLUX_NAMESPACE}/applications/${KONFLUX_APPLICATION_NAME}/pipelineruns/${PIPELINE_RUN}" - SOME_PIPELINE_FAILED=true + PRINTED_FAILED_HEADER=true elif [[ $(oc get pipelinerun/$PIPELINE_RUN -n ${KONFLUX_NAMESPACE} -o jsonpath="{.status.conditions[?(@.type==\"Succeeded\")].status}") == "True" ]]; then SOME_PIPELINE_SUCCEEDED=true fi done
124-171: Fail fast when no OCP versions are configured.If
OCP_VERSIONSis empty, nothing runs and the script later exits with little context. Check and fail early with a clear reason.echo "Running tests for OCP versions: ${OCP_VERSIONS[*]}" + if [[ ${#OCP_VERSIONS[@]} -eq 0 ]]; then + echo "No OCP versions found in RHADS config (OCP=\"...\"). Nothing to run. Failing." >&2 + exit 1 + fi + # Waits for condition for all started pipelines
216-218: Optional: summarize outcomes for observability.Before
exit 0, consider printing counts of succeeded/failed pipelines to make green runs auditable.if $SOME_PIPELINE_SUCCEEDED ; then - exit 0 + echo "At least one nested pipeline succeeded; main pipeline will pass." + exit 0 fi
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
integration-tests/pipelines/e2e-main-pipeline.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Red Hat Konflux / tssc-cli-on-pull-request
|
/retest |
1 similar comment
|
/retest |
openshift-merge-bot
bot
merged commit 9852260
into
redhat-appstudio:main
|
@rhopp: The following test has Failed, say /retest to rerun failed tests.
Inspecting Test ArtifactsTo inspect your test artifacts, follow these steps:
mkdir -p oras-artifacts
cd oras-artifacts
oras pull quay.io/konflux-test-storage/rhtap-team/rhtap-cli:e2e-4.18-config1-dg6jsTest results analysis<not enabled> |
3 participants
…uccess
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests