diff --git a/dev/conductor/core/lib/src/context.dart b/dev/conductor/core/lib/src/context.dart new file mode 100644 index 0000000000..3d4e5ee110 --- /dev/null +++ b/dev/conductor/core/lib/src/context.dart @@ -0,0 +1,54 @@ +// Copyright 2014 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'package:file/file.dart' show File; + +import 'globals.dart'; +import 'proto/conductor_state.pb.dart' as pb; +import 'repository.dart'; +import 'state.dart'; +import 'stdio.dart' show Stdio; + +/// Interface for shared functionality across all sub-commands. +/// +/// Different frontends (e.g. CLI vs desktop) can share [Context]s, although +/// methods for capturing user interaction may be overridden. +abstract class Context { + const Context({ + required this.checkouts, + required this.stateFile, + }); + + final Checkouts checkouts; + final File stateFile; + Stdio get stdio => checkouts.stdio; + + /// Confirm an action with the user before proceeding. + /// + /// The default implementation reads from STDIN. This can be overriden in UI + /// implementations that capture user interaction differently. + bool prompt(String message) { + stdio.write('${message.trim()} (y/n) '); + final String response = stdio.readLineSync().trim(); + final String firstChar = response[0].toUpperCase(); + if (firstChar == 'Y') { + return true; + } + if (firstChar == 'N') { + return false; + } + throw ConductorException( + 'Unknown user input (expected "y" or "n"): $response', + ); + } + + /// Save the release's [state]. + /// + /// This can be overridden by frontends that may not persist the state to + /// disk, and/or may need to call additional update hooks each time the state + /// is updated. + void updateState(pb.ConductorState state, [List logs = const []]) { + writeStateToFile(stateFile, state, logs); + } +} diff --git a/dev/conductor/core/lib/src/next.dart b/dev/conductor/core/lib/src/next.dart index 29ef3c4d40..29d76076b2 100644 --- a/dev/conductor/core/lib/src/next.dart +++ b/dev/conductor/core/lib/src/next.dart @@ -4,13 +4,13 @@ import 'package:args/command_runner.dart'; import 'package:file/file.dart' show File; -import 'package:meta/meta.dart' show visibleForTesting, visibleForOverriding; -import './globals.dart'; -import './proto/conductor_state.pb.dart' as pb; -import './proto/conductor_state.pbenum.dart'; -import './repository.dart'; -import './state.dart' as state_import; -import './stdio.dart'; + +import 'context.dart'; +import 'globals.dart'; +import 'proto/conductor_state.pb.dart' as pb; +import 'proto/conductor_state.pbenum.dart'; +import 'repository.dart'; +import 'state.dart' as state_import; const String kStateOption = 'state-file'; const String kYesFlag = 'yes'; @@ -68,21 +68,21 @@ class NextCommand extends Command { /// /// Any calls to functions that cause side effects are wrapped in methods to /// allow overriding in unit tests. -class NextContext { - NextContext({ +class NextContext extends Context { + const NextContext({ required this.autoAccept, required this.force, - required this.checkouts, - required this.stateFile, - }); + required Checkouts checkouts, + required File stateFile, + }) : super( + checkouts: checkouts, + stateFile: stateFile, + ); final bool autoAccept; final bool force; - final Checkouts checkouts; - final File stateFile; Future run(pb.ConductorState state) async { - final Stdio stdio = checkouts.stdio; const List finishedStates = [ CherrypickState.COMPLETED, CherrypickState.ABANDONED, @@ -142,9 +142,8 @@ class NextContext { } if (autoAccept == false) { final bool response = prompt( - 'Are you ready to push your engine branch to the repository ' - '${state.engine.mirror.url}?', - stdio, + 'Are you ready to push your engine branch to the repository ' + '${state.engine.mirror.url}?', ); if (!response) { stdio.printError('Aborting command.'); @@ -169,8 +168,7 @@ class NextContext { if (autoAccept == false) { // TODO(fujino): actually test if binaries have been codesigned on macOS final bool response = prompt( - 'Has CI passed for the engine PR and binaries been codesigned?', - stdio, + 'Has CI passed for the engine PR and binaries been codesigned?', ); if (!response) { stdio.printError('Aborting command.'); @@ -209,14 +207,14 @@ class NextContext { final String engineRevision = await engine.reverseParse('HEAD'); final Remote upstream = Remote( - name: RemoteName.upstream, - url: state.framework.upstream.url, + name: RemoteName.upstream, + url: state.framework.upstream.url, ); final FrameworkRepository framework = FrameworkRepository( - checkouts, - initialRef: state.framework.workingBranch, - upstreamRemote: upstream, - previousCheckoutLocation: state.framework.checkoutPath, + checkouts, + initialRef: state.framework.workingBranch, + upstreamRemote: upstream, + previousCheckoutLocation: state.framework.checkoutPath, ); // Check if the current candidate branch is enabled @@ -277,9 +275,8 @@ class NextContext { if (autoAccept == false) { final bool response = prompt( - 'Are you ready to push your framework branch to the repository ' - '${state.framework.mirror.url}?', - stdio, + 'Are you ready to push your framework branch to the repository ' + '${state.framework.mirror.url}?', ); if (!response) { stdio.printError('Aborting command.'); @@ -289,10 +286,10 @@ class NextContext { } await framework.pushRef( - fromRef: 'HEAD', - // Explicitly create new branch - toRef: 'refs/heads/${state.framework.workingBranch}', - remote: state.framework.mirror.name, + fromRef: 'HEAD', + // Explicitly create new branch + toRef: 'refs/heads/${state.framework.workingBranch}', + remote: state.framework.mirror.name, ); break; case pb.ReleasePhase.PUBLISH_VERSION: @@ -312,9 +309,8 @@ class NextContext { final String headRevision = await framework.reverseParse('HEAD'); if (autoAccept == false) { final bool response = prompt( - 'Are you ready to tag commit $headRevision as ${state.releaseVersion}\n' - 'and push to remote ${state.framework.upstream.url}?', - stdio, + 'Are you ready to tag commit $headRevision as ${state.releaseVersion}\n' + 'and push to remote ${state.framework.upstream.url}?', ); if (!response) { stdio.printError('Aborting command.'); @@ -347,10 +343,7 @@ class NextContext { dryRun: true, ); - final bool response = prompt( - 'Are you ready to publish this release?', - stdio, - ); + final bool response = prompt('Are you ready to publish this release?'); if (!response) { stdio.printError('Aborting command.'); updateState(state, stdio.logs); @@ -370,10 +363,7 @@ class NextContext { '\t$kLuciPackagingConsoleLink', ); if (autoAccept == false) { - final bool response = prompt( - 'Have all packaging builds finished successfully?', - stdio, - ); + final bool response = prompt('Have all packaging builds finished successfully?'); if (!response) { stdio.printError('Aborting command.'); updateState(state, stdio.logs); @@ -391,30 +381,4 @@ class NextContext { updateState(state, stdio.logs); } - - /// Save the release's [state]. - /// - /// This can be overridden by frontends that may not persist the state to - /// disk, and/or may need to call additional update hooks each time the state - /// is updated. - @visibleForOverriding - void updateState(pb.ConductorState state, [List logs = const []]) { - state_import.writeStateToFile(stateFile, state, logs); - } - - @visibleForTesting - bool prompt(String message, Stdio stdio) { - stdio.write('${message.trim()} (y/n) '); - final String response = stdio.readLineSync().trim(); - final String firstChar = response[0].toUpperCase(); - if (firstChar == 'Y') { - return true; - } - if (firstChar == 'N') { - return false; - } - throw ConductorException( - 'Unknown user input (expected "y" or "n"): $response', - ); - } } diff --git a/dev/conductor/core/lib/src/start.dart b/dev/conductor/core/lib/src/start.dart index 7c57fd48f1..cbf8cebfc7 100644 --- a/dev/conductor/core/lib/src/start.dart +++ b/dev/conductor/core/lib/src/start.dart @@ -6,18 +6,18 @@ import 'package:args/args.dart'; import 'package:args/command_runner.dart'; import 'package:file/file.dart'; import 'package:fixnum/fixnum.dart'; -import 'package:meta/meta.dart' show visibleForOverriding; import 'package:platform/platform.dart'; import 'package:process/process.dart'; -import './git.dart'; -import './globals.dart'; -import './proto/conductor_state.pb.dart' as pb; -import './proto/conductor_state.pbenum.dart' show ReleasePhase; -import './repository.dart'; -import './state.dart' as state_import; -import './stdio.dart'; -import './version.dart'; +import 'context.dart'; +import 'git.dart'; +import 'globals.dart'; +import 'proto/conductor_state.pb.dart' as pb; +import 'proto/conductor_state.pbenum.dart' show ReleasePhase; +import 'repository.dart'; +import 'state.dart' as state_import; +import 'stdio.dart'; +import 'version.dart'; const String kCandidateOption = 'candidate-branch'; const String kDartRevisionOption = 'dart-revision'; @@ -207,7 +207,6 @@ class StartCommand extends Command { processManager: processManager, releaseChannel: releaseChannel, stateFile: stateFile, - stdio: stdio, force: force, ); return context.run(); @@ -217,10 +216,9 @@ class StartCommand extends Command { /// Context for starting a new release. /// /// This is a frontend-agnostic implementation. -class StartContext { +class StartContext extends Context { StartContext({ required this.candidateBranch, - required this.checkouts, required this.dartRevision, required this.engineCherrypickRevisions, required this.engineMirror, @@ -232,13 +230,17 @@ class StartContext { required this.incrementLetter, required this.processManager, required this.releaseChannel, - required this.stateFile, - required this.stdio, + required Checkouts checkouts, + required File stateFile, this.force = false, - }) : git = Git(processManager); + }) : + git = Git(processManager), + super( + checkouts: checkouts, + stateFile: stateFile, + ); final String candidateBranch; - final Checkouts checkouts; final String? dartRevision; final List engineCherrypickRevisions; final String engineMirror; @@ -251,8 +253,6 @@ class StartContext { final Git git; final ProcessManager processManager; final String releaseChannel; - final File stateFile; - final Stdio stdio; /// If validations should be overridden. final bool force; @@ -410,16 +410,6 @@ class StartContext { stdio.printStatus(state_import.presentState(state)); } - /// Save the release's [state]. - /// - /// This can be overridden by frontends that may not persist the state to - /// disk, and/or may need to call additional update hooks each time the state - /// is updated. - @visibleForOverriding - void updateState(pb.ConductorState state, List logs) { - state_import.writeStateToFile(stateFile, state, logs); - } - /// Determine this release's version number from the [lastVersion] and the [incrementLetter]. Version calculateNextVersion(Version lastVersion) { if (incrementLetter == 'm') { @@ -457,7 +447,18 @@ class StartContext { candidateBranch, FrameworkRepository.defaultBranch, ); + final bool response = prompt( + 'About to tag the release candidate branch branchpoint of $branchPoint ' + 'as $requestedVersion and push it to ${framework.upstreamRemote.url}. ' + 'Is this correct?', + ); + + if (!response) { + throw ConductorException('Aborting command.'); + } + stdio.printStatus('Applying the tag $requestedVersion at the branch point $branchPoint'); + await framework.tag( branchPoint, requestedVersion.toString(), diff --git a/dev/conductor/core/test/common.dart b/dev/conductor/core/test/common.dart index f37c861832..93821b93b2 100644 --- a/dev/conductor/core/test/common.dart +++ b/dev/conductor/core/test/common.dart @@ -34,7 +34,7 @@ class TestStdio extends Stdio { TestStdio({ this.verbose = false, List? stdin, - }) : _stdin = stdin ?? []; + }) : stdin = stdin ?? []; String get error => logs.where((String log) => log.startsWith(r'[error] ')).join('\n'); @@ -43,15 +43,14 @@ class TestStdio extends Stdio { }).join('\n'); final bool verbose; - late final List _stdin; - List get stdin => _stdin; + final List stdin; @override String readLineSync() { - if (_stdin.isEmpty) { + if (stdin.isEmpty) { throw Exception('Unexpected call to readLineSync!'); } - return _stdin.removeAt(0); + return stdin.removeAt(0); } } diff --git a/dev/conductor/core/test/next_test.dart b/dev/conductor/core/test/next_test.dart index a1dfed957a..a519ead623 100644 --- a/dev/conductor/core/test/next_test.dart +++ b/dev/conductor/core/test/next_test.dart @@ -8,6 +8,7 @@ import 'package:conductor_core/src/proto/conductor_state.pb.dart' as pb; import 'package:conductor_core/src/proto/conductor_state.pbenum.dart' show ReleasePhase; import 'package:conductor_core/src/repository.dart'; import 'package:conductor_core/src/state.dart'; +import 'package:conductor_core/src/stdio.dart'; import 'package:file/file.dart'; import 'package:file/memory.dart'; import 'package:platform/platform.dart'; @@ -1054,6 +1055,27 @@ void main() { }); group('prompt', () { + test('can be overridden for different frontend implementations', () { + final FileSystem fileSystem = MemoryFileSystem.test(); + final Stdio stdio = _UnimplementedStdio.instance; + final Checkouts checkouts = Checkouts( + fileSystem: fileSystem, + parentDirectory: fileSystem.directory('/'), + platform: FakePlatform(), + processManager: FakeProcessManager.empty(), + stdio: stdio, + ); + final _TestNextContext context = _TestNextContext( + checkouts: checkouts, + stateFile: fileSystem.file('/statefile.json'), + ); + + final bool response = context.prompt( + 'A prompt that will immediately be agreed to', + ); + expect(response, true); + }); + test('throws if user inputs character that is not "y" or "n"', () { final FileSystem fileSystem = MemoryFileSystem.test(); final TestStdio stdio = TestStdio( @@ -1075,13 +1097,64 @@ void main() { ); expect( - () => context.prompt('Asking a question?', stdio), + () => context.prompt('Asking a question?'), throwsExceptionWith('Unknown user input (expected "y" or "n")'), ); }); }); } +/// A [Stdio] that will throw an exception if any of its methods are called. +class _UnimplementedStdio implements Stdio { + const _UnimplementedStdio(); + + static const _UnimplementedStdio _instance = _UnimplementedStdio(); + static _UnimplementedStdio get instance => _instance; + + Never _throw() => throw Exception('Unimplemented!'); + + @override + List get logs => _throw(); + + @override + void printError(String message) => _throw(); + + @override + void printWarning(String message) => _throw(); + + @override + void printStatus(String message) => _throw(); + + @override + void printTrace(String message) => _throw(); + + @override + void write(String message) => _throw(); + + @override + String readLineSync() => _throw(); +} + +class _TestNextContext extends NextContext { + const _TestNextContext({ + bool autoAccept = false, + bool force = false, + required File stateFile, + required Checkouts checkouts, + }) : super( + autoAccept: autoAccept, + force: force, + checkouts: checkouts, + stateFile: stateFile, + ); + + @override + bool prompt(String message) { + // always say yes + return true; + } +} + void _initializeCiYamlFile( File file, { List? enabledBranches, diff --git a/dev/conductor/core/test/start_test.dart b/dev/conductor/core/test/start_test.dart index 255aa886d3..c33400a7a4 100644 --- a/dev/conductor/core/test/start_test.dart +++ b/dev/conductor/core/test/start_test.dart @@ -306,6 +306,7 @@ void main() { }); test('creates state file if provided correct inputs', () async { + stdio.stdin.add('y'); // accept prompt from ensureBranchPointTagged() const String revision2 = 'def789'; const String revision3 = '123abc'; const String branchPointRevision='deadbeef';