Resolve simultaneous deployments with rebase #1054
Conversation
Determining whether to create a merge commit would elicit a nested conditional, which could be hard to parse for a human reader. This is avoided by returning early as soon as possible for a dry run. This also resolves the erroneous 'changes committed' message when no changes were actually committed because of the dry run. A message specific to dry-run is logged instead.
Existing behaviour is equivalent to force=true, so the default value is true.
I think the fetch will update the origin/gh-pages branch but not the gh-pages branch, despite requesting gh-pages. This means that when I later attempt to rebase the temp branch on top of the gh-pages branch, there will be nothing to do, because that's already where it is.
I don't expect this to ever require more than one attempt in production, but in theory it's possible that this procedure could loop forever. We would need to keep fetching and rebasing if changes keep being added to the remote. In practice, I believe this would only happen if there are lots of workflows simultaneously deploying to the same branch, all using this action. In this case only one would be able to secure a lock at a time, leading to the total number of attempts being equal to the number of simultaneous deployments, assuming each deployment makes each attempt at the exact same time. The limit may need to be increased or even be configurable, but 3 should cover most uses.
|
Im planning to review this over the weekend. Thanks for the contribution! |
There was a problem hiding this comment.
Looks great I couldn't find any bugs after my first look. Do you mind adding a test case for this into the workflows/integration.yml file? We should add a couple of cases that validate some of the different workflow permutations such as single-commit and repository-name. For the repository name you can point it to MontezumaIves/lab
| @@ -60,6 +60,11 @@ inputs: | |||
| description: 'Do not actually push back, but use `--dry-run` on `git push` invocations insead.' | |||
| required: false | |||
|
|
|||
| force: | |||
There was a problem hiding this comment.
Can we add this to the README under the Optional Choices section?
|
Just noting that I intend to get around to implementing integration tests later this week. |
This test is composed of 3 jobs. The first two jobs run simultaneously, and as such both depend on the previous integration test only. The final job cleans up afterwards, and depends on both of the prior jobs. The two jobs are identical except that they both create a temporary file in a different location. This is to ensure that they conflict. Correctly resolving this conflict by rebasing one deployment over the other, resulting in a deployment containing both files, indicates a successful test.
|
@JamesIves I've created an integration test, though I'm not sure what your preferences are regarding what it should actually do, and I'm not sure that I'm able to even run the test myself. I invite you to look over the jobs I added to the workflow and adjust as appropriate. I suspect you may need to take the reins on the permutations you mentioned. |
Codecov Report
@@ Coverage Diff @@
## dev #1054 +/- ##
==========================================
- Coverage 99.58% 92.96% -6.63%
==========================================
Files 7 7
Lines 243 270 +27
Branches 64 73 +9
==========================================
+ Hits 242 251 +9
- Misses 1 19 +18
Continue to review full report at Codecov.
|
|
Sounds good, I'll take it from here. I'm traveling at the minute but will get to this once I'm home. |
Fixes #1052.
Deployed version of fork: https://github.com/rossjrw/github-pages-deploy-action/tree/dev-merge-test
Use with
uses: rossjrw/github-pages-deploy-action@dev-merge-testThis PR adds a new parameter,
force, which defaults to true. When set to true the action will deploy all changes withgit push --force. When set to any other value, the action will do the following:git pushgit fetchthe remote branch andgit rebaselocal changes onto it.Test repository: https://github.com/rossjrw/temp-pages-deploy-action-merge-test
I've invited you as a collaborator on both repositories in case there are any changes you'd like to make to any branch beyond the single one that 'Allow edits by maintainers' grants.
I have not yet updated tests (edit: I have updated existing tests, which now pass, but have not added any additional ones) or documentation. I've also not tested to see what happens when used in combination with e.g.
singleCommit.