Skip to content

DNM/TEST: OTE: Improve downstream sync PR diagnostics#80164

Open
jluhrsen wants to merge 2 commits into
openshift:mainfrom
jluhrsen:improve-sync-diagnostics-no-test-validation
Open

DNM/TEST: OTE: Improve downstream sync PR diagnostics#80164
jluhrsen wants to merge 2 commits into
openshift:mainfrom
jluhrsen:improve-sync-diagnostics-no-test-validation

Conversation

@jluhrsen
Copy link
Copy Markdown
Contributor

@jluhrsen jluhrsen commented Jun 5, 2026

Enhancements to the downstream sync automation to provide better diagnostics when issues occur:

  1. Merge Conflict Reporting:

    • Capture conflicted files when merge fails
    • Add conflict details to PR description with expandable sections
    • Skip go mod tidy and test sync when conflicts exist
  2. Enhanced PR Handling:

    • Mark PRs as draft when conflicts or failures occur
    • Update PR title with failure types
    • Add /hold comment with resolution steps
    • Include all diagnostic info in PR body for easier triage
  3. JSON Escaping Fix:

    • Use jq to properly build JSON payloads for API calls
    • Prevents issues with special characters in titles/bodies

Summary by CodeRabbit

This PR updates the downstream-sync step in the OpenShift CI configuration (ci-operator step-registry for github/downstream-sync) to provide richer diagnostics and safer failure handling when syncing changes into downstream repositories.

Practical effects and affected area

  • Affects the downstream sync automation invoked by ci-operator (github/downstream-sync step). This changes how the sync step reports failures and how it interacts with Prow/GitHub for downstream PR handling.

Key changes

  • Merge conflict capture and reporting

    • When a merge fails, the script captures the list of conflicted files and embeds that list into the conflict commit message and into the downstream PR body (rendered as an expandable diagnostic section) for easier triage.
    • Sets a persisted CONFLICT=true flag so downstream steps know a conflict occurred.
    • Skips the openshift/go.mod sync and the subsequent go mod vendor / test-annotation sync when conflicts exist to avoid cascading failures.
  • PR handling and operator-visible signals

    • Marks downstream PRs as draft when conflicts or other failures are detected.
    • Updates PR titles to indicate failure type.
    • Posts a /hold comment with resolution steps and more readable newlines (fixes newline handling in the /hold comment body).
    • Includes all collected diagnostic information in the PR body so maintainers see the full context without chasing logs.
  • Robust JSON construction for API calls

    • Replaces inline-escaped JSON with jq-built JSON payloads for GitHub/Prow HTTP requests, preventing issues with special characters in titles/bodies.

Why it matters

  • Provides clearer, on-PR diagnostics that reduce manual triage time.
  • Prevents partial sync work (like go.mod/vendor/test syncs) from running after a merge conflict, reducing risk of leaving downstream repos in a broken state.
  • Fixes edge cases around JSON escaping and comment/newline formatting that previously caused malformed API payloads or unreadable comments.

Files touched (high level)

  • ci-operator/step-registry/github/downstream-sync/github-downstream-sync-commands.sh — merge conflict handling, PR body/comment construction, jq-based API payloads, and skip logic for go.mod syncs.

This change improves observability and safety of automated downstream syncs used by OpenShift CI.

Enhancements to the downstream sync automation to provide better
diagnostics when issues occur:

1. Merge Conflict Reporting:
   - Capture conflicted files when merge fails
   - Add conflict details to PR description with expandable sections
   - Skip go mod tidy and test sync when conflicts exist

2. Enhanced PR Handling:
   - Mark PRs as draft when conflicts or failures occur
   - Update PR title with failure types
   - Add /hold comment with resolution steps
   - Include all diagnostic info in PR body for easier triage

3. JSON Escaping Fix:
   - Use jq to properly build JSON payloads for API calls
   - Prevents issues with special characters in titles/bodies

Signed-off-by: Jamo Luhrsen <jluhrsen@gmail.com>
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@jluhrsen
Copy link
Copy Markdown
Contributor Author

jluhrsen commented Jun 5, 2026

/pj-rehearse

@openshift-merge-bot
Copy link
Copy Markdown
Contributor

@jluhrsen: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jun 5, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 021562ca-cc2b-483b-a8f4-c3104a7e0663

📥 Commits

Reviewing files that changed from the base of the PR and between d1354d0 and b7b66e9.

