[flutter_tools] refactor packages_autoroller.dart script (#106580)

dev/bots/analyze.dart

@@ -1778,7 +1778,7 @@ const Set<String> kExecutableAllowlist = <String>{
   'dev/bots/docs.sh',
 
   'dev/conductor/bin/conductor',
-  'dev/conductor/bin/roll-packages',
+  'dev/conductor/bin/packages_autoroller',
   'dev/conductor/core/lib/src/proto/compile_proto.sh',
 
   'dev/customer_testing/ci.sh',

dev/conductor/bin/roll-packages → dev/conductor/bin/packages_autoroller

@@ -43,4 +43,4 @@ DART_BIN="$REPO_DIR/bin/dart"
 # Ensure pub get has been run in the repo before running the conductor
 (cd "$REPO_DIR/dev/conductor/core"; "$DART_BIN" pub get 1>&2)
 
-exec "$DART_BIN" --enable-asserts "$REPO_DIR/dev/conductor/core/bin/roll_packages.dart" "$@"
+exec "$DART_BIN" --enable-asserts "$REPO_DIR/dev/conductor/core/bin/packages_autoroller.dart" "$@"

dev/conductor/core/bin/packages_autoroller.dart

@@ -9,6 +9,7 @@ import 'package:conductor_core/conductor_core.dart';
 import 'package:conductor_core/packages_autoroller.dart';
 import 'package:file/file.dart';
 import 'package:file/local.dart';
+import 'package:meta/meta.dart' show visibleForTesting;
 import 'package:platform/platform.dart';
 import 'package:process/process.dart';
 
@@ -17,12 +18,21 @@ const String kGithubClient = 'github-client';
 const String kMirrorRemote = 'mirror-remote';
 const String kUpstreamRemote = 'upstream-remote';
 
-Future<void> main(List<String> args) async {
+Future<void> main(List<String> args) {
+  return run(args);
+}
+
+@visibleForTesting
+Future<void> run(
+  List<String> args, {
+  FileSystem fs = const LocalFileSystem(),
+  ProcessManager processManager = const LocalProcessManager(),
+}) async {
   final ArgParser parser = ArgParser();
   parser.addOption(
     kTokenOption,
-    help: 'GitHub access token env variable name.',
-    defaultsTo: 'GITHUB_TOKEN',
+    help: 'Path to GitHub access token file.',
+    mandatory: true,
   );
   parser.addOption(
     kGithubClient,
@@ -56,12 +66,17 @@ ${parser.usage}
 
   final String mirrorUrl = results[kMirrorRemote]! as String;
   final String upstreamUrl = results[kUpstreamRemote]! as String;
-  const Platform platform = LocalPlatform();
-  final String tokenName = results[kTokenOption]! as String;
-  final String? token = platform.environment[tokenName];
-  if (token == null || token.isEmpty) {
-    throw FormatException(
-      'Tried to read a GitHub access token from env variable \$$tokenName but it was undefined or empty',
+  final String tokenPath = results[kTokenOption]! as String;
+  final File tokenFile = fs.file(tokenPath);
+  if (!tokenFile.existsSync()) {
+    throw ArgumentError(
+      'Provided token path $tokenPath but no file exists at ${tokenFile.absolute.path}',
+    );
+  }
+  final String token = tokenFile.readAsStringSync().trim();
+  if (token.isEmpty) {
+    throw ArgumentError(
+      'Tried to read a GitHub access token from file ${tokenFile.path} but it was empty',
     );
   }
 
@@ -76,7 +91,7 @@ ${parser.usage}
     githubClient: results[kGithubClient] as String? ?? 'gh',
     orgName: _parseOrgName(mirrorUrl),
     token: token,
-    processManager: const LocalProcessManager(),
+    processManager: processManager,
   ).roll();
 }
 
@@ -126,3 +141,8 @@ Directory get _localFlutterRoot {
   );
   return fileSystem.directory(checkoutsDirname);
 }
+
+@visibleForTesting
+void validateTokenFile(String filePath, [FileSystem fs = const LocalFileSystem()]) {
+
+}

dev/conductor/core/lib/src/packages_autoroller.dart

@@ -10,6 +10,7 @@ import 'package:process/process.dart';
 import 'git.dart';
 import 'globals.dart';
 import 'repository.dart';
+import 'stdio.dart';
 
 /// A service for rolling the SDK's pub packages to latest and open a PR upstream.
 class PackageAutoroller {
@@ -19,7 +20,10 @@ class PackageAutoroller {
     required this.framework,
     required this.orgName,
     required this.processManager,
+    this.githubUsername = 'fluttergithubbot',
+    Stdio? stdio,
   }) {
+    this.stdio = stdio ?? VerboseStdio.local();
     if (token.trim().isEmpty) {
       throw Exception('empty token!');
     }
@@ -31,12 +35,16 @@ class PackageAutoroller {
     }
   }
 
+  late final Stdio stdio;
+
   final FrameworkRepository framework;
   final ProcessManager processManager;
 
   /// Path to GitHub CLI client.
   final String githubClient;
 
+  final String githubUsername;
+
   /// GitHub API access token.
   final String token;
 
@@ -63,23 +71,46 @@ This PR was generated by `flutter update-packages --force-upgrade`.
     return name(x);
   })();
 
+  void log(String message) {
+    stdio.printStatus(_redactToken(message));
+  }
+
   /// Name of the GitHub organization to push the feature branch to.
   final String orgName;
 
   Future<void> roll() async {
+    try {
       await authLogin();
-    await updatePackages();
+      final bool openPrAlready = await hasOpenPrs();
+      if (openPrAlready) {
+        // Don't open multiple roll PRs.
+        return;
+      }
+      final bool didUpdate = await updatePackages();
+      if (!didUpdate) {
+        log('Packages are already at latest.');
+        return;
+      }
       await pushBranch();
-    await createPr(
-      repository: await framework.checkoutDirectory,
-    );
+      await createPr(repository: await framework.checkoutDirectory);
       await authLogout();
+    } on Exception catch (exception) {
+      final String message = _redactToken(exception.toString());
+      throw Exception('${exception.runtimeType}: $message');
+    }
   }
 
-  Future<void> updatePackages({
+  // Ensure we don't leak the GitHub token in exception messages
+  String _redactToken(String message) => message.replaceAll(token, '[GitHub TOKEN]');
+
+  /// Attempt to update all pub packages.
+  ///
+  /// Will return whether or not any changes were made.
+  Future<bool> updatePackages({
     bool verbose = true,
-    String author = 'flutter-packages-autoroller <flutter-packages-autoroller@google.com>'
   }) async {
+    final String author = '$githubUsername <$githubUsername@gmail.com>';
+
     await framework.newBranch(await featureBranchName);
     final io.Process flutterProcess = await framework.streamFlutter(<String>[
       if (verbose) '--verbose',
@@ -90,18 +121,26 @@ This PR was generated by `flutter update-packages --force-upgrade`.
     if (exitCode != 0) {
       throw ConductorException('Failed to update packages with exit code $exitCode');
     }
+    // If the git checkout is clean, then pub packages are already at latest that cleanly resolve.
+    if (await framework.gitCheckoutClean()) {
+      return false;
+    }
     await framework.commit(
       'roll packages',
       addFirst: true,
       author: author,
     );
+    return true;
   }
 
   Future<void> pushBranch() async {
+    final String projectName = framework.mirrorRemote!.url.split(r'/').last;
+    // Encode the token into the remote URL for authentication to work
+    final String remote = 'https://$token@$hostname/$orgName/$projectName';
     await framework.pushRef(
       fromRef: await featureBranchName,
       toRef: await featureBranchName,
-      remote: framework.mirrorRemote!.url,
+      remote: remote,
     );
   }
 
@@ -123,7 +162,7 @@ This PR was generated by `flutter update-packages --force-upgrade`.
         'https',
         '--with-token',
       ],
-      stdin: token,
+      stdin: '$token\n',
     );
   }
 
@@ -151,6 +190,8 @@ This PR was generated by `flutter update-packages --force-upgrade`.
         '$orgName:${await featureBranchName}',
         '--base',
         base,
+        '--label',
+        'tool',
         if (draft)
           '--draft',
       ],
@@ -165,13 +206,16 @@ This PR was generated by `flutter update-packages --force-upgrade`.
     ]);
   }
 
