pr-preview-action
pr-preview-action copied to clipboard
Add `single-commit` input
This change simply adds a configurable single-commit
option to be passed to JamesIves/github-pages-deploy-action
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.
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:
-
single-commit: true
makes the action checkout the deployment branch as an orphan (no history) - Files from the preview are rsync'ed onto that branch
-
git push
is rejected because the history differs from upstream (obviously) -
force: false
makes the action rebase the commit onto the upstream branch's history - this undoes step 1 -
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:
- Squash all extant commits on the branch into one
- Rsync the changes onto the branch and
git add
-
git commit --amend
-
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
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.