📒 Files selected for processing (1)
  • ci-operator/step-registry/github/downstream-sync/github-downstream-sync-commands.sh
🚧 Files skipped from review as they are similar to previous changes (1)
  • ci-operator/step-registry/github/downstream-sync/github-downstream-sync-commands.sh

Walkthrough

This PR updates the downstream-sync script to capture conflicted file names and set CONFLICT=true, embed conflicted files in the conflict commit message and PR body, skip openshift/go.mod sync when conflicts exist, and construct GitHub API request bodies using jq-generated JSON payload variables instead of inline escaped JSON.

Changes

Merge-conflict handling and GitHub API integration

Layer / File(s) Summary
Conflict detection and downstream-sync gating
ci-operator/step-registry/github/downstream-sync/github-downstream-sync-commands.sh
Collect conflicted file names via git diff --name-only --diff-filter=U, include them in the conflict commit message, set CONFLICT=true, and prevent openshift/go.mod sync (and subsequent vendor/test annotation sync) when conflicts are present.
Conflict reporting and GitHub API payload construction
ci-operator/step-registry/github/downstream-sync/github-downstream-sync-commands.sh
When CONFLICT is true, append a collapsible HTML <details> block to the PR body listing conflicted files. Refactor the PR-hold and /ok-to-test + /payload HTTP calls to build JSON request bodies using jq-generated variables (TITLE_PAYLOAD, COMMENT_PAYLOAD) before calling curl.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels: rehearsals-ack

Suggested reviewers:

  • jcaamano
  • martinkennelly

Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 error)

