fix(release): rebase before pushing local branch and tag #983
Conversation
dhayab
requested review from
a team,
FabienMotte and
sarahdayan
and removed request for
a team
There was a problem hiding this comment.
Most of the Git stuff lives in shipjs-lib and is tested there.
It could be better to have your command there and tested the same way, and here only test the flow.
| export default ({ remote, currentBranch, dir, dryRun }) => | ||
| runStep({ title: 'Rebasing.' }, () => { | ||
| run({ | ||
| command: `git fetch && git rebase ${remote}/${currentBranch} ${currentBranch}`, |
There was a problem hiding this comment.
I believe git fetch && git rebase can be replaced with git pull --rebase.
There was a problem hiding this comment.
This works indeed, thanks! I removed the fetchAndRebase function and replaced it with the existing pull function with an additional flag for --rebase.
As for the location of git operations, what I'm seeing is that most/all the methods that gather information on git repositories are indeed in shipjs-lib, but the methods used to actually perform git operations are in shipjs, for instance:
gitPushin shipjs/src/helperpullin shipjs/src/step
For testing, apart from empirical testing which was done with a dummy repository, there is no flow test I could hook into or a way to simulate changes on a remote repository, or an individual test on the pull function. I added basic tests that verify the command is well-formed similarly to other steps / helpers.
There was a problem hiding this comment.
This can be done later, but we could add integration tests for the full flow. We create a fake project in which Ship.js is a dependency (like any of our repos) and perform flows with a mocked CI.
There was a problem hiding this comment.
I looked a bit further and there's a workflow similar to this in packages/shipjs-lib/tests with shell scripts bootstrapping mock repositories. This could be reused for packages/shipjs probably.
I won't be able to put more time on it during this sprint, so we can do it later or keep this PR open and add it here next sprint, both are fine for me.
There was a problem hiding this comment.
Yes! Can you just add a ticket in Jira to track it?
packages/shipjs/src/step/pull.js
Outdated
| @@ -1,7 +1,13 @@ | |||
| import runStep from './runStep'; | |||
| import { run } from '../util'; | |||
|
|
|||
| export default ({ remote, currentBranch, dir, dryRun }) => | |||
| export default ({ remote, currentBranch, dir, dryRun, rebase = false }) => | |||
There was a problem hiding this comment.
Or allow more flexibility and pass any flag?
| @@ -43,6 +44,8 @@ async function release({ help = false, dir = '.', dryRun = false }) { | |||
| runPublish({ isYarn, config, releaseTag, dir, dryRun }); | |||
| await runAfterPublish({ version, releaseTag, config, dir, dryRun }); | |||
| const { tagName } = createGitTag({ version, config, dir, dryRun }); | |||
| const currentBranch = getCurrentBranch(dir); | |||
| pull({ remote, currentBranch, dir, dryRun, rebase: true }); | |||
There was a problem hiding this comment.
This should be updated to pass options instead, no?
Summary
On rare occasions, the release process orchestrated by Ship.js cannot be completed if the local main branch and its remote counterpart are not in sync, when an unrelated PR is merged just after a release is started for instance (see one example of such issue).
This is not critical, but it can be a bother as the latest git tag won't be pushed to the remote, and the releases list on GitHub will not be updated too.
This PR addresses the issue by performing a rebase onto the main branch in order to sync them before performing the git tag operation.
Result
The release flow should now be more resilient to this kind of issue.