diff --git a/dev/bots/analyze.dart b/dev/bots/analyze.dart index c14bfcbf3d52..c11a82069897 100644 --- a/dev/bots/analyze.dart +++ b/dev/bots/analyze.dart @@ -1778,7 +1778,7 @@ const Set kExecutableAllowlist = { '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', diff --git a/dev/conductor/bin/roll-packages b/dev/conductor/bin/packages_autoroller similarity index 98% rename from dev/conductor/bin/roll-packages rename to dev/conductor/bin/packages_autoroller index 231d54453c3c..c42429600912 100755 --- a/dev/conductor/bin/roll-packages +++ b/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" "$@" diff --git a/dev/conductor/core/bin/packages_autoroller.dart b/dev/conductor/core/bin/packages_autoroller.dart index d69c846e32d7..db9635e73a30 100644 --- a/dev/conductor/core/bin/packages_autoroller.dart +++ b/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 main(List args) async { +Future main(List args) { + return run(args); +} + +@visibleForTesting +Future run( + List 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()]) { + +} diff --git a/dev/conductor/core/lib/src/packages_autoroller.dart b/dev/conductor/core/lib/src/packages_autoroller.dart index e05d4941a0ea..8e277f8bc712 100644 --- a/dev/conductor/core/lib/src/packages_autoroller.dart +++ b/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 roll() async { - await authLogin(); - await updatePackages(); - await pushBranch(); - await createPr( - repository: await framework.checkoutDirectory, - ); - await authLogout(); + try { + await authLogin(); + 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 authLogout(); + } on Exception catch (exception) { + final String message = _redactToken(exception.toString()); + throw Exception('${exception.runtimeType}: $message'); + } } - Future 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 updatePackages({ bool verbose = true, - String author = 'flutter-packages-autoroller ' }) async { + final String author = '$githubUsername <$githubUsername@gmail.com>'; + await framework.newBranch(await featureBranchName); final io.Process flutterProcess = await framework.streamFlutter([ 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 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 cli( + /// Run a sub-process with the GitHub CLI client. + /// + /// Will return STDOUT of the sub-process. + Future cli( List 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( [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 hasOpenPrs() async { + // gh pr list --author christopherfujino --repo flutter/flutter --state open --json number + final String openPrString = await cli([ + '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 openPrs = json.decode(openPrString) as List; + + if (openPrs.isNotEmpty) { + log('$githubUsername already has open tool PRs:\n$openPrs'); + return true; + } + return false; } } diff --git a/dev/conductor/core/test/packages_autoroller_test.dart b/dev/conductor/core/test/packages_autoroller_test.dart index 97992615dd47..c1828d13f70a 100644 --- a/dev/conductor/core/test/packages_autoroller_test.dart +++ b/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> controller = StreamController>(); processManager.addCommands([ @@ -150,7 +152,7 @@ void main() { 'commit', '--message', 'roll packages', - '--author="flutter-packages-autoroller "', + '--author="fluttergithubbot "', ]), const FakeCommand(command: [ 'git', @@ -160,7 +162,282 @@ void main() { const FakeCommand(command: [ '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 rollFuture = autoroller.roll(); + await controller.stream.drain(); + await rollFuture; + }, + throwsA(isA().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> controller = + StreamController>(); + processManager.addCommands([ + FakeCommand(command: const [ + 'gh', + 'auth', + 'login', + '--hostname', + 'github.com', + '--git-protocol', + 'https', + '--with-token', + ], stdin: io.IOSink(controller.sink)), + const FakeCommand(command: [ + '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 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> controller = + StreamController>(); + processManager.addCommands([ + FakeCommand(command: const [ + 'gh', + 'auth', + 'login', + '--hostname', + 'github.com', + '--git-protocol', + 'https', + '--with-token', + ], stdin: io.IOSink(controller.sink)), + const FakeCommand(command: [ + '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: [ + 'git', + 'clone', + '--origin', + 'upstream', + '--', + FrameworkRepository.defaultUpstream, + '$checkoutsParentDirectory/flutter_conductor_checkouts/framework', + ]), + const FakeCommand(command: [ + 'git', + 'remote', + 'add', + 'mirror', + mirrorUrl, + ]), + const FakeCommand(command: [ + 'git', + 'fetch', + 'mirror', + ]), + const FakeCommand(command: [ + 'git', + 'checkout', + FrameworkRepository.defaultBranch, + ]), + const FakeCommand(command: [ + 'git', + 'rev-parse', + 'HEAD', + ], stdout: 'deadbeef'), + const FakeCommand(command: [ + 'git', + 'ls-remote', + '--heads', + 'mirror', + ]), + const FakeCommand(command: [ + 'git', + 'checkout', + '-b', + 'packages-autoroller-branch-1', + ]), + const FakeCommand(command: [ + '$checkoutsParentDirectory/flutter_conductor_checkouts/framework/bin/flutter', + 'help', + ]), + const FakeCommand(command: [ + '$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: [ + 'git', + 'status', + '--porcelain', + ]), + ]); + final Future rollFuture = autoroller.roll(); + await controller.stream.drain(); + await rollFuture; + expect(processManager, hasNoRemainingExpectations); + }); + + test('can roll with correct inputs', () async { + final StreamController> controller = + StreamController>(); + processManager.addCommands([ + FakeCommand(command: const [ + 'gh', + 'auth', + 'login', + '--hostname', + 'github.com', + '--git-protocol', + 'https', + '--with-token', + ], stdin: io.IOSink(controller.sink)), + const FakeCommand(command: [ + '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: [ + 'git', + 'clone', + '--origin', + 'upstream', + '--', + FrameworkRepository.defaultUpstream, + '$checkoutsParentDirectory/flutter_conductor_checkouts/framework', + ]), + const FakeCommand(command: [ + 'git', + 'remote', + 'add', + 'mirror', mirrorUrl, + ]), + const FakeCommand(command: [ + 'git', + 'fetch', + 'mirror', + ]), + const FakeCommand(command: [ + 'git', + 'checkout', + FrameworkRepository.defaultBranch, + ]), + const FakeCommand(command: [ + 'git', + 'rev-parse', + 'HEAD', + ], stdout: 'deadbeef'), + const FakeCommand(command: [ + 'git', + 'ls-remote', + '--heads', + 'mirror', + ]), + const FakeCommand(command: [ + 'git', + 'checkout', + '-b', + 'packages-autoroller-branch-1', + ]), + const FakeCommand(command: [ + '$checkoutsParentDirectory/flutter_conductor_checkouts/framework/bin/flutter', + 'help', + ]), + const FakeCommand(command: [ + '$checkoutsParentDirectory/flutter_conductor_checkouts/framework/bin/flutter', + '--verbose', + 'update-packages', + '--force-upgrade', + ]), + const FakeCommand(command: [ + '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: [ + '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: [ + 'git', + 'add', + '--all', + ]), + const FakeCommand(command: [ + 'git', + 'commit', + '--message', + 'roll packages', + '--author="fluttergithubbot "', + ]), + const FakeCommand(command: [ + 'git', + 'rev-parse', + 'HEAD', + ], stdout: '000deadbeef'), + const FakeCommand(command: [ + 'git', + 'push', + 'https://$token@github.com/$orgName/flutter.git', 'packages-autoroller-branch-1:packages-autoroller-branch-1', ]), const FakeCommand(command: [ @@ -175,6 +452,8 @@ void main() { 'flutter-roller:packages-autoroller-branch-1', '--base', FrameworkRepository.defaultBranch, + '--label', + 'tool', ]), const FakeCommand(command: [ 'gh', @@ -187,7 +466,48 @@ void main() { final Future 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( + ['--token', tokenPath, '--mirror-remote', mirrorRemote], + fs: fileSystem, + processManager: processManager, + ), + throwsA(isA().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( + ['--token', tokenPath, '--mirror-remote', mirrorRemote], + fs: fileSystem, + processManager: processManager, + ), + throwsA(isA().having( + (ArgumentError err) => err.message, + 'message', + contains('Tried to read a GitHub access token from file $tokenPath but it was empty'), + )), + ); + expect(processManager, hasNoRemainingExpectations); + }); }); }