[tool] Switch PR description overrides over to labels (#6145)

script/tool/CHANGELOG.md

@@ -1,3 +1,10 @@
+## 0.9.0
+
+* Replaces PR-description-based version/changelog/breaking change check
+  overrides in `version-check` with label-based overrides using a new
+  `pr-labels` flag, since we don't actually have reliable access to the
+  PR description in checks.
+  
 ## 0.8.10
 
 - Adds a new `remove-dev-dependencies` command to remove `dev_dependencies`

script/tool/lib/src/version_check_command.dart

@@ -123,47 +123,49 @@ class VersionCheckCommand extends PackageLoopingCommand {
             '(e.g., PR description or commit message).\n\n'
             'If supplied, this is used to allow overrides to some version '
             'checks.');
+    argParser.addOption(_prLabelsArg,
+        help: 'A comma-separated list of labels associated with this PR, '
+            'if applicable.\n\n'
+            'If supplied, this may be to allow overrides to some version '
+            'checks.');
     argParser.addFlag(_checkForMissingChanges,
         help: 'Validates that changes to packages include CHANGELOG and '
             'version changes unless they meet an established exemption.\n\n'
-            'If used with --$_changeDescriptionFile, this is should only be '
+            'If used with --$_prLabelsArg, this is should only be '
-            'used in pre-submit CI checks, to  prevent the possibility of '
+            'used in pre-submit CI checks, to  prevent post-submit breakage '
-            'post-submit breakage if an override justification is not '
+            'when labels are no longer applicable.',
-            'transferred into the commit message.',
         hide: true);
     argParser.addFlag(_ignorePlatformInterfaceBreaks,
         help: 'Bypasses the check that platform interfaces do not contain '
             'breaking changes.\n\n'
             'This is only intended for use in post-submit CI checks, to '
-            'prevent the possibility of post-submit breakage if a change '
+            'prevent post-submit breakage when overriding the check with '
-            'description justification is not transferred into the commit '
+            'labels. Pre-submit checks should always use '
-            'message. Pre-submit checks should always use '
+            '--$_prLabelsArg instead.',
-            '--$_changeDescriptionFile instead.',
         hide: true);
   }
 
   static const String _againstPubFlag = 'against-pub';
   static const String _changeDescriptionFile = 'change-description-file';
+  static const String _prLabelsArg = 'pr-labels';
   static const String _checkForMissingChanges = 'check-for-missing-changes';
   static const String _ignorePlatformInterfaceBreaks =
       'ignore-platform-interface-breaks';
 
-  /// The string that must be in [_changeDescriptionFile] to allow a breaking
+  /// The label that must be on a PR to allow a breaking
   /// change to a platform interface.
-  static const String _breakingChangeJustificationMarker =
+  static const String _breakingChangeOverrideLabel =
-      '## Breaking change justification';
+      'override: allow breaking change';
 
-  /// The string that must be at the start of a line in [_changeDescriptionFile]
+  /// The label that must be on a PR to allow skipping a version change for a PR
-  /// to allow skipping a version change for a PR that would normally require
+  /// that would normally require one.
-  /// one.
+  static const String _missingVersionChangeOverrideLabel =
-  static const String _missingVersionChangeJustificationMarker =
+      'override: no versioning needed';
-      'No version change:';
 
-  /// The string that must be at the start of a line in [_changeDescriptionFile]
+  /// The label that must be on a PR to allow skipping a CHANGELOG change for a
-  /// to allow skipping a CHANGELOG change for a PR that would normally require
+  /// PR that would normally require one.
-  /// one.
+  static const String _missingChangelogChangeOverrideLabel =
-  static const String _missingChangelogChangeJustificationMarker =
+      'override: no changelog needed';
-      'No CHANGELOG change:';
 
   final PubVersionFinder _pubVersionFinder;
 
@@ -172,6 +174,7 @@ class VersionCheckCommand extends PackageLoopingCommand {
   late final List<String> _changedFiles;
 
   late final String _changeDescription = _loadChangeDescription();
+  late final Set<String> _prLabels = _getPRLabels();
 
   @override
   final String name = 'version-check';
@@ -498,18 +501,25 @@ ${indentation}The first version listed in CHANGELOG.md is $fromChangeLog.
       return true;
     }
 
