Skip to content

fix(ci): Make sure to checkout the PR branch before pushing the bundle diff generated - #1599

Merged
rm3l merged 1 commit into
redhat-developer:mainfrom
rm3l:fix/fix_detached_state_when_pushing_bundle_diff
Sep 5, 2025
Merged

fix(ci): Make sure to checkout the PR branch before pushing the bundle diff generated#1599
rm3l merged 1 commit into
redhat-developer:mainfrom
rm3l:fix/fix_detached_state_when_pushing_bundle_diff

Conversation

@rm3l

@rm3l rm3l commented Sep 5, 2025

Copy link
Copy Markdown
Member

Description

Should hopefully fix https://github.com/redhat-developer/rhdh-operator/actions/runs/17475391158/job/49670949036?pr=1584

This only affects workflows that have steps that auto-commit/push certain changes to the PR branch.

Which issue(s) does this PR fix or relate to

Some failures on PR checks: https://github.com/redhat-developer/rhdh-operator/actions/runs/17475391158/job/49670949036?pr=1584

PR acceptance criteria

  • Tests
  • Documentation

How to test changes / Special notes to the reviewer

Summary by Sourcery

CI:

  • Configure and fetch the PR fork, checkout its head branch, and push bundle/installer manifest updates back to the PR

@openshift-ci

openshift-ci Bot commented Sep 5, 2025

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign rm3l for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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

@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Remote Exists

Adding the 'fork' remote without checking if it already exists may fail on re-runs. Consider guarding with 'git remote get-url fork || git remote add ...' or using '|| true'.

git remote add fork "https://github.com/${{ github.event.pull_request.head.repo.full_name }}.git"
git fetch fork ${{ github.event.pull_request.head.ref }}
Shallow Fetch

If the checkout action used a shallow clone, fetching without '--depth' may still be shallow; confirm required history depth or use 'git fetch --depth=1 fork ' for speed and consistency.

git fetch fork ${{ github.event.pull_request.head.ref }}
git checkout -B pr-branch fork/${{ github.event.pull_request.head.ref }}
Branch Name

Using a fixed local branch name 'pr-branch' can clash across matrix jobs or re-runs; consider a unique name (e.g., include GITHUB_RUN_ID) or use 'git switch -C' with a temp name.

git checkout -B pr-branch fork/${{ github.event.pull_request.head.ref }}

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey there - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Authenticate remote for CI push

Pushing to a new HTTPS remote will fail without authentication. Use the workflow
token in the remote URL so the push succeeds in CI. Keep the rest of the flow
unchanged.

.github/workflows/pr-bundle-diff-checks.yaml [66-78]

-git remote add fork "https://github.com/${{ github.event.pull_request.head.repo.full_name }}.git"
-git fetch fork ${{ github.event.pull_request.head.ref }}
-git checkout -B pr-branch fork/${{ github.event.pull_request.head.ref }}
+git remote add fork "https://x-access-token:${{ secrets.GITHUB_TOKEN }}@github.com/${{ github.event.pull_request.head.repo.full_name }}.git"
+git fetch fork "${{ github.event.pull_request.head.ref }}"
+git checkout -B pr-branch "fork/${{ github.event.pull_request.head.ref }}"
 
 git config user.name 'github-actions[bot]'
 git config user.email 'github-actions[bot]@users.noreply.github.com'
 
 git add -A .
 git commit \
   -m "Regenerate bundle/installer manifests" \
   -m "Co-authored-by: $GITHUB_ACTOR <$GITHUB_ACTOR@users.noreply.github.com>"
 
-git push fork pr-branch:${{ github.event.pull_request.head.ref }}
+git push fork pr-branch:"${{ github.event.pull_request.head.ref }}"
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies that pushing to an HTTPS remote requires authentication, which is missing in the PR. This is a critical fix, as the git push command would otherwise fail.

High
Skip push on forked PRs

The workflow token cannot push to contributor forks. Guard the push so it only
runs for same-repo PRs, preventing hard failures on external fork PRs.

.github/workflows/pr-bundle-diff-checks.yaml [78]

-git push fork pr-branch:${{ github.event.pull_request.head.ref }}
+if [ "${{ github.event.pull_request.head.repo.full_name }}" = "${{ github.repository }}" ]; then
+  git push fork pr-branch:"${{ github.event.pull_request.head.ref }}"
+else
+  echo "Skipping push: workflow token cannot write to contributor forks."
+fi
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly points out that the default GITHUB_TOKEN has read-only permissions for PRs from forks, which would cause the push to fail. The proposed guard makes the workflow robust for contributions from forks.

High
Security
Quote dynamic variables

Quote dynamic inputs to prevent word-splitting or accidental command injection
via crafted branch names. This hardens the script and avoids subtle failures.

.github/workflows/pr-bundle-diff-checks.yaml [67-78]

-git fetch fork ${{ github.event.pull_request.head.ref }}
-git checkout -B pr-branch fork/${{ github.event.pull_request.head.ref }}
-git push fork pr-branch:${{ github.event.pull_request.head.ref }}
+git fetch fork "${{ github.event.pull_request.head.ref }}"
+git checkout -B pr-branch "fork/${{ github.event.pull_request.head.ref }}"
+git push fork pr-branch:"${{ github.event.pull_request.head.ref }}"
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly recommends quoting variables to prevent word-splitting and potential injection if branch names contain special characters. This is a good practice that improves the script's robustness and security.

Low
  • More

@rm3l
rm3l merged commit a329c84 into redhat-developer:main Sep 5, 2025
6 of 7 checks passed
@rm3l
rm3l deleted the fix/fix_detached_state_when_pushing_bundle_diff branch September 5, 2025 16:48
@rm3l

rm3l commented Sep 5, 2025

Copy link
Copy Markdown
Member Author

/cherry-pick release-1.6 release-1.7

@openshift-cherrypick-robot

Copy link
Copy Markdown

@rm3l: new pull request created: #1603

Details

In response to this:

/cherry-pick release-1.6 release-1.7

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.

@rm3l

rm3l commented Sep 5, 2025

Copy link
Copy Markdown
Member Author

/cherry-pick release-1.7

@openshift-cherrypick-robot

Copy link
Copy Markdown

@rm3l: new pull request created: #1604

Details

In response to this:

/cherry-pick release-1.7

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.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants