feat(version): Add `--include-merged-tags` option #1712

HazAT · 2018-10-02T20:26:33Z

Description

Disclaimer:
This is the successor PR to
#1704

lerna fails to detect/consider tags from merged branches.
This results in that if you tag for example on a release branch, lerna changed returns all changes until the last tag of the current branch.

Let's say you have the following setup:

When running lerna changed results in this output:

yarn lerna changed
yarn run v1.3.2
$ /Users/haza/Projects/sentry-javascript/node_modules/.bin/lerna changed
lerna notice cli v3.4.0
lerna info Looking for changed packages since 4.0.5
@sentry/browser
@sentry/core
@sentry/hub
@sentry/minimal
@sentry/node
@sentry/types
@sentry/utils
lerna success found 7 packages ready to publish
✨  Done in 0.95s.

When clearly there is a merged release branch with version 4.0.6.
It's still true that 4.0.5 was tagged on the master branch.

lerna runs following command under the hood: git describe --always --long --dirty --first-parent which returns: 4.0.5-9-g89885fae which is correct but not taking tags on other branches into the calculation.

A new option include-merged-tags therefore was added to support tags from merged branches.
It changes the previous git describe command to
git describe --always --long --dirty --tags which results in 4.0.6-8-g6fbeff9a which in the example stated above is correct.

We are using lerna for our javascript SDKs https://github.com/getsentry/sentry-javascript

Motivation and Context

We think that tagging in a release branch should be a valid scenario the lerna should be able to support. Right now the only way to work around this limitation is if we would work around lerna for bumping and releasing.

How Has This Been Tested?

Locally in our repo + added test cases highlighting the new behavior.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

HazAT · 2018-10-02T20:31:24Z

@evocateur Sorry for the PR spam, while I stated in the PR before that we found a workaround with --since which is true to basically replace lerna changed but it's not allowing us to use lerna version.

This PR is still WIP I just want to know if you consider merging this new feature at all, if so please let me know I will finish the PR (write tests + docs for the new option).

Thanks for your time and the great project.

HazAT · 2018-10-09T09:35:58Z

OK, added docs + tests, one existing test is failing (that I did not touch), not sure why though.

dzsodzso63 · 2018-10-16T08:55:04Z

Sorry for the PR spam, while I stated in the PR before that we found a workaround with --since which is true to basically replace lerna changed but it's not allowing us to use lerna version.

@HazAT can you please explain this "workaround"? we have this issue since upgrading to lerna 3, that we cannot determine changed packages because of the --first-parent param (publish from release branches + PR), but --since switch doesn't work since lerna 3. What we should do?

HazAT · 2018-10-16T10:35:15Z

@dzsodzso63 So we found a workaround for changed, using --since
yarn lerna exec --since $(git rev-list --tags --max-count=1) pwd
But it doesn't work for version / publish so right now we are using my fork right now.

dzsodzso63 · 2018-10-16T11:26:32Z

@HazAT thanks, this made us an idea to check changes like
lerna exec --since $(git rev-list --tags --max-count=1) --loglevel=silent -- npm v . name
but the real win would be this PR merged (we now can scope down other processes (e.g. tests), but we publish everything on each merge).
Do you think this merge can happen sometime?

HazAT · 2018-10-16T11:33:37Z

@dzsodzso63 I mean I don't see why this shouldn't be merged but it's not in my power ;)
I am happy to do anything to get this merged since we also don't want to rely on my fork for this ^^

dzsodzso63 · 2018-10-17T14:25:56Z

@evocateur Can you please review this one?

danielcondemarin · 2018-10-26T12:58:13Z

Hi @HazAT @dzsodzso63 Did you consider rebasing over true merges? Seems to work fine if you rebase your release branch onto master.

HazAT · 2018-10-29T07:56:47Z

Rebased ♻️

dzsodzso63 · 2018-10-30T16:00:51Z

Hi @HazAT @dzsodzso63 Did you consider rebasing over true merges? Seems to work fine if you rebase your release branch onto master.

@danielcondemarin Maybe I missing something here, but in my understanding, when I rebase the publisher branch to master, then the sha changes, so the tags created by lerna will point to non-existing commits. What do I miss?

HazAT · 2018-10-30T16:35:11Z

@dzsodzso63 @danielcondemarin Right, we create the tag in our release branch, rebasing would create a new sha.
We really want to have a "snapshot" of the tagged commit that we built.
While rebasing and tagging afterward will probably work 99% of the time we run all tests on this specific commit/tag and want to release it without building it again.

dzsodzso63 · 2018-10-30T20:34:33Z

@HazAT ok, then we are on the same page (and we cannot even think about publishing after merge, as we have protected master branch and we cannot commit to that directly)