-    if (_getChangeDescription().contains(_breakingChangeJustificationMarker)) {
+    if (_prLabels.contains(_breakingChangeOverrideLabel)) {
       logWarning(
           '${indentation}Allowing breaking change to ${package.displayName} '
-          'due to "$_breakingChangeJustificationMarker" in the change '
+          'due to the "$_breakingChangeOverrideLabel" label.');
-          'description.');
       return true;
     }
 
     return false;
   }
 
-  String _getChangeDescription() => _changeDescription;
+  /// Returns the labels associated with this PR, if any, or an empty set
+  /// if that flag is not provided.
+  Set<String> _getPRLabels() {
+    final String labels = getStringArg(_prLabelsArg);
+    if (labels.isEmpty) {
+      return <String>{};
+    }
+    return labels.split(',').map((String label) => label.trim()).toSet();
+  }
 
   /// Returns the contents of the file pointed to by [_changeDescriptionFile],
   /// or an empty string if that flag is not provided.
@@ -569,44 +579,38 @@ ${indentation}The first version listed in CHANGELOG.md is $fromChangeLog.
     }
 
     if (state.needsVersionChange) {
-      final String changeDescription = _getChangeDescription();
+      if (_prLabels.contains(_missingVersionChangeOverrideLabel)) {
-      if (changeDescription.split('\n').any((String line) =>
+        logWarning('Ignoring lack of version change due to the '
-          line.startsWith(_missingVersionChangeJustificationMarker))) {
+            '"$_missingVersionChangeOverrideLabel" label.');
-        logWarning('Ignoring lack of version change due to '
+      } else if (_isAllowedDependabotChange(package, _changeDescription)) {
-            '"$_missingVersionChangeJustificationMarker" in the '
-            'change description.');
-      } else if (_isAllowedDependabotChange(package, changeDescription)) {
         logWarning('Ignoring lack of version change for Dependabot change to '
             'a known internal dependency.');
       } else {
         printError(
             'No version change found, but the change to this package could '
             'not be verified to be exempt from version changes according to '
-            'repository policy. If this is a false positive, please '
+            'repository policy. If this is a false positive, please comment in '
-            'add a line starting with\n'
+            'the PR to explain why the PR is exempt, and add (or ask your '
-            '$_missingVersionChangeJustificationMarker\n'
+            'reviewer to add) the "$_missingVersionChangeOverrideLabel" '
-            'to your PR description with an explanation of why it is exempt.');
+            'label.');
         return 'Missing version change';
       }
     }
 
     if (!state.hasChangelogChange) {
-      final String changeDescription = _getChangeDescription();
+      if (_prLabels.contains(_missingChangelogChangeOverrideLabel)) {
-      if (changeDescription.split('\n').any((String line) =>
+        logWarning('Ignoring lack of CHANGELOG update due to the '
-          line.startsWith(_missingChangelogChangeJustificationMarker))) {
+            '"$_missingChangelogChangeOverrideLabel" label.');
-        logWarning('Ignoring lack of CHANGELOG update due to '
+      } else if (_isAllowedDependabotChange(package, _changeDescription)) {
-            '"$_missingChangelogChangeJustificationMarker" in the '
-            'change description.');
-      } else if (_isAllowedDependabotChange(package, changeDescription)) {
         logWarning('Ignoring lack of CHANGELOG update for Dependabot change to '
             'a known internal dependency.');
       } else {
         printError(
             'No CHANGELOG change found. If this PR needs an exemption from '
             'the standard policy of listing all changes in the CHANGELOG, '
-            'please add a line starting with\n'
+            'comment in the PR to explain why the PR is exempt, and add (or '
-            '$_missingChangelogChangeJustificationMarker\n'
+            'ask your reviewer to add) the '
-            'to your PR description with an explanation of why.');
+            '"$_missingChangelogChangeOverrideLabel" label.');
         return 'Missing CHANGELOG change';
       }
     }
