Optional follow-up: direct commit to main for auto bump#1609
Optional follow-up: direct commit to main for auto bump#1609benceruleanlu wants to merge 1 commit intoben/auto-bump-comfyui-releasefrom
Conversation
dosubot
bot
added
the
size:M
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing touches🧪 Generate unit tests (beta)
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 |
christian-byrne
left a comment
christian-byrne
left a comment
There was a problem hiding this comment.
Is there no value in seeing test results on the PR and making human review the checks before merging? That process has never caught something in the past that it wouldn't have if we committed directly to main?
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eceef8945b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fi | ||
|
|
||
| git commit -m "Bump ComfyUI to v${TARGET_VERSION}" | ||
| git push origin HEAD:main |
There was a problem hiding this comment.
git push origin HEAD:main at the end of this long workflow can fail whenever another commit lands on main after the initial checkout, because this is a non-forced push and git push -h documents --force as the flag for non-fast-forward updates. In that case the bump job aborts with a rejected push and the release update is missed until someone reruns it; adding a fetch/rebase (or a retry path on non-fast-forward) would make the automation resilient.
Useful? React with 👍 / 👎.
I don't think it's ever caught anything, except for the manual testing that follows. So then maybe we should keep it as a PR and autorun the test build specifically for this case? |
There was a problem hiding this comment.
Pull request overview
This PR converts the automated ComfyUI version bump workflow from a pull request-based approach to direct commits to the main branch. The workflow is triggered by repository dispatch events (when ComfyUI releases a new version) or manual workflow dispatch, and it updates the ComfyUI version in package.json, regenerates the core requirements patch, and recompiles pre-shipped Python requirements.
Changes:
- Removed pull request creation mechanism in favor of direct commits to main
- Removed
pull-requests: writepermission (onlycontents: writeis needed) - Added explicit
ref: mainto checkout step to ensure working on the main branch
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: Commit and push bump to main | ||
| if: steps.version.outputs.should_update == 'true' | ||
| uses: peter-evans/create-pull-request@v6 | ||
| with: | ||
| token: ${{ secrets.GH_APP_TOKEN }} | ||
| branch: automated/bump-comfyui-v${{ steps.version.outputs.target_version }} | ||
| delete-branch: true | ||
| commit-message: Bump ComfyUI to v${{ steps.version.outputs.target_version }} | ||
| title: Bump ComfyUI to v${{ steps.version.outputs.target_version }} | ||
| body: | | ||
| ## Summary | ||
| - bump `config.comfyUI.version` from `${{ steps.version.outputs.current_version }}` to `${{ steps.version.outputs.target_version }}` | ||
| - regenerate `scripts/core-requirements.patch` from upstream ComfyUI requirements | ||
| - recompile pre-shipped requirements in `assets/requirements/*.compiled` using `uv pip compile` commands stored in file headers | ||
|
|
||
| ## Upstream Release | ||
| - ${{ steps.version.outputs.target_release_url }} | ||
|
|
||
| ## Testing | ||
| - workflow regenerated compiled requirements and patch artifacts | ||
| labels: dependencies | ||
| add-paths: | | ||
| package.json | ||
| scripts/core-requirements.patch | ||
| env: | ||
| TARGET_VERSION: ${{ steps.version.outputs.target_version }} | ||
| run: | | ||
| set -euo pipefail | ||
|
|
||
| git config user.name "github-actions[bot]" | ||
| git config user.email "41898282+github-actions[bot]@users.noreply.github.com" | ||
|
|
||
| git add \ | ||
| package.json \ | ||
| scripts/core-requirements.patch \ | ||
| assets/requirements/*.compiled | ||
|
|
||
| if git diff --cached --quiet; then | ||
| echo "No bump changes to commit after regeneration; skipping push." | ||
| exit 0 | ||
| fi | ||
|
|
||
| git commit -m "Bump ComfyUI to v${TARGET_VERSION}" | ||
| git push origin HEAD:main |
There was a problem hiding this comment.
The workflow now pushes directly to main without CI validation. The workflow is triggered on repository_dispatch and workflow_dispatch events, both of which are manual triggers. However, direct pushes to main bypass any branch protection rules that may be configured, and also bypass the CI/CD pipeline defined in ci.yaml which runs linting, type checking, unit tests, and formatting checks.
Consider whether this is the desired behavior. If branch protection is enabled on main (requiring status checks to pass), this push may fail. If branch protection is not enabled, the workflow will succeed but will bypass all automated quality checks that would normally run on a pull request.
| git commit -m "Bump ComfyUI to v${TARGET_VERSION}" | ||
| git push origin HEAD:main |
There was a problem hiding this comment.
The workflow previously used 'secrets.GH_APP_TOKEN' for creating pull requests, but now relies on the implicit GITHUB_TOKEN for pushing to main. Verify that:
- The default GITHUB_TOKEN has sufficient permissions to push directly to main (it should with 'contents: write')
- If branch protection rules are enabled on main requiring status checks or reviews, the default GITHUB_TOKEN cannot bypass them and the push will fail
- If you need to bypass branch protection, you'll need to either:
- Use a GitHub App token or PAT with admin privileges
- Configure branch protection to allow this specific workflow to bypass restrictions
- Remove branch protection requirements for automated version bumps
The previous approach using pull requests had the advantage of going through the normal CI/CD pipeline and review process.
| echo "No bump changes to commit after regeneration; skipping push." | ||
| exit 0 | ||
| fi | ||
|
|
There was a problem hiding this comment.
The git push is performed immediately after committing, without pulling the latest changes from the remote main branch first. While the concurrency group prevents parallel runs of this workflow, it doesn't prevent other commits from being pushed to main between the checkout (line 25-28) and the push (line 189).
Consider adding 'git pull --rebase origin main' before the push, or using 'git push --force-with-lease origin HEAD:main' instead of 'git push origin HEAD:main'. The '--force-with-lease' option is safer as it will fail if the remote has diverged since checkout, preventing accidental overwriting of changes made through other means (manual commits, other workflows, web UI, etc.).
| git pull --rebase origin main |
2 participants
optional
┆Issue is synchronized with this Notion page by Unito