HazAT · 2018-11-15T11:34:02Z

@evocateur Is there anything left to do for this getting merged?

StarpTech · 2018-11-18T14:18:54Z

utils/describe-ref/lib/describe-ref.js

+    // we want to consider all tags, also from merged branches
+    args = args.filter(arg => arg !== "--first-parent");
+    // also include lightweight tags
+    args.push("--tags");


Did you consider these regarding lightweight tags?

https://github.com/lerna/lerna/blob/aa4ae510b9c0a0935f0b1989859ba873a1e864cc/CHANGELOG.md#breaking-changes-2

https://github.com/lerna/lerna/blob/f674f354f0260c57d885c181e16c9ce23ac252a8/doc/troubleshooting.md#publish-does-not-detect-manually-created-tags-in-fixed-mode-with-githubgithub-enterprise

This doesn't really work for our case. As explained we want to tag in a release branch that gets merged into master. But the tagged commit is on the branch.

I only want to point to the fact that lightweight tags aren't coompatible with lerna and it seems that it will hard to land this.

I see but it isn't really just a lightweight tag, right?

This is not going to fly. Lightweight tags are not intended to be used for tagging releases.

evocateur

I like this approach, sorry for the delay in review

evocateur · 2018-11-23T20:41:05Z

commands/changed/command.js