@@ -617,6 +621,11 @@ ${indentation}The first version listed in CHANGELOG.md is $fromChangeLog.
   /// Returns true if [changeDescription] matches a Dependabot change for a
   /// dependency roll that should bypass the normal version and CHANGELOG change
   /// checks (for dependencies that are known not to have client impact).
+  ///
+  /// Depending on CI, [changeDescription] may either be the PR description, or
+  /// the description of the last commit (see for example discussion in
+  /// https://github.com/cirruslabs/cirrus-ci-docs/issues/1029), so this needs
+  /// to handle both.
   bool _isAllowedDependabotChange(
       RepositoryPackage package, String changeDescription) {
     // Espresso exports some dependencies that are normally just internal test
@@ -629,8 +638,7 @@ ${indentation}The first version listed in CHANGELOG.md is $fromChangeLog.
     // any other PR, to identify Dependabot PRs.
     const String dependabotPRDescriptionMarker =
         'Dependabot commands and options';
-    // The same thing, but for the Dependabot commit message, to work around
+    // The same thing, but for the Dependabot commit message.
-    // https://github.com/cirruslabs/cirrus-ci-docs/issues/1029.
     const String dependabotCommitMessageMarker =
         'Signed-off-by: dependabot[bot]';
     // Expression to extract the name of the dependency being updated.

script/tool/pubspec.yaml

@@ -1,7 +1,7 @@
 name: flutter_plugin_tools
 description: Productivity utils for flutter/plugins and flutter/packages
 repository: https://github.com/flutter/plugins/tree/main/script/tool
-version: 0.8.10
+version: 0.9.0
 
 dependencies:
   args: ^2.1.0

script/tool/test/version_check_command_test.dart

@@ -364,35 +364,25 @@ void main() {
           ]));
     });
 
-    test('allows breaking changes to platform interfaces with explanation',
+    test('allows breaking changes to platform interfaces with override label',
         () async {
       createFakePlugin('plugin_platform_interface', packagesDir,
           version: '2.0.0');
       processRunner.mockProcessesForExecutable['git-show'] = <io.Process>[
         MockProcess(stdout: 'version: 1.0.0'),
       ];
-      final File changeDescriptionFile =
-          fileSystem.file('change_description.txt');
-      changeDescriptionFile.writeAsStringSync('''
-Some general PR description
 
-## Breaking change justification
-
-This is necessary because of X, Y, and Z
-
-## Another section''');
       final List<String> output = await runCapturingPrint(runner, <String>[
         'version-check',
         '--base-sha=main',
-        '--change-description-file=${changeDescriptionFile.path}'
+        '--pr-labels=some label,override: allow breaking change,another-label'
       ]);
 
       expect(
         output,
         containsAllInOrder(<Matcher>[
           contains('Allowing breaking change to plugin_platform_interface '
-              'due to "## Breaking change justification" in the change '
+              'due to the "override: allow breaking change" label.'),
-              'description.'),
           contains('Ran for 1 package(s) (1 with warnings)'),
         ]),
       );
@@ -408,42 +398,6 @@ This is necessary because of X, Y, and Z
           ]));
     });
 
-    test('throws if a nonexistent change description file is specified',
-        () async {
-      createFakePlugin('plugin_platform_interface', packagesDir,
-          version: '2.0.0');
-      processRunner.mockProcessesForExecutable['git-show'] = <io.Process>[
-        MockProcess(stdout: 'version: 1.0.0'),
-      ];
-
-      Error? commandError;
-      final List<String> output = await runCapturingPrint(runner, <String>[
-        'version-check',
-        '--base-sha=main',
-        '--change-description-file=a_missing_file.txt'
-      ], errorHandler: (Error e) {
-        commandError = e;
-      });
-
-      expect(commandError, isA<ToolExit>());
-      expect(
-        output,
-        containsAllInOrder(<Matcher>[
-          contains('No such file: a_missing_file.txt'),
-        ]),
-      );
-      expect(
-          processRunner.recordedCalls,
-          containsAllInOrder(const <ProcessCall>[
-            ProcessCall(
-                'git-show',
-                <String>[
-                  'main:packages/plugin_platform_interface/pubspec.yaml'
-                ],
-                null)
-          ]));
-    });
-
     test('allows breaking changes to platform interfaces with bypass flag',
         () async {
       createFakePlugin('plugin_platform_interface', packagesDir,
@@ -965,7 +919,7 @@ packages/plugin/CHANGELOG.md
         );
       });
 