-  Future<void> cli(
+  /// Run a sub-process with the GitHub CLI client.
+  ///
+  /// Will return STDOUT of the sub-process.
+  Future<String> cli(
     List<String> args, {
     bool allowFailure = false,
     String? stdin,
     String? workingDirectory,
   }) async {
-    print('Executing "$githubClient ${args.join(' ')}" in $workingDirectory');
+    log('Executing "$githubClient ${args.join(' ')}" in $workingDirectory');
     final io.Process process = await processManager.start(
       <String>[githubClient, ...args],
       workingDirectory: workingDirectory,
@@ -203,6 +247,36 @@ This PR was generated by `flutter update-packages --force-upgrade`.
         args,
       );
     }
-    print(stdout);
+    log(stdout);
+    return stdout;
+  }
+
+  Future<bool> hasOpenPrs() async {
+    // gh pr list --author christopherfujino --repo flutter/flutter --state open --json number
+    final String openPrString = await cli(<String>[
+      'pr',
+      'list',
+      '--author',
+      githubUsername,
+      '--repo',
+      'flutter/flutter',
+      '--state',
+      'open',
+      // We are only interested in pub rolls, not devicelab flaky PRs
+      '--label',
+      'tool',
+      // Return structured JSON with the PR numbers of open PRs
+      '--json',
+      'number',
+    ]);
+
+    // This will be an array of objects, one for each open PR.
+    final List<Object?> openPrs = json.decode(openPrString) as List<Object?>;
+
+    if (openPrs.isNotEmpty) {
+      log('$githubUsername already has open tool PRs:\n$openPrs');
+      return true;
+    }
+    return false;
   }
 }

