-
Notifications
You must be signed in to change notification settings - Fork 30
feat: update existing branch with the new SHA #398
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
@peterjgrainger can you review it please? thx! |
76a76a5 to
e4b9e8c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances the branch creation action to support idempotent workflows by implementing fast-forward updates for existing branches. When a branch already exists, the action now updates it to the requested SHA instead of failing, allowing repeated workflow runs to advance the branch with new commits.
Key Changes:
- Switched from
repos.getBranchtogit.getReffor checking branch existence, followed bygit.updateReffor fast-forward updates when the branch exists - Added comprehensive debug logging for troubleshooting branch operations
- Updated documentation and action metadata to reflect the new update behavior
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/create-branch.ts | Implements the new branch update logic using getRef/updateRef APIs with fallback to createRef for new branches |
| dist/index.js | Compiled JavaScript output reflecting the TypeScript source changes |
| action.yml | Updated input and output descriptions to clarify the update behavior |
| tests/create-branch.test.ts | Added test case for the new fast-forward update scenario and updated existing test mocks |
| README.md | Enhanced documentation explaining the fast-forward update behavior and corrected spelling errors |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
|
@Yermanaco. Thanks for the contribution. I'll have to take a look to make sure it doesn't break existing functionality, but I think it's a good idea! |
|
@Yermanaco I've been through this again and it changes the functionality instead of creating a branch it now creates or updates which might be confusing as the name is create branch. Could you show me a workflow where you get the idempotency problem please? |
Thanks for the feedback! I cannot share any workflow due to is an internal private workflow but let me clarify the use case. The ProblemCurrent behavior blocks idempotent workflows:
Real scenario in our CI/CD: We have workflows that:
What happens when a workflow fails mid-execution: - name: Bump version
run: npm version patch # Creates new commit + tag on release-XY
- name: Create cert branch
uses: peterjgrainger/[email protected]
with:
branch: ver-BrandA-24.29
sha: ${{ steps.get-sha.outputs.sha }} # Points to new bump commit
- name: Deploy # ❌ This step fails
# Re-run the workflow:
# - release-XY has the new bumped commit
# - But ver-BrandA-24.29 still points to the OLD commit (before bump)
# - Action skips because branch exists → STALE SHA ❌Another use case
Behavior after this change:
This is 100% backward compatible:
Name Still Makes SenseThe action "creates" the branch state: ensures it exists at the given SHA. Similar to how actions/checkout creates/updates the local branch, or Terraform "creates" resources (even if they exist, it ensures the desired state). If you prefer, I can:
|
@peterjgrainger WDYT? Does that sound good to you? |
|
I agree this is an issue. I'm trying to figure out the best way to implement without it having unintended side effects. I'm thinking more of failing the action if a branch exists which would be clearer. My biggest concern is someone making a mistake and fast-forwarding a branch they didn't intend to. e.g.
What are your thoughts on having a new action for updating a branch e.g. From your example Either that or put all the logic in the workflow rather than the action:
|
First of all, Happy new year! Thanks for considering this! I understand your concern about unintended updates, but I think creating a separate action adds unnecessary complexity. Why a Separate Action Isn't NeededThe two-action workflow you proposed requires:
Better Solution: Optional FlagAdd an - name: Create or update cert branch
uses: peterjgrainger/[email protected]
with:
branch: ver-BrandA-24.29
sha: ${{ steps.get-sha.outputs.sha }}
allow_update: true # Optional flagSafety Is Already Built-InYour concern about accidentally updating main is valid, but these safeguards already exist:
My RecommendationI'd suggest making allow_update default to true because:
But if you prefer conservative defaults, we can set allow_update: false by default and require explicit opt-in. What I Can DoI can update the PR to add the allow_update input. Which default would you prefer?
|
Use case: This change fixes an idempotency problem in workflows that automate version bumps and commits. Previously, after the action created a branch and later runs produced new commits, the action did nothing when the branch already existed and the branch could remain behind the latest commits. With this PR the action attempts a fast‑forward update to move the existing branch to the requested SHA when possible, so repeated runs will advance the branch to include new commits.
Note: non‑fast‑forward updates are rejected by the API (no force), and branch protection or insufficient token scopes can still prevent the update