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

Fix deploy workflow (take 2) #1126

Merged
merged 6 commits into from
Nov 29, 2024
Merged

Fix deploy workflow (take 2) #1126

merged 6 commits into from
Nov 29, 2024

Conversation

zcorpan
Copy link
Member

@zcorpan zcorpan commented Nov 21, 2024

Generate data after rebasing; only rebase when main has changed; force push. Fixes #1125.

Generate data after rebasing; only rebase when main has changed; force push. Fixes #1125.
.github/workflows/build-and-deploy.yml Show resolved Hide resolved
.github/workflows/build-and-deploy.yml Outdated Show resolved Hide resolved

# Push changes back to `gh-pages`
git remote set-url origin [email protected]:${{ github.repository }}.git
git push origin gh-pages
git push origin gh-pages --force
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that you want force pushes for this. If the build fails, that's likely because you are running multiple concurrent builds. Let it fail in that case. You'll get an email, but the next build should repair things.

On the other hand, force pushes can destroy history.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rebase (line 50) means you have to force push, though, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe using merge commits is a better alternative?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think about what leads to a conflict here. If this is the only process by which the gh-pages branch is updated, then you only end up here if there are two workflows running concurrently. That happens if it takes a little while and there are two runs triggered. The cron job would be fine, but pushes to main can conflict.

If there is a conflict then, it's most likely to be because you are the run associated with the second (or third...) push to main. A failure therefore likely results in the state from an old push to main being pushed to gh-pages.

I would suggest that you minimize the exposure by making the gh-pages checkout and the ensuing update be as close as possible together. Then, if it still fails, you can maybe check that you are the last commit on main (git ls-remote origin refs/heads/main should return the same hash as git show-ref refs/heads/main). If you are, then looping back to the point where you fetch and try to push is probably best. Do that at most a few times.

You shouldn't have to fetch many more commits if there is a mid-air collision, so make sure things go faster the second time by reusing the checkout. (I might suggest putting the gh-pages checkout in a subdirectory using actions/checkout, then refreshing the checkout in the loop.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added concurrency group for this action to avoid mid-air collision issues.

Copy link
Member

@tantek tantek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL "force-with-lease". This looks like it addresses the concern @martinthomson pointed out but will await his confirmation for this fix.

# Rebase `gh-pages` onto `main` to ensure a linear history
- name: Rebase `gh-pages` onto `main` (if triggered by push to main)
if: github.event_name == 'push' && github.ref == 'refs/heads/main'
run: |
git rebase main
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should have said: this is where things get a little whacky. It's not going to be possible to have a linear history like this. At some point, you will need to do something.

If you want the history of gh-pages to track main, then you will want merges. I suggest using -s theirs to resolve conflicts on the merge. That should work. However, you should probably have some sort of check that the files you are generating do not appear on main.

The other race condition considerations I outlined still apply.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, if you wanted to ensure that this gets the right commits, then you need to use ${{ github.sha }} rather than main here. Otherwise, if there are multiple pushes to main, you could end up with strange things happening. I think that GitHub's checkout action does funny things with ref names, so maybe that's not a genuine problem, but maybe avoid having to think about that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, thanks. Fixed. The generated files are listed in .gitignore, do you think that's sufficient?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also fixed ${{ github.sha }}

Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zcorpan zcorpan merged commit f186ec7 into main Nov 29, 2024
1 check passed
@zcorpan zcorpan deleted the zcorpan/deploy-take-2 branch November 29, 2024 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deploy workflow is broken
3 participants