pr-preview-action icon indicating copy to clipboard operation
pr-preview-action copied to clipboard

Add `single-commit` input

Open xaviergmail opened this issue 5 months ago • 3 comments

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

xaviergmail avatar Jan 29 '24 19:01 xaviergmail

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 avatar Jan 30 '24 10:01 rossjrw

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 https://github.com/JamesIves/github-pages-deploy-action/issues/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

rossjrw avatar Jan 30 '24 12:01 rossjrw

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.

xaviergmail avatar Jan 30 '24 18:01 xaviergmail