diff --git a/dev/conductor/core/lib/src/git.dart b/dev/conductor/core/lib/src/git.dart index 7fb5addc0c..3a353a59ff 100644 --- a/dev/conductor/core/lib/src/git.dart +++ b/dev/conductor/core/lib/src/git.dart @@ -9,6 +9,12 @@ import 'package:process/process.dart'; import './globals.dart'; /// A wrapper around git process calls that can be mocked for unit testing. +/// +/// The `Git` class is a relatively (compared to `Repository`) lightweight +/// abstraction over invocations to the `git` cli tool. The main +/// motivation for creating this class was so that it could be overridden in +/// tests. However, now that tests rely on the [FakeProcessManager] this +/// abstraction is redundant. class Git { const Git(this.processManager); diff --git a/dev/conductor/core/lib/src/next.dart b/dev/conductor/core/lib/src/next.dart index f5f8d048ae..d06ecba753 100644 --- a/dev/conductor/core/lib/src/next.dart +++ b/dev/conductor/core/lib/src/next.dart @@ -18,6 +18,11 @@ const String kStateOption = 'state-file'; const String kYesFlag = 'yes'; /// Command to proceed from one [pb.ReleasePhase] to the next. +/// +/// After `conductor start`, the rest of the release steps are initiated by the +/// user via `conductor next`. Thus this command's behavior is conditional upon +/// which phase of the release the user is currently in. This is implemented +/// with a switch case statement. class NextCommand extends Command { NextCommand({ required this.checkouts, diff --git a/dev/conductor/core/lib/src/repository.dart b/dev/conductor/core/lib/src/repository.dart index fd505a4792..bac53f4baf 100644 --- a/dev/conductor/core/lib/src/repository.dart +++ b/dev/conductor/core/lib/src/repository.dart @@ -60,6 +60,39 @@ class Remote { } /// A source code repository. +/// +/// This class is an abstraction over a git +/// repository on the local disk. Ideally this abstraction would hide from +/// the outside libraries what git calls were needed to either read or update +/// data in the underlying repository. In practice, most of the bugs in the +/// conductor codebase are related to the git calls made from this and its +/// subclasses. +/// +/// Two factors that make this code more complicated than it would otherwise +/// need to be are: +/// 1. That any particular invocation of the conductor may or may not already +/// have the git checkout present on disk, depending on what commands were +/// previously run; and +/// 2. The need to provide overrides for integration tests (in particular +/// the ability to mark a [Repository] instance as a [localUpstream] made +/// integration tests more hermetic, at the cost of complexity in the +/// implementation). +/// +/// The only way to simplify the first factor would be to change the behavior of +/// the conductor tool to be a long-lived dart process that keeps all of its +/// state in memory and blocks on user input. This would add the constraint that +/// the user would need to keep the process running for the duration of a +/// release, which could potentially take multiple days and users could not +/// manually change the state of the release process (via editing the JSON +/// config file). However, these may be reasonable trade-offs to make the +/// codebase simpler and easier to reason about. +/// +/// The way to simplify the second factor would be to not put any special +/// handling in this library for integration tests. This would make integration +/// tests more difficult/less hermetic, but the production code more reliable. +/// This is probably the right trade-off to make, as the integration tests were +/// still not hermetic or reliable, and the main integration test was ultimately +/// deleted in #84354. abstract class Repository { Repository({ required this.name, diff --git a/dev/conductor/core/lib/src/start.dart b/dev/conductor/core/lib/src/start.dart index c02bc1615e..d96c30ec81 100644 --- a/dev/conductor/core/lib/src/start.dart +++ b/dev/conductor/core/lib/src/start.dart @@ -34,6 +34,16 @@ const String kVersionOverrideOption = 'version-override'; const String kGithubUsernameOption = 'github-username'; /// Command to print the status of the current Flutter release. +/// +/// This command has many required options which the user must provide +/// via command line arguments (or optionally environment variables). +/// +/// This command is the one with the worst user experience (as the user has to +/// carefully type out many different options into their terminal) and the one +/// that would benefit the most from a GUI frontend. This command will +/// optionally read its options from an environment variable to facilitate a workflow +/// in which configuration is provided by editing a bash script that sets environment +/// variables and then invokes the conductor tool. class StartCommand extends Command { StartCommand({ required this.checkouts, diff --git a/dev/conductor/core/lib/src/state.dart b/dev/conductor/core/lib/src/state.dart index c4bf2a815b..4dd4a51ee5 100644 --- a/dev/conductor/core/lib/src/state.dart +++ b/dev/conductor/core/lib/src/state.dart @@ -33,6 +33,19 @@ const String stablePostReleaseMsg = """ '\t 4. Post announcement flutter release hotline chat room', '\t\t Chatroom: ${globals.flutterReleaseHotline}', """; +// The helper functions in `state.dart` wrap the code-generated dart files in +// `lib/src/proto/`. The most interesting of these functions is: + +// * `pb.ConductorState readStateFromFile(File)` - uses the code generated +// `.mergeFromProto3Json()` method to deserialize the JSON content from the +// config file into a Dart instance of the `ConductorState` class. +// * `void writeStateFromFile(File, pb.ConductorState, List)` +// - similarly calls the `.toProto3Json()` method to serialize a +// * `ConductorState` instance to a JSON string which is then written to disk. +// `String phaseInstructions(pb.ConductorState state)` - returns instructions +// for what the user is supposed to do next based on `state.currentPhase`. +// * `String presentState(pb.ConductorState state)` - pretty print the state file. +// This is a little easier to read than the raw JSON. String luciConsoleLink(String channel, String groupName) { assert( diff --git a/dev/conductor/core/lib/src/stdio.dart b/dev/conductor/core/lib/src/stdio.dart index 11276d048e..8777b91f40 100644 --- a/dev/conductor/core/lib/src/stdio.dart +++ b/dev/conductor/core/lib/src/stdio.dart @@ -6,6 +6,14 @@ import 'dart:io' as io; import 'package:meta/meta.dart'; +/// An interface for presenting text output to the user. +/// +/// Although this could have been simplified by calling `print()` +/// from the tool, this abstraction allows unit tests to verify output +/// and allows a GUI frontend to provide an alternative implementation. +/// +/// User input probably should be part of this class–however it is currently +/// part of context.dart. abstract class Stdio { final List logs = [];