dev/conductor/core/test/packages_autoroller_test.dart

@@ -12,6 +12,7 @@ import 'package:file/memory.dart';
 import 'package:platform/platform.dart';
 
 import './common.dart';
+import '../bin/packages_autoroller.dart' show run;
 
 void main() {
   const String flutterRoot = '/flutter';
@@ -61,10 +62,11 @@ void main() {
       framework: framework,
       orgName: orgName,
       processManager: processManager,
+      stdio: stdio,
     );
   });
 
-  test('can roll with correct inputs', () async {
+  test('GitHub token is redacted from exceptions while pushing', () async {
     final StreamController<List<int>> controller =
         StreamController<List<int>>();
     processManager.addCommands(<FakeCommand>[
@@ -150,7 +152,7 @@ void main() {
         'commit',
         '--message',
         'roll packages',
-        '--author="flutter-packages-autoroller <flutter-packages-autoroller@google.com>"',
+        '--author="fluttergithubbot <fluttergithubbot@google.com>"',
       ]),
       const FakeCommand(command: <String>[
         'git',
@@ -160,7 +162,282 @@ void main() {
       const FakeCommand(command: <String>[
         'git',
         'push',
+        'https://$token@github.com/$orgName/flutter.git',
+        'packages-autoroller-branch-1:packages-autoroller-branch-1',
+      ], exitCode: 1, stderr: 'Authentication error!'),
+    ]);
+    await expectLater(
+      () async {
+        final Future<void> rollFuture = autoroller.roll();
+        await controller.stream.drain();
+        await rollFuture;
+      },
+      throwsA(isA<Exception>().having(
+        (Exception exc) => exc.toString(),
+        'message',
+        isNot(contains(token)),
+      )),
+    );
+  });
+
+  test('Does not attempt to roll if bot already has an open PR', () async {
+    final StreamController<List<int>> controller =
+        StreamController<List<int>>();
+    processManager.addCommands(<FakeCommand>[
+      FakeCommand(command: const <String>[
+        'gh',
+        'auth',
+        'login',
+        '--hostname',
+        'github.com',
+        '--git-protocol',
+        'https',
+        '--with-token',
+      ], stdin: io.IOSink(controller.sink)),
+      const FakeCommand(command: <String>[
+        'gh',
+        'pr',
+        'list',
+        '--author',
+        'fluttergithubbot',
+        '--repo',
+        'flutter/flutter',
+        '--state',
+        'open',
+        '--label',
+        'tool',
+        '--json',
+        'number',
+      // Non empty array means there are open PRs by the bot with the tool label
+      // We expect no further commands to be run
+      ], stdout: '[{"number": 123}]'),
+    ]);
+    final Future<void> rollFuture = autoroller.roll();
+    await controller.stream.drain();
+    await rollFuture;
+    expect(processManager, hasNoRemainingExpectations);
+    expect(stdio.stdout, contains('fluttergithubbot already has open tool PRs'));
+    expect(stdio.stdout, contains(r'[{number: 123}]'));
+  });
+
+  test('Does not commit or create a PR if no changes were made', () async {
+    final StreamController<List<int>> controller =
+        StreamController<List<int>>();
+    processManager.addCommands(<FakeCommand>[
+      FakeCommand(command: const <String>[
+        'gh',
+        'auth',
+        'login',
+        '--hostname',
+        'github.com',
+        '--git-protocol',
+        'https',
+        '--with-token',
+      ], stdin: io.IOSink(controller.sink)),
+      const FakeCommand(command: <String>[
+        'gh',
+        'pr',
+        'list',
+        '--author',
+        'fluttergithubbot',
+        '--repo',
+        'flutter/flutter',
+        '--state',
+        'open',
+        '--label',
+        'tool',
+        '--json',
+        'number',
+      // Returns empty array, as there are no other open roll PRs from the bot
+      ], stdout: '[]'),
+      const FakeCommand(command: <String>[
+        'git',
+        'clone',
+        '--origin',
+        'upstream',
+        '--',
+        FrameworkRepository.defaultUpstream,
+        '$checkoutsParentDirectory/flutter_conductor_checkouts/framework',
+      ]),
+      const FakeCommand(command: <String>[
+        'git',
+        'remote',
+        'add',
+        'mirror',
+        mirrorUrl,
+      ]),
+      const FakeCommand(command: <String>[
+        'git',
+        'fetch',
+        'mirror',
+      ]),
+      const FakeCommand(command: <String>[
+        'git',
+        'checkout',
+        FrameworkRepository.defaultBranch,
+      ]),
+      const FakeCommand(command: <String>[
+        'git',
+        'rev-parse',
+        'HEAD',
+      ], stdout: 'deadbeef'),
+      const FakeCommand(command: <String>[
+        'git',
+        'ls-remote',
+        '--heads',
+        'mirror',
+      ]),
+      const FakeCommand(command: <String>[
+        'git',
+        'checkout',
+        '-b',
+        'packages-autoroller-branch-1',
+      ]),
+      const FakeCommand(command: <String>[
+        '$checkoutsParentDirectory/flutter_conductor_checkouts/framework/bin/flutter',
+        'help',
+      ]),
+      const FakeCommand(command: <String>[
+        '$checkoutsParentDirectory/flutter_conductor_checkouts/framework/bin/flutter',
+        '--verbose',
+        'update-packages',
+        '--force-upgrade',
+      ]),
+      // Because there is no stdout to git status, the script should exit cleanly here
+      const FakeCommand(command: <String>[
+        'git',
+        'status',
+        '--porcelain',
+      ]),
+    ]);
+    final Future<void> rollFuture = autoroller.roll();
+    await controller.stream.drain();
+    await rollFuture;
+    expect(processManager, hasNoRemainingExpectations);
+  });
+
+  test('can roll with correct inputs', () async {
+    final StreamController<List<int>> controller =
+        StreamController<List<int>>();
+    processManager.addCommands(<FakeCommand>[
+      FakeCommand(command: const <String>[
+        'gh',
+        'auth',
+        'login',
+        '--hostname',
+        'github.com',
+        '--git-protocol',
+        'https',
+        '--with-token',
+      ], stdin: io.IOSink(controller.sink)),
+      const FakeCommand(command: <String>[
+        'gh',
+        'pr',
+        'list',
+        '--author',
+        'fluttergithubbot',
+        '--repo',
+        'flutter/flutter',
+        '--state',
+        'open',
+        '--label',
+        'tool',
+        '--json',
+        'number',
+      // Returns empty array, as there are no other open roll PRs from the bot
+      ], stdout: '[]'),
+      const FakeCommand(command: <String>[
+        'git',
+        'clone',
+        '--origin',
+        'upstream',
+        '--',
+        FrameworkRepository.defaultUpstream,
+        '$checkoutsParentDirectory/flutter_conductor_checkouts/framework',
+      ]),
+      const FakeCommand(command: <String>[
+        'git',
+        'remote',
+        'add',
+        'mirror',
         mirrorUrl,
+      ]),
+      const FakeCommand(command: <String>[
+        'git',
+        'fetch',
+        'mirror',
+      ]),
+      const FakeCommand(command: <String>[
+        'git',
+        'checkout',
+        FrameworkRepository.defaultBranch,
+      ]),
+      const FakeCommand(command: <String>[
+        'git',
+        'rev-parse',
+        'HEAD',
+      ], stdout: 'deadbeef'),
+      const FakeCommand(command: <String>[
+        'git',
+        'ls-remote',
+        '--heads',
+        'mirror',
+      ]),
+      const FakeCommand(command: <String>[
+        'git',
+        'checkout',
+        '-b',
+        'packages-autoroller-branch-1',
+      ]),
+      const FakeCommand(command: <String>[
+        '$checkoutsParentDirectory/flutter_conductor_checkouts/framework/bin/flutter',
+        'help',
+      ]),
+      const FakeCommand(command: <String>[
+        '$checkoutsParentDirectory/flutter_conductor_checkouts/framework/bin/flutter',
+        '--verbose',
+        'update-packages',
+        '--force-upgrade',
+      ]),
+      const FakeCommand(command: <String>[
+        'git',
+        'status',
+        '--porcelain',
+      ], stdout: '''
+ M packages/foo/pubspec.yaml
+ M packages/bar/pubspec.yaml
+ M dev/integration_tests/test_foo/pubspec.yaml
+'''),
+      const FakeCommand(command: <String>[
+        'git',
+        'status',
+        '--porcelain',
+      ], stdout: '''
+ M packages/foo/pubspec.yaml
+ M packages/bar/pubspec.yaml
+ M dev/integration_tests/test_foo/pubspec.yaml
+'''),
+      const FakeCommand(command: <String>[
+        'git',
+        'add',
+        '--all',
+      ]),
+      const FakeCommand(command: <String>[
+        'git',
+        'commit',
+        '--message',
+        'roll packages',
+        '--author="fluttergithubbot <fluttergithubbot@gmail.com>"',
+      ]),
+      const FakeCommand(command: <String>[
+        'git',
+        'rev-parse',
+        'HEAD',
+      ], stdout: '000deadbeef'),
+      const FakeCommand(command: <String>[
+        'git',
+        'push',
+        'https://$token@github.com/$orgName/flutter.git',
         'packages-autoroller-branch-1:packages-autoroller-branch-1',
       ]),
       const FakeCommand(command: <String>[
@@ -175,6 +452,8 @@ void main() {
         'flutter-roller:packages-autoroller-branch-1',
         '--base',
         FrameworkRepository.defaultBranch,
+        '--label',
+        'tool',
       ]),
       const FakeCommand(command: <String>[
         'gh',
@@ -187,7 +466,48 @@ void main() {
     final Future<void> rollFuture = autoroller.roll();
     final String givenToken =
         await controller.stream.transform(const Utf8Decoder()).join();
-    expect(givenToken, token);
+    expect(givenToken.trim(), token);
     await rollFuture;
+    expect(processManager, hasNoRemainingExpectations);
+  });
+
+  group('command argument validations', () {
+    const String tokenPath = '/path/to/token';
+    const String mirrorRemote = 'https://githost.com/org/project';
+
+    test('validates that file exists at --token option', () async {
+      await expectLater(
+        () => run(
+          <String>['--token', tokenPath, '--mirror-remote', mirrorRemote],
+          fs: fileSystem,
+          processManager: processManager,
+        ),
+        throwsA(isA<ArgumentError>().having(
+          (ArgumentError err) => err.message,
+          'message',
+          contains('Provided token path $tokenPath but no file exists at'),
+        )),
+      );
+      expect(processManager, hasNoRemainingExpectations);
+    });
+
+    test('validates that the token file is not empty', () async {
+      fileSystem.file(tokenPath)
+        ..createSync(recursive: true)
+        ..writeAsStringSync('');
+      await expectLater(
+        () => run(
+          <String>['--token', tokenPath, '--mirror-remote', mirrorRemote],
+          fs: fileSystem,
+          processManager: processManager,
+        ),
+        throwsA(isA<ArgumentError>().having(
+          (ArgumentError err) => err.message,
+          'message',
+          contains('Tried to read a GitHub access token from file $tokenPath but it was empty'),
+        )),
+      );
+      expect(processManager, hasNoRemainingExpectations);
+    });
   });
 }