Add --since flag to all commands that accept --scope #822

treshugart · 2017-05-18T21:47:39Z

Description

The --since option applies to all commands that accept --scope.

By default --since will scope the command to all packages that have been updated since the latest tag. If a ref is passed (i.e. --since ref), then it will scope the command to all packages that have been updated since the specified ref.

Motivation and Context

Currently we have a fairly large repo of packages (currently 68) and the builds take quite awhile to complete because when CI runs, it uses lerna run and that executes on all packages. This means our build only gets slower as we add more packages.

Since we only care about packages that have actually changed, we can theoretically use --scope to filter the packages that should be run. However, in practice, this isn't very simple because we have to figure out which packages have been updated in the branch and then pass them off to this argument. This means we have to create a separate command outside of Lerna that then proxies lerna run --scope.

The problem with creating a separate script means that we're no longer directly using Lerna. Any new scripts we add to packages must now be run through this separate script and it creates what seems like unnecessary indirection when Lerna already has the facilities to do this internally. We felt the most elegant way would be to create a flag like --scope that follows the same naming convention and can even be used in conjunction with it, but that allows the caller to also specify a ref to scope the filtered packages to. If a ref isn't provided, it defaults to the latest tag, as it already does internally.

How Has This Been Tested?

The changes were tested at the main Command level in the .runValidations() test by adding a section for .filteredPackages. I felt this was the best place for a few reasons:

It can apply to most commands and packages are filtered in the base Command class.
It seemed like there were other places the behaviour of --scope was integration tested, but I'm not sure if that would have been too much duplication for the testing of this since it would be testing pretty much the same thing every time. This also keeps the surface area of the changes smaller.

There was one change to the Command test file which was required in order to get the expected packages after updating the fixtures. It appears that whole modules were being mocked via Jest but not actually being used because the mocks that were being used were being manually done later in the file in *Each() blocks. I ended up just removing the global mocks and everything worked.

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.

Related issues

No original issue was provided but this also:

evocateur · 2017-05-18T21:51:55Z

I just merged #726, does that cover your use case?

treshugart · 2017-05-18T21:52:51Z

test/Command.js

+
+      class TestCommand extends Command {}
+
+      function cli(cmd, ...args) {


I saw similar helpers in the tests for the updated command, however, in order to reduce the amount of changes, I decided to write a few extra lines of code rather than refactoring them out into a common lib.

Ideally, this would be the approach used for all unit tests of commands. The .run() method really needs to return a Promise, already... (out of scope for this PR, clearly)

treshugart · 2017-05-18T21:54:00Z

test/fixtures/UpdatedCommand/basic/packages/package-5/package.json

@@ -1,7 +1,7 @@
 {
  "name": "package-5",
  "version": "1.0.0",
-  "private": "true",
+  "private": true,


Good samaritan fix.

treshugart · 2017-05-18T21:54:48Z

test/Command.js

+        expect(filteredPackages[1].name).toEqual("package-3");
+      });
+
+      // it("should respect --scope and --scoped-updated when used together", () => {


treshugart · 2017-05-18T22:00:12Z

test/Command.js

