Skip to content
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

Add single-commit input #78

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

xaviergmail
Copy link

This change simply adds a configurable single-commit option to be passed to JamesIves/github-pages-deploy-action

@rossjrw
Copy link
Owner

rossjrw commented Jan 30, 2024

Thanks @xaviergmail! I assume this option works fine with force: false?

I think I'm going to tweak the docs a little bit - given that this is passed to JamesIves' action, it's a bit odd that our description of it would be more verbose than theirs. Given that I intend to add more parameters that are passed along directly (e.g. #32) I'll probably end up just adding a little list like "Parameters passed directly: single-commit, this, that, something-else". The warnings are appreciated but they're probably something that would be better contributed upstream.

@rossjrw rossjrw added the enhancement New feature or request label Jan 30, 2024
@rossjrw
Copy link
Owner

rossjrw commented Jan 30, 2024

I assume this option works fine with force: false?

Looks like the answer is possibly no - logs from testing with xaviergmail/pr-preview-action@457ac9e117d2f318d3146bb035afe506d943c6c1, with single-commit: true, after observing that there were still lots of commits on the gh-pages branch:

    Starting to commit changes…
/usr/bin/git ls-remote --heads ***github.com/rossjrw/***.git refs/heads/gh-pages
    d8dac3b154ec123bb1eee30b94a91dd4ffef5635	refs/heads/gh-pages
    Creating worktree…
/usr/bin/git fetch --no-recurse-submodules --depth=1 origin gh-pages
    From https://github.com/rossjrw/***
     * branch            gh-pages   -> FETCH_HEAD
     * [new branch]      gh-pages   -> origin/gh-pages
/usr/bin/git worktree add --no-checkout --detach github-pages-deploy-action-temp-deployment-folder
    Preparing worktree (detached HEAD bdbdf1e)
/usr/bin/git checkout --orphan gh-pages origin/gh-pages
    Previous HEAD position was bdbdf1e Merge 9d2a46fca40768ac4872994ae19a7aa87fabfd22 into cd026d128385e03c1dfda3ba96a6dc949080f4de
    Switched to a new branch 'gh-pages'
/usr/bin/chmod -R +rw /home/runner/work/***/***/dist
    Creating target folder if it doesn't already exist… 📌
/usr/bin/rsync -q -av --checksum --progress /home/runner/work/***/***/dist/. github-pages-deploy-action-temp-deployment-folder/pr-preview-v1/pr-17 --delete --exclude CNAME --exclude .nojekyll --exclude .ssh --exclude .git --exclude .github
/usr/bin/git add --all .
    Checking if there are files to commit…
/usr/bin/git add --all .
/usr/bin/git checkout -b github-pages-deploy-action/6p10kamt0
    Switched to a new branch 'github-pages-deploy-action/6p10kamt0'
/usr/bin/git commit -m Deploy preview for PR 17 🛫 --quiet --no-verify
    Pushing changes… (attempt 1 of 3)
/usr/bin/git push --porcelain ***github.com/rossjrw/***.git github-pages-deploy-action/6p10kamt0:gh-pages
    error: failed to push some refs to 'https://github.com/rossjrw/***.git'
    hint: Updates were rejected because a pushed branch tip is behind its remote
    hint: counterpart. If you want to integrate the remote changes, use 'git pull'
    hint: before pushing again.
    hint: See the 'Note about fast-forwards' in 'git push --help' for details.
    To https://github.com/rossjrw/***.git
    !	refs/heads/github-pages-deploy-action/6p10kamt0:refs/heads/gh-pages	[rejected] (non-fast-forward)
    Done
    Updates were rejected
    Fetching upstream gh-pages…
/usr/bin/git fetch ***github.com/rossjrw/***.git gh-pages:gh-pages
    From https://github.com/rossjrw/***
     * [new branch]      gh-pages   -> gh-pages
    Rebasing this deployment onto gh-pages…
/usr/bin/git rebase gh-pages github-pages-deploy-action/6p10kamt0
    warning: skipped previously applied commit 86db876
    hint: use --reapply-cherry-picks to include skipped commits
    hint: Disable this message with "git config advice.skippedCherryPicks false"                                                                              
    Successfully rebased and updated refs/heads/github-pages-deploy-action/6p10kamt0.
    Pushing changes… (attempt 2 of 3)
/usr/bin/git push --porcelain ***github.com/rossjrw/***.git github-pages-deploy-action/6p10kamt0:gh-pages
    To https://github.com/rossjrw/***.git
    =	refs/heads/github-pages-deploy-action/6p10kamt0:refs/heads/gh-pages	[up to date]
    Done
    Changes committed to the gh-pages branch… 📦

With force: false it looks like the workflow goes something like:

  1. single-commit: true makes the action checkout the deployment branch as an orphan (no history)
  2. Files from the preview are rsync'ed onto that branch
  3. git push is rejected because the history differs from upstream (obviously)
  4. force: false makes the action rebase the commit onto the upstream branch's history - this undoes step 1
  5. git push succeeds, albeit the history is retained instead of destroyed as we intended

I'm thinking that single-commit: true, at least the way it's implemented in JamesIves/github-pages-deploy-action by creating an orphan branch, is fundamentally incompatible with this action. By destroying the previous history of the branch we hit the same issues that I describe in JamesIves/github-pages-deploy-action#1052 (which originally implemented force: false) where there's a chance that a force push from either the main or a preview deployment will destroy the other - except in this case, single-commit: true is guaranteed to destroy all other deployments on the target branch except for the one that's actively being deployed.

It's really lucky in that case that force: false cancels out single-commit: true, as otherwise anyone using this preview action alongside a JamesIves-style main deployment with single-commit: true would've lost all of their previews every time someone pushed to main!

It'd probably be better if, when force: false, single-commit: true instead does something more like:

  1. Squash all extant commits on the branch into one
  2. Rsync the changes onto the branch and git add
  3. git commit --amend
  4. git push --force

Problem with that approach is that force-pushing necessarily risks destroying work contributed by simultaneous workflows, even if we minimise it by pulling immediately before pushing - there's always going to be some amount of time in between. For reference, we do need to worry about simultaneous workflows, because merging a PR will dispatch the 'PR closed' event that removes the preview, and the 'push to main' event that starts a main deployment.

I'm not convinced there's a way around this, honestly. If you want to replace/squash commits you need to rewrite history, if you want to rewrite history you need to force push, and if you want to force push you risk desyncs... right? I'd really love it if one of those assumptions wasn't true.


EDIT: nedbat in #git on Libera.Chat has pointed me to git push --force-with-lease which solves exactly this problem!

I might consider trying to implement this upstream at some point - I'm thinking a try-to-push loop, similar to the try-to-pull loop that force: false uses

@xaviergmail
Copy link
Author

You make good points around pr previews and main deployments! In our case, our dev/staging/prod deployments end up in S3 so I wasn't worried about conflicting deployments but I see the problem.

EDIT: nedbat in #git on Libera.Chat has pointed me to git push --force-with-lease which solves exactly this problem!
Fun to see some familiar names! --force-with-lease is interesting for sure. If you end up making a PR for that upstream, I'd be very grateful. In the meantime, my fork with force-push seems to work well for that use case.

@rossjrw rossjrw added the upstream Issue is relevant to an upstream repo label Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request upstream Issue is relevant to an upstream repo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants