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.

Sign up for GitHub

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

Jump to bottom

fix(release): rebase before pushing local branch and tag #983

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
+45 −3
Draft

fix(release): rebase before pushing local branch and tag #983

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
02f087e
fix(release): rebase before pushing local branch and tag
dhayab Jun 21, 2022
3c5df94
use modified pull function with --rebase flag
dhayab Jun 21, 2022
26a4c12
add basic unit test for pull function
dhayab Jun 21, 2022
1a9d991
allow any option to be passed to pull method
dhayab Jun 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion packages/shipjs/src/flow/release.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { loadConfig } from 'shipjs-lib';
import { getCurrentBranch, loadConfig } from 'shipjs-lib';

import printHelp from '../step/release/printHelp';
import printDryRunBanner from '../step/printDryRunBanner';
Expand All @@ -15,6 +15,7 @@ import createGitHubRelease from '../step/release/createGitHubRelease';
import notifyReleaseSuccess from '../step/release/notifyReleaseSuccess';
import checkGitHubToken from '../step/checkGitHubToken';
import finished from '../step/release/finished';
import fetchAndRebase from '../step/fetchAndRebase';
import { detectYarn } from '../util';

async function release({ help = false, dir = '.', dryRun = false }) {
Expand Down Expand Up @@ -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);
await fetchAndRebase({ remote, currentBranch, dir, dryRun });
gitPush({ tagName, config, dir, dryRun });
await createGitHubRelease({ version, config, dir, dryRun });
await notifyReleaseSuccess({
Expand Down
11 changes: 11 additions & 0 deletions packages/shipjs/src/step/fetchAndRebase.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import runStep from './runStep';
import { run } from '../util';

export default ({ remote, currentBranch, dir, dryRun }) =>
runStep({ title: 'Rebasing.' }, () => {
run({
command: `git fetch && git rebase ${remote}/${currentBranch} ${currentBranch}`,
Copy link
Member

Choose a reason for hiding this comment

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

I believe git fetch && git rebase can be replaced with git pull --rebase.

Copy link
Member Author

Choose a reason for hiding this comment

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

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:

  • gitPush in shipjs/src/helper
  • pull in 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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

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 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.

Copy link
Member

Choose a reason for hiding this comment

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

Yes! Can you just add a ticket in Jira to track it?

dir,
dryRun,
});
});