@@ -15,7 +15,13 @@ exports.describe = "List local packages that have changed since the last tagged
 exports.builder = yargs => {
  listable.options(yargs);

-  return versionOptions(yargs, "changed");
+  const newYargs = versionOptions(yargs, "changed");
+  newYargs.option("include-merged-tags", {


Since this was already added in commands/version/command.js, you don't need to duplicate it here? versionOptions() was designed to avoid this duplication. It is hidden, true, but it still works.

commands/version/__tests__/version-command.test.js

evocateur · 2018-11-23T20:51:55Z

commands/changed/__tests__/changed-command.test.js

@@ -41,6 +44,26 @@ const setupGitChanges = async (cwd, filePaths) => {
  await gitCommit(cwd, "Commit");
 };

+const setupGitChangesWithBranch = async (cwd, masterPaths, branchPaths) => {


This helper and the tests should be moved to a focused test file under the version command, as described previously. lerna changed is just a lightweight wrapper around the internals used by lerna version, it doesn't have any logic itself beyond the list output options.

evocateur · 2018-11-23T20:54:36Z

commands/changed/README.md

@@ -27,3 +27,12 @@ package-2
 Unlike `lerna ls`, however, `lerna changed` **does not** support [filter options](https://www.npmjs.com/package/@lerna/filter-options), as filtering is not supported by `lerna version` or `lerna publish`.

 `lerna changed` also supports all the flags supported by [`lerna version`](https://github.com/lerna/lerna/tree/master/commands/version#options), but the only relevant one is [`--ignore-changes`](https://github.com/lerna/lerna/tree/master/commands/version#--ignore-changes).
+
+### `--include-merged-tags`


This duplicated options block is not necessary. Adding a link to the docs under commands/version directly is fine.

evocateur · 2018-11-23T20:55:36Z

helpers/git-checkout/index.js

+
+const execa = require("execa");
+
+module.exports = gitCheckout;


Thanks for these helpers, they are handy :)

evocateur · 2018-11-23T20:57:52Z

utils/describe-ref/lib/describe-ref.js

+    // we want to consider all tags, also from merged branches
+    args = args.filter(arg => arg !== "--first-parent");
+    // also include lightweight tags
+    args.push("--tags");


This is not going to fly. Lightweight tags are not intended to be used for tagging releases.

HazAT · 2018-11-27T16:04:53Z

@evocateur Thanks for your review, I made the changes you requested.

evocateur

Excellent, thanks!

lock · 2019-01-26T18:40:50Z

This thread has been automatically locked because there has not been any recent activity after it was closed. Please open a new issue for related bugs.

HazAT changed the title ~~[WIP] Add include-merged-tags option to changed / version comand~~ [WIP] feat(changes/version): Add include-merged-tags option to changed / version comand Oct 2, 2018

HazAT changed the title ~~[WIP] feat(changes/version): Add include-merged-tags option to changed / version comand~~ WIP: feat(changes/version): Add include-merged-tags option to changed / version comand Oct 2, 2018

HazAT changed the title ~~WIP: feat(changes/version): Add include-merged-tags option to changed / version comand~~ WIP: feat(changed/version): Add include-merged-tags option to changed / version comand Oct 2, 2018

HazAT force-pushed the master branch from 24855bd to 3d4e745 Compare October 3, 2018 07:43

HazAT changed the title ~~WIP: feat(changed/version): Add include-merged-tags option to changed / version comand~~ feat(changed/version): Add include-merged-tags option to changed / version comand Oct 9, 2018

HazAT changed the title ~~feat(changed/version): Add include-merged-tags option to changed / version comand~~ feat(changed/version): Add include-merged-tags option to changed / version command Oct 9, 2018

HazAT force-pushed the master branch from 4254ed2 to 485488e Compare October 9, 2018 12:06

dzsodzso63 mentioned this pull request Oct 25, 2018

[version/publish] should be able to work with merged tags #1747

Closed

HazAT force-pushed the master branch from 485488e to 5a59923 Compare October 29, 2018 07:56

zhusee2 mentioned this pull request Oct 30, 2018

React 16 upgrade iCHEF/gypcrete#81

Closed

8 tasks

StarpTech reviewed Nov 18, 2018

View reviewed changes

evocateur suggested changes Nov 23, 2018

View reviewed changes

HazAT added 2 commits November 27, 2018 14:59

Add include-merged-tags option to changed/version

23e8389

feat: Add tests + docs

e6110c5

HazAT force-pushed the master branch from 5a59923 to e6110c5 Compare November 27, 2018 14:01

HazAT added 2 commits November 27, 2018 16:04

ref: Move changed tests to version tests

352c8ce

ref: Remove support for lightweight tags

e9f995d

fix: Remove lightweight tags from tests

4e29fcc

HazAT changed the title ~~feat(changed/version): Add include-merged-tags option to changed / version command~~ feat(version): Add include-merged-tags option to version command Nov 27, 2018

evocateur approved these changes Nov 27, 2018

View reviewed changes

evocateur changed the title ~~feat(version): Add include-merged-tags option to version command~~ feat(version): Add --include-merged-tags option Nov 27, 2018

evocateur merged commit 7ee05d7 into lerna:master Nov 27, 2018

nicolo-ribaudo pushed a commit to babel/lerna that referenced this pull request Dec 18, 2018

feat(version): Add --include-merged-tags option (lerna#1712)

6b8a7a4

lock bot locked as resolved and limited conversation to collaborators Jan 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(version): Add `--include-merged-tags` option #1712

feat(version): Add `--include-merged-tags` option #1712

HazAT commented Oct 2, 2018 •

edited

HazAT commented Oct 2, 2018

HazAT commented Oct 9, 2018

dzsodzso63 commented Oct 16, 2018

HazAT commented Oct 16, 2018 •

edited

dzsodzso63 commented Oct 16, 2018

HazAT commented Oct 16, 2018

dzsodzso63 commented Oct 17, 2018

danielcondemarin commented Oct 26, 2018

HazAT commented Oct 29, 2018 •

edited

dzsodzso63 commented Oct 30, 2018 •

edited

HazAT commented Oct 30, 2018

dzsodzso63 commented Oct 30, 2018

HazAT commented Nov 15, 2018

StarpTech Nov 18, 2018

HazAT Nov 19, 2018

StarpTech Nov 19, 2018

HazAT Nov 19, 2018

evocateur Nov 23, 2018

evocateur left a comment

evocateur Nov 23, 2018

evocateur Nov 23, 2018

evocateur Nov 23, 2018

evocateur Nov 23, 2018

evocateur Nov 23, 2018

HazAT commented Nov 27, 2018

evocateur left a comment

lock bot commented Jan 26, 2019


		const execa = require("execa");

		module.exports = gitCheckout;

feat(version): Add --include-merged-tags option #1712

feat(version): Add --include-merged-tags option #1712

Conversation

HazAT commented Oct 2, 2018 • edited

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

HazAT commented Oct 2, 2018

HazAT commented Oct 9, 2018

dzsodzso63 commented Oct 16, 2018

HazAT commented Oct 16, 2018 • edited

dzsodzso63 commented Oct 16, 2018

HazAT commented Oct 16, 2018

dzsodzso63 commented Oct 17, 2018

danielcondemarin commented Oct 26, 2018

HazAT commented Oct 29, 2018 • edited

dzsodzso63 commented Oct 30, 2018 • edited

HazAT commented Oct 30, 2018

dzsodzso63 commented Oct 30, 2018

HazAT commented Nov 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evocateur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HazAT commented Nov 27, 2018

evocateur left a comment

Choose a reason for hiding this comment

lock bot commented Jan 26, 2019

feat(version): Add `--include-merged-tags` option #1712

feat(version): Add `--include-merged-tags` option #1712

HazAT commented Oct 2, 2018 •

edited

HazAT commented Oct 16, 2018 •

edited

HazAT commented Oct 29, 2018 •

edited

dzsodzso63 commented Oct 30, 2018 •

edited