Check name Status Explanation Resolution
No-Sensitive-Data-In-Logs ❌ Error Script embeds GITHUB_TOKEN in git remote URL at line 169 (https://x-access-token:${GITHUB_TOKEN}@...), violating best practice of not putting tokens in URLs or logs where they could be exposed. Use git credential helper or store token in .netrc/.git-credentials instead of embedding in remote URL to prevent token exposure in logs.
✅ Passed checks (14 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title references 'DNM/TEST' (Do Not Merge/Test) prefixes and mentions improving diagnostics, which aligns with the actual changes to merge-conflict handling, PR diagnostics, and JSON payload improvements in the downstream sync script.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed PR modifies a shell script for CI/CD automation, not Go Ginkgo tests. No Ginkgo test definitions or imports found. Check not applicable.
Test Structure And Quality ✅ Passed This PR modifies a bash shell script and configuration files, not Ginkgo test code. The custom check is not applicable as there are no Go test files in this PR.
Microshift Test Compatibility ✅ Passed This PR modifies only a bash script (github-downstream-sync-commands.sh) for CI/CD automation in the openshift/release repository, not Ginkgo e2e tests. No Go test files were added or modified.
Single Node Openshift (Sno) Test Compatibility ✅ Passed This PR modifies a bash CI automation script (github-downstream-sync-commands.sh) for downstream repository synchronization. No Ginkgo e2e tests are added, so SNO compatibility check does not apply.
Topology-Aware Scheduling Compatibility ✅ Passed The PR modifies a CI automation shell script for downstream repository merge handling, not deployment manifests, operators, or controllers. No Kubernetes scheduling constraints are introduced.
Ote Binary Stdout Contract ✅ Passed PR modifies only a bash shell script for CI automation. OTE Binary Stdout Contract applies to Go applications/tests, not bash scripts.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed PR contains no new Ginkgo e2e tests. Changes are infrastructure-only (shell script and YAML config), so IPv6/disconnected network test compatibility check does not apply.
No-Weak-Crypto ✅ Passed No weak crypto detected: PR only uses SHA-256 RSA signing for JWT creation, no MD5/SHA1/DES/RC4/3DES/Blowfish/ECB, no custom crypto, no unsafe token comparisons.
Container-Privileges ✅ Passed Pull request contains no container/K8s manifests with privileged escalation configs (privileged:true, hostPID/Network/IPC, SYS_ADMIN, allowPrivilegeEscalation:true, or unnecessary root access).
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@openshift-ci openshift-ci Bot requested review from arkadeepsen and pperiyasamy June 5, 2026 16:22
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Jun 5, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jluhrsen

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 5, 2026
Copy link
Copy Markdown
Contributor

@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.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
ci-operator/step-registry/github/downstream-sync/github-downstream-sync-commands.sh (1)

221-224: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Literal \n strings won't render as line breaks in the GitHub comment.

Using \n in bash string concatenation produces literal backslash-n characters. When passed to jq --arg, these become \\n in the JSON output, causing GitHub to display \n literally instead of line breaks.

This is inconsistent with lines 238-240 which correctly use $'\n' for actual newlines.

🐛 Proposed fix using actual newlines
  HOLD_REASON="/hold"
  for step in "${STEPS[@]}"; do
-   HOLD_REASON="${HOLD_REASON}\n${step}"
+   HOLD_REASON="${HOLD_REASON}"$'\n'"${step}"
  done
🤖 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
`@ci-operator/step-registry/github/downstream-sync/github-downstream-sync-commands.sh`
around lines 221 - 224, The HOLD_REASON string is being concatenated with
literal "\n" so GitHub shows backslash-n instead of real newlines; update the
concatenation in the for loop that builds HOLD_REASON (the variable and loop
iterating over STEPS) to use an actual newline (e.g., $'\n' or printf with a
newline) when appending each step so the value passed to jq/--arg contains real
newline characters like the later block at lines 238-240.
🤖 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.

Outside diff comments:
In
`@ci-operator/step-registry/github/downstream-sync/github-downstream-sync-commands.sh`:
- Around line 221-224: The HOLD_REASON string is being concatenated with literal
"\n" so GitHub shows backslash-n instead of real newlines; update the
concatenation in the for loop that builds HOLD_REASON (the variable and loop
iterating over STEPS) to use an actual newline (e.g., $'\n' or printf with a
newline) when appending each step so the value passed to jq/--arg contains real
newline characters like the later block at lines 238-240.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: dcd76fc4-f5df-43ee-9830-8e4214cd479f

📥 Commits

Reviewing files that changed from the base of the PR and between a596c64 and d1354d0.

📒 Files selected for processing (1)
  • ci-operator/step-registry/github/downstream-sync/github-downstream-sync-commands.sh

Use bash $'\n' syntax to create actual newlines instead of literal \n
characters in the PR hold comment body.

Signed-off-by: Jamo Luhrsen <jluhrsen@gmail.com>
@jluhrsen
Copy link
Copy Markdown
Contributor Author

jluhrsen commented Jun 5, 2026

/pj-rehearse

@openshift-merge-bot
Copy link
Copy Markdown
Contributor

@jluhrsen: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel.

@openshift-merge-bot
Copy link
Copy Markdown
Contributor

[REHEARSALNOTIFIER]
@jluhrsen: the pj-rehearse plugin accommodates running rehearsal tests for the changes in this PR. Expand 'Interacting with pj-rehearse' for usage details. The following rehearsable tests have been affected by this change:

Test name Repo Type Reason
periodic-ci-openshift-ovn-kubernetes-release-5.0-periodics-downstream-merge N/A periodic Registry content changed
Interacting with pj-rehearse

Comment: /pj-rehearse to run up to 5 rehearsals
Comment: /pj-rehearse skip to opt-out of rehearsals
Comment: /pj-rehearse {test-name}, with each test separated by a space, to run one or more specific rehearsals
Comment: /pj-rehearse more to run up to 10 rehearsals
Comment: /pj-rehearse max to run up to 25 rehearsals
Comment: /pj-rehearse auto-ack to run up to 5 rehearsals, and add the rehearsals-ack label on success
Comment: /pj-rehearse list to get an up-to-date list of affected jobs
Comment: /pj-rehearse abort to abort all active rehearsals
Comment: /pj-rehearse network-access-allowed to allow rehearsals of tests that have the restrict_network_access field set to false. This must be executed by an openshift org member who is not the PR author

Once you are satisfied with the results of the rehearsals, comment: /pj-rehearse ack to unblock merge. When the rehearsals-ack label is present on your PR, merge will no longer be blocked by rehearsals.
If you would like the rehearsals-ack label removed, comment: /pj-rehearse reject to re-block merging.

@jluhrsen
Copy link
Copy Markdown
Contributor Author

jluhrsen commented Jun 5, 2026

/pj-rehearse

@openshift-merge-bot
Copy link
Copy Markdown
Contributor

@jluhrsen: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel.

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Jun 5, 2026

@jluhrsen: The following test 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/rehearse/periodic-ci-openshift-ovn-kubernetes-release-5.0-periodics-downstream-merge b7b66e9 link unknown /pj-rehearse periodic-ci-openshift-ovn-kubernetes-release-5.0-periodics-downstream-merge

Full PR test history. Your PR dashboard.

Details

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant