[tool] add `all` and `dry-run` flags to publish-plugin command #3776

cyanglaz · 2021-03-31T19:38:00Z

Add a new all flag to the publish plugin command does the following:

Look for all the pubspec changes in the current commit and determine what plugin(s) need to be released. If the new version is lower than the existing latest tagged version, skip.
Release the plugin as how publish-plugin works: publishing to pub, git tag release etc.

Add a new dry-run flag to dry-run the publish and tag release process. This is for testing locally and ci without actually publishing or git tag anything.

This command is going to be part of auto-publish workflow.

part of flutter/flutter#79830

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

cyanglaz · 2021-04-01T01:34:04Z

As we discussed in the doc, we are probably going to use the alternative if LUCI push to git works.
If that's the case, the command should include both publish and git tag. I will convert this to draft until implement the publish part.

stuartmorgan · 2021-04-23T16:15:15Z

script/tool/lib/src/publish_plugin_command.dart

  StreamSubscription<String> _stdinSubscription;

  @override
  Future<void> run() async {
    final String package = argResults[_packageOption] as String;
-    if (package == null) {
+    final bool all = argResults[_allFlag] as bool;


Nit: how about publishAll as the variable name so it's more descriptive.

stuartmorgan · 2021-04-23T16:16:42Z

script/tool/lib/src/publish_plugin_command.dart

@@ -64,13 +68,31 @@ class PublishPluginCommand extends PluginCommand {
      // Flutter convention is to use "upstream" for the single source of truth, and "origin" for personal forks.
      defaultsTo: 'upstream',
    );
+    argParser.addFlag(
+      _allFlag,


Maybe we should call this flag all-changed? --all sounds more like it should do something like the Melos publish-all command.

sounds good! DONE

stuartmorgan · 2021-04-23T16:17:04Z

script/tool/lib/src/publish_plugin_command.dart

@@ -111,7 +135,74 @@ class PublishPluginCommand extends PluginCommand {
    }
    _print('Local repo is ready!');

-    final Directory packageDir = _getPackageDir(package);
+    if (all) {
+      await _publishAllPackages(


Similarly, _publishAllChangedPackages

stuartmorgan · 2021-04-23T16:19:47Z

script/tool/lib/src/publish_plugin_command.dart

+    final List<String> changedPubspecs =
+        await gitVersionFinder.getChangedPubSpecs();
+    if (changedPubspecs.isEmpty) {
+      _print('No version updates in this commit, exiting...');


"..." indicates more to follow, which seems odd for something that says it's exiting.

Done, remove the exit part.

stuartmorgan · 2021-04-23T16:20:14Z

script/tool/lib/src/publish_plugin_command.dart

+        await baseGitDir.runCommand(<String>['tag', '--sort=-committerdate']);
+    final List<String> existingTags = (existingTagsResult.stdout as String)
+        .split('\n')
+          ..removeWhere((element) => element == '');


stuartmorgan · 2021-04-23T16:25:02Z

script/tool/lib/src/publish_plugin_command.dart

@@ -120,7 +211,50 @@ class PublishPluginCommand extends PluginCommand {
          remoteUrl: remoteUrl,
          shouldPushTag: shouldPushTag);
    }
-    await _finishSuccesfully();
+    _print('Release ${packageDir.basename} successful.');


There aren't any bools for the sub-steps; does that mean they are fatal errors when something goes wrong?

If we try to release 3 packages, an error with one of them presumably shouldn't kill the whole thing, but instead move on to the next, and then at the end list all the successes and failures.

done. Made all sub-steps within publishing a plugin to return a bool instead of throw directly, then the main process will decide to throw depends on the returned value.

script/tool/lib/src/publish_plugin_command.dart

stuartmorgan · 2021-04-23T16:26:40Z

script/tool/lib/src/publish_plugin_command.dart

+    }
+
+    if (pubspec.name == null) {
+      printErrorAndExit(errorMessage: 'Fatal: Package name is null.');


Same concerns here; if this is fatal, then all packages will fail, not just the one that's wrong.

Fixed, commented above.

stuartmorgan · 2021-04-23T16:28:50Z

script/tool/lib/src/publish_plugin_command.dart

+    final String latestTag = existingTags.isNotEmpty
+        ? existingTags
+            .firstWhere((String tag) => tag.split('-v').first == pubspec.name)
+        : '';


You can use orElse here instead of checking isNotEmpty.

In fact, I think without that change the current logic is wrong, since just because the set of tags isn't empty doesn't mean that this particular package has been released before, so it seems like this would throw as soon as we had a new plugin. (Which suggests there's a test case missing.)

Good catch! Thanks! Fixed and added a test for it

script/tool/lib/src/publish_plugin_command.dart

cyanglaz

Updated per review comments. PTAL @stuartmorgan

cyanglaz · 2021-04-23T16:55:44Z

script/tool/lib/src/publish_plugin_command.dart

@@ -64,13 +68,31 @@ class PublishPluginCommand extends PluginCommand {
      // Flutter convention is to use "upstream" for the single source of truth, and "origin" for personal forks.
      defaultsTo: 'upstream',
    );
+    argParser.addFlag(
+      _allFlag,


sounds good! DONE

cyanglaz · 2021-04-23T16:56:29Z

script/tool/lib/src/publish_plugin_command.dart

  StreamSubscription<String> _stdinSubscription;

  @override
  Future<void> run() async {
    final String package = argResults[_packageOption] as String;
-    if (package == null) {
+    final bool all = argResults[_allFlag] as bool;


cyanglaz · 2021-04-23T16:56:35Z

script/tool/lib/src/publish_plugin_command.dart

@@ -111,7 +135,74 @@ class PublishPluginCommand extends PluginCommand {
    }
    _print('Local repo is ready!');

-    final Directory packageDir = _getPackageDir(package);
+    if (all) {
+      await _publishAllPackages(


cyanglaz · 2021-04-23T16:57:14Z

script/tool/lib/src/publish_plugin_command.dart

+    final List<String> changedPubspecs =
+        await gitVersionFinder.getChangedPubSpecs();
+    if (changedPubspecs.isEmpty) {
+      _print('No version updates in this commit, exiting...');


Done, remove the exit part.

cyanglaz · 2021-04-23T16:57:32Z

script/tool/lib/src/publish_plugin_command.dart

+        await baseGitDir.runCommand(<String>['tag', '--sort=-committerdate']);
+    final List<String> existingTags = (existingTagsResult.stdout as String)
+        .split('\n')
+          ..removeWhere((element) => element == '');


cyanglaz · 2021-04-23T18:09:32Z

script/tool/lib/src/publish_plugin_command.dart

@@ -120,7 +211,50 @@ class PublishPluginCommand extends PluginCommand {
          remoteUrl: remoteUrl,
          shouldPushTag: shouldPushTag);
    }
-    await _finishSuccesfully();
+    _print('Release ${packageDir.basename} successful.');


done. Made all sub-steps within publishing a plugin to return a bool instead of throw directly, then the main process will decide to throw depends on the returned value.

script/tool/lib/src/publish_plugin_command.dart

cyanglaz · 2021-04-23T18:22:50Z

script/tool/lib/src/publish_plugin_command.dart

+    }
+
+    if (pubspec.name == null) {
+      printErrorAndExit(errorMessage: 'Fatal: Package name is null.');


Fixed, commented above.

cyanglaz · 2021-04-23T18:40:40Z

script/tool/lib/src/publish_plugin_command.dart

+    final String latestTag = existingTags.isNotEmpty
+        ? existingTags
+            .firstWhere((String tag) => tag.split('-v').first == pubspec.name)
+        : '';


Good catch! Thanks! Fixed and added a test for it

script/tool/lib/src/publish_plugin_command.dart

stuartmorgan · 2021-04-26T18:00:50Z

script/tool/lib/src/publish_plugin_command.dart

+      {String remote,
+      String remoteUrl,
+      bool shouldPushTag,
+      GitDir baseGitDir}) async {


Optional nit: add trailing commas here and below to improve formatting

stuartmorgan · 2021-04-26T18:02:55Z

script/tool/lib/src/publish_plugin_command.dart

+    if (packagesFailed.isNotEmpty) {
+      return false;
+    }
+    return true;


These four lines can be simplified to: return packagesFailed.isEmpty;

stuartmorgan · 2021-04-26T18:06:02Z

script/tool/lib/src/publish_plugin_command.dart

+        if (!needsRelease) {
+          continue;
+        }
+      } on ToolExit catch (e) {


Catching a ToolExit seems potentially very confusing. How about making _checkNeedsRelease return a release/norelease/failure enum?

Yup, sounds good!

stuartmorgan · 2021-04-26T18:07:55Z

script/tool/lib/src/publish_plugin_command.dart

+          ],
+          workingDir: packageDir);
+    } on ToolExit catch (e) {
+      print(e);


Why print and _print?

oops, this was a testing code, removed

stuartmorgan · 2021-04-26T18:08:27Z

script/tool/lib/src/publish_plugin_command.dart

+  Future<bool> _checkGitStatus(Directory packageDir) async {
+    io.ProcessResult statusResult;
+    try {
+      statusResult = await processRunner.runAndExitOnError(


The naming here suggests there's a version that doesn't exit on error, so we wouldn't need to catch ToolExits?

runAndExitOnError does something extra (printing error message in a specific format etc), if we don't use it, then we have to re-write that code. I think we should rename this method to run(bool logError = true, bool exitOnError = true...).

Actually, I just saw that the run method that we have already has a exitOnError flag which doesn't do anything, which suggests that we used to have it like this but somehow refactored into 2 methods. https://github.com/flutter/plugins/blob/master/script/tool/lib/src/common.dart#L507

I'm happy to do the refactoring in a separate PR! Then rebase this on top after that lands.

#3827 @stuartmorgan here you go

stuartmorgan · 2021-04-26T18:08:56Z

script/tool/lib/src/publish_plugin_command.dart

+            workingDir: packagesDir);
+      }
+    } on ToolExit catch (e) {
+      print(e);


Same question as above.

same answer above :)

cyanglaz

@stuartmorgan Fixed all review comments as well as removing all the unnecessary try-catch. PTAL

stuartmorgan

LGTM with nits.

stuartmorgan · 2021-04-30T18:04:56Z

script/tool/lib/src/publish_plugin_command.dart

-  Future<void> _publishPlugin({@required Directory packageDir}) async {
-    await _checkGitStatus(packageDir);
-    await _publish(packageDir);
+  // Returns `true` if needs to release the version, `false` if needs to skip


Fix comment for new return type.

stuartmorgan · 2021-04-30T18:05:14Z

script/tool/lib/src/publish_plugin_command.dart

+    }
+    if (packagesFailed.isNotEmpty) {
+      _print(
+          'Packages failed to release: ${packagesFailed.join(', ')}, see above for details.');


Nit: "Failed to release the following packages:"

stuartmorgan · 2021-04-30T18:06:30Z

script/tool/lib/src/publish_plugin_command.dart

+    if (pubspec.version == null) {
+      _print('No version found. A package that '
+          'intentionally has no version should be marked '
+          '"publish_to: none".');


Nit: Fix the excessive line breaking here?

stuartmorgan · 2021-04-30T18:08:34Z

script/tool/lib/src/publish_plugin_command.dart

+  // The package does not need to be released.
+  noRelease,
+
+  // There's an error when trying to access if the package needs to be released.


Nit: s/access if/determine whether/

stuartmorgan · 2021-04-30T18:12:00Z

script/tool/test/util.dart

@@ -85,14 +87,14 @@ Directory createFakePlugin(
    final Directory exampleDir = pluginDirectory.childDirectory('example')
      ..createSync();
    createFakePubspec(exampleDir,
-        name: '${name}_example', isFlutter: isFlutter);
+        name: '${name}_example', isFlutter: isFlutter, includeVersion: false, publish_to: 'none');


While you're touching the utils, could you fix the naming of this parameter? It should be publishTo.

flutter/plugins#3776)

flutter/plugins#3776) (#81589)

* upstream/master: (383 commits) Add implement and registerWith method to plugins (flutter#3833) Update third_party license checking (flutter#3844) [tool] add `all` and `dry-run` flags to publish-plugin command (flutter#3776) Re-add bin/ to flutter_plugin_tools (flutter#3839) switch from using 'tuneup' to analyze to 'dart analyze' (flutter#3837) [in_app_purchase] Federated iOS implementation (flutter#3832) Prep the tools for publishing (flutter#3836) [in_app_purchase] Make PurchaseDetails.status mandatory (flutter#3831) Fix grammatical error in contributing guide (flutter#3217) [google_sign_in_web] fix README typos. [tool] combine run and runAndExitOnError (flutter#3827) [camera] android-rework part 2: Android auto focus feature (flutter#3796) [in_app_purchase_platform_interface] Added additional fields to ProductDetails (flutter#3826) Move all null safety packages' min dart sdk to 2.12.0 (flutter#3822) [path_provider_*] code cleanup: sort directives (flutter#3823) [in_app_purchase] Implementation of platform interface (flutter#3781) [google_sign_in] Add todo WRT correctly setting X-Goog-AuthUser header (flutter#3819) [tools] fix version check command not working for new packages (flutter#3818) [camera] android-rework part 1: Base classes to support Android Camera features (flutter#3795) fix MD (flutter#3815) ...

…er#3776)

google-cla bot added the cla: yes label Mar 31, 2021

cyanglaz requested a review from stuartmorgan March 31, 2021 19:38

cyanglaz marked this pull request as draft April 1, 2021 01:34

cyanglaz removed the request for review from stuartmorgan April 1, 2021 01:34

cyanglaz mentioned this pull request Apr 1, 2021

[tool] refactor publish plugin command #3779

Merged

add --all and --dry-run to publish-plugin

9ba2d7d

cyanglaz force-pushed the tag_command branch from 3971157 to 9ba2d7d Compare April 21, 2021 18:17

cyanglaz changed the title ~~[ci] add tag-release command~~ [tool] add all and dry-run flags to publish-plugin command Apr 21, 2021

cyanglaz marked this pull request as ready for review April 21, 2021 22:00

cyanglaz requested a review from stuartmorgan April 21, 2021 23:27

stuartmorgan suggested changes Apr 23, 2021

View reviewed changes

cyanglaz commented Apr 23, 2021

View reviewed changes

cyanglaz requested a review from stuartmorgan April 23, 2021 19:16

stuartmorgan suggested changes Apr 26, 2021

View reviewed changes

cyanglaz requested a review from stuartmorgan April 26, 2021 18:40

Chris Yang added 2 commits April 27, 2021 09:36

review fixes

9a760ee

merge master

6055c91

cyanglaz force-pushed the tag_command branch from d7285a1 to 6055c91 Compare April 27, 2021 17:22

cyanglaz commented Apr 27, 2021

View reviewed changes

stuartmorgan approved these changes Apr 30, 2021

View reviewed changes

review

b54688b

cyanglaz added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Apr 30, 2021

fluttergithubbot merged commit a2ce3da into flutter:master Apr 30, 2021

engine-flutter-autoroll mentioned this pull request May 1, 2021

Roll Plugins from 54aad0754cd6 to a2ce3dab4661 (1 revision) flutter/flutter#81589

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 1, 2021

a2ce3da [tool] add all and dry-run flags to publish-plugin command (

beb8f03

flutter/plugins#3776)

fluttergithubbot pushed a commit to flutter/flutter that referenced this pull request May 1, 2021

a2ce3da [tool] add all and dry-run flags to publish-plugin command (

daaefc3

flutter/plugins#3776) (#81589)

fotiDim pushed a commit to fotiDim/plugins that referenced this pull request Sep 13, 2021

[tool] add all and dry-run flags to publish-plugin command (flutt…

6de7b8a

…er#3776)

[tool] add all and dry-run flags to publish-plugin command #3776

[tool] add all and dry-run flags to publish-plugin command #3776

Conversation

cyanglaz commented Mar 31, 2021 • edited Loading

Pre-launch Checklist

cyanglaz commented Apr 1, 2021

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

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

cyanglaz 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

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

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

cyanglaz left a comment

Choose a reason for hiding this comment

stuartmorgan 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

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

[tool] add `all` and `dry-run` flags to publish-plugin command #3776

[tool] add `all` and `dry-run` flags to publish-plugin command #3776

cyanglaz commented Mar 31, 2021 •

edited

Loading