-      test('allows missing version change with justification', () async {
+      test('allows missing version change with override label', () async {
         final RepositoryPackage plugin =
             createFakePlugin('plugin', packagesDir, version: '1.0.0');
 
@@ -985,23 +939,16 @@ packages/plugin/pubspec.yaml
 '''),
         ];
 
-        final File changeDescriptionFile =
-            fileSystem.file('change_description.txt');
-        changeDescriptionFile.writeAsStringSync('''
-Some general PR description
-
-No version change: Code change is only to implementation comments.
-''');
         final List<String> output =
             await _runWithMissingChangeDetection(<String>[
-          '--change-description-file=${changeDescriptionFile.path}'
+          '--pr-labels=some label,override: no versioning needed,another-label'
         ]);
 
         expect(
           output,
           containsAllInOrder(<Matcher>[
-            contains('Ignoring lack of version change due to '
+            contains('Ignoring lack of version change due to the '
-                '"No version change:" in the change description.'),
+                '"override: no versioning needed" label.'),
           ]),
         );
       });
@@ -1124,28 +1071,56 @@ packages/plugin/example/lib/foo.dart
 '''),
         ];
 
-        final File changeDescriptionFile =
-            fileSystem.file('change_description.txt');
-        changeDescriptionFile.writeAsStringSync('''
-Some general PR description
-
-No CHANGELOG change: Code change is only to implementation comments.
-''');
         final List<String> output =
             await _runWithMissingChangeDetection(<String>[
-          '--change-description-file=${changeDescriptionFile.path}'
+          '--pr-labels=some label,override: no changelog needed,another-label'
         ]);
 
         expect(
           output,
           containsAllInOrder(<Matcher>[
-            contains('Ignoring lack of CHANGELOG update due to '
+            contains('Ignoring lack of CHANGELOG update due to the '
-                '"No CHANGELOG change:" in the change description.'),
+                '"override: no changelog needed" label.'),
           ]),
         );
       });
 
       group('dependabot', () {
+        test('throws if a nonexistent change description file is specified',
+            () async {
+          final RepositoryPackage plugin =
+              createFakePlugin('plugin', packagesDir, version: '1.0.0');
+
+          const String changelog = '''
+## 1.0.0
+* Some changes.
+''';
+          plugin.changelogFile.writeAsStringSync(changelog);
+          processRunner.mockProcessesForExecutable['git-show'] = <io.Process>[
+            MockProcess(stdout: 'version: 1.0.0'),
+          ];
+          processRunner.mockProcessesForExecutable['git-diff'] = <io.Process>[
+            MockProcess(stdout: '''
+packages/plugin/android/build.gradle
+'''),
+          ];
+
+          Error? commandError;
+          final List<String> output = await _runWithMissingChangeDetection(
+              <String>['--change-description-file=a_missing_file.txt'],
+              errorHandler: (Error e) {
+            commandError = e;
+          });
+
+          expect(commandError, isA<ToolExit>());
+          expect(
+            output,
+            containsAllInOrder(<Matcher>[
+              contains('No such file: a_missing_file.txt'),
+            ]),
+          );
+        });
+
         test('allows missing version and CHANGELOG change for mockito',
             () async {
           final RepositoryPackage plugin =
@@ -1308,8 +1283,6 @@ packages/plugin/android/build.gradle
           );
         });
 
-        // Tests workaround for
-        // https://github.com/cirruslabs/cirrus-ci-docs/issues/1029.
         test('allow list works for commit messages', () async {
           final RepositoryPackage plugin =
               createFakePlugin('plugin', packagesDir, version: '1.0.0');