+        return cmd;
+      }
+
+      it("--scope should filter packages", () => {


This isn't part of the functionality that I'm adding, but I wanted to make sure this behaviour was tested in this manner, especially since I'm testing the combination of --scope and --since.

treshugart · 2017-05-18T22:02:39Z

test/Command.js

+        const { filteredPackages } = run({ scopeUpdated: "" });
+        expect(filteredPackages.length).toEqual(2);
+        expect(filteredPackages[0].name).toEqual("package-2");
+        expect(filteredPackages[1].name).toEqual("package-3");


Why is it that package-3 appears in the list when package-2 has changed? This is the default behaviour of the UpdatedPackagesCollector.

It would appear that packages that have devDependencies on packages that have changed get added to the list. This is more of a question than anything else, I guess, so that I understand it better.

treshugart · 2017-05-18T22:39:29Z

@evocateur

This is the same as only-updated in that it acts as a flag if no ref is provided. However, it differs in that it allows you to provide a ref. For our use case, we'd probably just use master as the target since we can't currently get the target branch for a PR in BitBucket Pipelines. However, Travis does allow this, so one would probably run it against TRAVIS_BRANCH in a PR and leave it off if run from master.

With the current functionality, it would be very easy to have a build run against packages that actually haven't changed if you don't release on every commit or push, for whatever reason be it feature branches that eventually go into master or if you accumulate commits in master and release every so often.

treshugart · 2017-05-22T18:46:00Z

I spoke to @jameslnewell about this internally, and he seemed to think that being able to specify a ref would be useful, too. Maybe we could work on merging the two ideas into this?

evocateur · 2017-05-22T19:05:38Z

I'm cool with --only-updated=[sinceRef] or a more explicit --since=ref, sure.

We definitely want to change the this.flags.onlyUpdated references to this.options.onlyUpdated to support "durable" config settings in lerna.json, I missed this during the previous PR.

treshugart · 2017-05-22T20:22:39Z

Okay, cool. Would you prefer I bring in latest master and rework the --only-updated flag in with this functionality (so we don't lose the extra tests with the first PR)? Or do you want to revert it and then use only this one?

Either way, I'll do the following:

Rename --scope-updated to --since
Squash back to a single commit
Add docs (I intentionally kept this out until everything was hashed out)

evocateur · 2017-05-22T20:26:24Z

Rebase and squash is fine, I like new tests :)

treshugart · 2017-05-24T19:57:55Z

Merged and squashed commits after merge. Will annotate updates.

treshugart · 2017-05-24T19:59:01Z

Hmm, I don't think that rebase went as expected. I'll fix up.

treshugart · 2017-05-24T20:09:04Z

README.md

@@ -675,6 +675,22 @@ $ lerna exec --scope my-component -- ls -la
 $ lerna run --scope toolbar-* test
 ```

+#### --since [ref]


I moved this to just after the --scope arg since it relates very closely to it.

treshugart · 2017-05-24T20:09:47Z

README.md

+```
+
+```
+$ lerna ls --since some-branch


I noticed while doing my updates that --since when used with lerna ls seems to make lerna updated redundant (except that one shows versions and one doesn't).

treshugart · 2017-05-24T20:10:44Z

src/UpdatedPackagesCollector.js

@@ -52,52 +52,45 @@ export default class UpdatedPackagesCollector {
  collectUpdatedPackages() {
    this.logger.info("", "Checking for updated packages...");

-    const hasTags = GitUtilities.hasTags(this.execOpts);
+    const { execOpts, options } = this;


Boyscout: Instead of accessing on this or options multiple times, I destructured here.

treshugart · 2017-05-24T20:11:16Z

src/UpdatedPackagesCollector.js

-      this.logger.warn("", "No tags found!");
-      this.logger.info("", "Comparing with initial commit.");
-    }
+    if (GitUtilities.hasTags(execOpts)) {


While this is a boyscout, it's also a fix for the lines I added to the test to make sure that since overrides the latest tag, if there are tags present.

treshugart · 2017-05-24T20:16:42Z

src/UpdatedPackagesCollector.js

    }

+    this.logger.info("", `Comparing with ${since || "initial commit"}.`);


@evocateur (mentioning since I want to place special emphasis here), I removed the warning if no tags exist because it doesn't really feel like a warning, especially with the addition of --since as an empty arg is implicitly the latest tag (or initial commit).

If there's any contention here, or it's a breaking change, I'm more than happy to add it back in.

treshugart · 2017-05-24T20:18:11Z

test/Command.js

+      });
+
+      it("--since \"ref\" should return packages updated since the specified ref", () => {
+        // We first tag, then modify master to ensure that specifying --since will override checking against


This was the addition that caught the bug where the latest tag would override the specified ref in --since. I annotated the spot above that fixes it.

treshugart · 2017-05-24T20:34:58Z

src/commands/ExecCommand.js

-        packageUpdates
-      );
-    }
+    const { filteredPackages } = this;


Boyscout: kept assignment but destructured and converted to const instead.

treshugart · 2017-05-24T20:35:22Z

src/commands/ExecCommand.js


    this.batchedPackages = this.toposort
      ? PackageUtilities.topologicallyBatchPackages(filteredPackages)
-      : [this.filteredPackages];
+      : [filteredPackages];


Since we're not filtering anymore, we can use the const.

treshugart · 2017-05-24T20:35:30Z

src/commands/RunCommand.js

-        packageUpdates
-      );
-    }
+    const { filteredPackages } = this;


Boyscout: kept assignment but destructured and converted to const instead.

…mands that accept --scope.

evocateur · 2017-05-25T22:41:06Z

@treshugart Just a quick note to say I haven't forgotten you, I'm just swamped at work and will try to carve out some time soon!

treshugart · 2017-05-26T01:08:06Z

@evocateur ready for re-review. Not sure why AppVeyor is failing. I can't seem to spot the error, but it looks like something might have timed out? I ran the tests locally and everything seemed fine. I'll also have someone from my team review, too.

EDIT

It looks like I made this comment just after you had commented. Thanks for letting me know :)

…e about its usefulness in CI.

chrisui · 2017-06-12T13:08:45Z

This looks great! I just opened an issue requesting this elsewhere. I also mentioned the use of a --until option (or --git-to in that issue) which might be good to add here as well?

See #876

treshugart · 2017-06-13T00:39:03Z

@chrisui I personally don't have a use case for it (yet?), but it sounds like it could be useful. I would vote for keeping this PR as is with a plan to implement --until (or whatever) after it lands.

treshugart · 2017-06-22T07:03:45Z

Hey @evocateur, I hate to be a pest, but I was hoping I could get a review / merge soon. We're currently running off of my fork and it seems that quite a few others need this as well. Cheers for the time you've spent already.

evocateur

Really sorry I took so long to review this.

evocateur · 2017-06-28T18:25:29Z

test/Command.js

+
+      class TestCommand extends Command {}
+
+      function cli(cmd, ...args) {


Ideally, this would be the approach used for all unit tests of commands. The .run() method really needs to return a Promise, already... (out of scope for this PR, clearly)

evocateur · 2017-06-28T18:28:57Z

README.md

+List the contents of packages that have changed since the latest tag:
+
+```sh
+$ lerna exec --since -- ls -la


My only complaint with this usage is that it isn't as readable as lerna exec --only-updated. The nature of the beast, I suppose. (Aliases that are distinct long options are a bit awkward, and probably better to omit entirely).

treshugart · 2017-06-28T19:09:46Z

No problem. Totally understand. Thanks for the review and merge!

chrisui · 2017-06-28T19:40:24Z

This is great 👍 good work!

refs #822

lock · 2018-12-27T13:10:36Z

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.

treshugart force-pushed the master branch from 4ad7182 to 7409311 Compare May 18, 2017 21:50

treshugart commented May 18, 2017

View reviewed changes

treshugart force-pushed the master branch from 7409311 to d9a7350 Compare May 18, 2017 21:58

treshugart commented May 18, 2017

View reviewed changes

treshugart force-pushed the master branch from d1f302d to 9139fea Compare May 24, 2017 19:56

treshugart force-pushed the master branch from 9139fea to d1f302d Compare May 24, 2017 20:04

treshugart commented May 24, 2017

View reviewed changes

treshugart force-pushed the master branch 2 times, most recently from 3914a29 to 16f4f54 Compare May 24, 2017 20:33

treshugart commented May 24, 2017

View reviewed changes

treshugart changed the title ~~feat: Add ability to specify a --scope-updated flag that applies to a…~~ feat: Add ability to specify a --since flag that applies to a… May 24, 2017

feat: Add ability to specify a --since option that applies to all com…

65c49ef

…mands that accept --scope.

treshugart force-pushed the master branch from 16f4f54 to 65c49ef Compare May 24, 2017 20:38

treshugart mentioned this pull request May 25, 2017

lerna bootstrap should support --only-updated flag #847

Closed

treshugart added 2 commits May 25, 2017 14:33

Merge branch 'master' of github.com:lerna/lerna

eac8c17

docs: Update command documentation.

39c3b58

treshugart mentioned this pull request May 30, 2017

lerna diff against arbitrary gitref #855

Closed

docs: Update README to have better descriptions of commands and a not…

cc429c8

…e about its usefulness in CI.

treshugart mentioned this pull request May 31, 2017

lerna run: Don't error if no scoped packages are matched #859

Closed

treshugart force-pushed the master branch from 2aec328 to cc429c8 Compare June 5, 2017 21:10

ricky mentioned this pull request Jun 12, 2017

lerna [updated|diff] <commit> #876

Closed

treshugart mentioned this pull request Jun 22, 2017

Durable options in lerna.json #893

Closed

treshugart added 2 commits June 22, 2017 12:18

Merge branch 'master' of github.com:lerna/lerna

2e6e621

Merge branch 'master' of github.com:lerna/lerna

762af6f

evocateur approved these changes Jun 28, 2017

View reviewed changes

evocateur added the Tag: Enhancement label Jun 28, 2017

evocateur changed the title ~~feat: Add ability to specify a --since flag that applies to a…~~ Add --since flag to all commands that accept --scope Jun 28, 2017

evocateur merged commit cf11901 into lerna:master Jun 28, 2017

evocateur added a commit that referenced this pull request Mar 9, 2018

fix(filter-options): require a git repo when using --since

d21b66e

refs #822

lock bot locked as resolved and limited conversation to collaborators Dec 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --since flag to all commands that accept --scope #822

Add --since flag to all commands that accept --scope #822

treshugart commented May 18, 2017 •

edited

Loading

evocateur commented May 18, 2017

treshugart May 18, 2017

evocateur Jun 28, 2017

treshugart May 18, 2017

treshugart May 18, 2017

treshugart May 18, 2017 •

edited

Loading

treshugart May 18, 2017

treshugart commented May 18, 2017

treshugart commented May 22, 2017

evocateur commented May 22, 2017

treshugart commented May 22, 2017 •

edited

Loading

evocateur commented May 22, 2017

treshugart commented May 24, 2017

treshugart commented May 24, 2017

treshugart May 24, 2017

treshugart May 24, 2017

treshugart May 24, 2017

treshugart May 24, 2017

treshugart May 24, 2017

treshugart May 24, 2017

treshugart May 24, 2017

treshugart May 24, 2017

treshugart May 24, 2017

evocateur commented May 25, 2017

treshugart commented May 26, 2017 •

edited

Loading

chrisui commented Jun 12, 2017

treshugart commented Jun 13, 2017

treshugart commented Jun 22, 2017

evocateur left a comment

evocateur Jun 28, 2017

evocateur Jun 28, 2017

treshugart commented Jun 28, 2017

chrisui commented Jun 28, 2017

lock bot commented Dec 27, 2018


		class TestCommand extends Command {}

		function cli(cmd, ...args) {

		}

		this.logger.info("", `Comparing with ${since \|\| "initial commit"}.`);

Add --since flag to all commands that accept --scope #822

Add --since flag to all commands that accept --scope #822

Conversation

treshugart commented May 18, 2017 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Related issues

evocateur commented May 18, 2017

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

treshugart May 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

treshugart commented May 18, 2017

treshugart commented May 22, 2017

evocateur commented May 22, 2017

treshugart commented May 22, 2017 • edited Loading

evocateur commented May 22, 2017

treshugart commented May 24, 2017

treshugart commented May 24, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evocateur commented May 25, 2017

treshugart commented May 26, 2017 • edited Loading

chrisui commented Jun 12, 2017

treshugart commented Jun 13, 2017

treshugart commented Jun 22, 2017

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

treshugart commented Jun 28, 2017

chrisui commented Jun 28, 2017

lock bot commented Dec 27, 2018

treshugart commented May 18, 2017 •

edited

Loading

treshugart May 18, 2017 •

edited

Loading

treshugart commented May 22, 2017 •

edited

Loading

treshugart commented May 26, 2017 •

edited

Loading