[flutter_tools] refactor packages_autoroller.dart script (#106580)
diff --git a/dev/bots/analyze.dart b/dev/bots/analyze.dart
index c14bfcb..c11a820 100644
--- a/dev/bots/analyze.dart
+++ b/dev/bots/analyze.dart
@@ -1778,7 +1778,7 @@
'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 97%
rename from dev/conductor/bin/roll-packages
rename to dev/conductor/bin/packages_autoroller
index 231d544..c424296 100755
--- a/dev/conductor/bin/roll-packages
+++ b/dev/conductor/bin/packages_autoroller
@@ -43,4 +43,4 @@
# 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 d69c846..db9635e 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/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 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 @@
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 @@
githubClient: results[kGithubClient] as String? ?? 'gh',
orgName: _parseOrgName(mirrorUrl),
token: token,
- processManager: const LocalProcessManager(),
+ processManager: processManager,
).roll();
}
@@ -126,3 +141,8 @@
);
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 e05d494..8e277f8 100644
--- a/dev/conductor/core/lib/src/packages_autoroller.dart
+++ b/dev/conductor/core/lib/src/packages_autoroller.dart
@@ -10,6 +10,7 @@
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 @@
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 @@
}
}
+ 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 @@
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 {
- 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<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 @@
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 @@
'https',
'--with-token',
],
- stdin: token,
+ stdin: '$token\n',
);
}
@@ -151,6 +190,8 @@
'$orgName:${await featureBranchName}',
'--base',
base,
+ '--label',
+ 'tool',
if (draft)
'--draft',
],
@@ -165,13 +206,16 @@
]);
}
- 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 @@
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;
}
}
diff --git a/dev/conductor/core/test/packages_autoroller_test.dart b/dev/conductor/core/test/packages_autoroller_test.dart
index 9799261..c1828d1 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:platform/platform.dart';
import './common.dart';
+import '../bin/packages_autoroller.dart' show run;
void main() {
const String flutterRoot = '/flutter';
@@ -61,10 +62,11 @@
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 @@
'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 @@
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 @@
'flutter-roller:packages-autoroller-branch-1',
'--base',
FrameworkRepository.defaultBranch,
+ '--label',
+ 'tool',
]),
const FakeCommand(command: <String>[
'gh',
@@ -187,7 +466,48 @@
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);
+ });
});
}
