Skip to content

[flutter_conductor] fix initialref to explicitly include remote name #96481

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
fluttergithubbot merged 3 commits into from
Jan 13, 2022
Merged

[flutter_conductor] fix initialref to explicitly include remote name #96481

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4de6658
fix initialref to explicitly include remote name
christopherfujino Jan 11, 2022
b70b07a
stop updating ciyaml
christopherfujino Jan 11, 2022
3b8da40
fix analysis
christopherfujino Jan 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 0 additions & 31 deletions dev/conductor/core/lib/src/next.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,22 +101,6 @@ class NextContext extends Context {
upstreamRemote: upstream,
previousCheckoutLocation: state.engine.checkoutPath,
);
// check if the candidate branch is enabled in .ci.yaml
final CiYaml engineCiYaml = await engine.ciYaml;
if (!engineCiYaml.enabledBranches.contains(state.engine.candidateBranch)) {
engineCiYaml.enableBranch(state.engine.candidateBranch);
// commit
final String revision = await engine.commit(
'add branch ${state.engine.candidateBranch} to enabled_branches in .ci.yaml',
addFirst: true,
);
// append to list of cherrypicks so we know a PR is required
state.engine.cherrypicks.add(pb.Cherrypick(
appliedRevision: revision,
state: pb.CherrypickState.COMPLETED,
));
}

if (!state_import.requiresEnginePR(state)) {
stdio.printStatus(
'This release has no engine cherrypicks. No Engine PR is necessary.\n',
Expand Down Expand Up @@ -213,21 +197,6 @@ class NextContext extends Context {
previousCheckoutLocation: state.framework.checkoutPath,
);

// Check if the current candidate branch is enabled
if (!(await framework.ciYaml).enabledBranches.contains(state.framework.candidateBranch)) {
(await framework.ciYaml).enableBranch(state.framework.candidateBranch);
// commit
final String revision = await framework.commit(
'add branch ${state.framework.candidateBranch} to enabled_branches in .ci.yaml',
addFirst: true,
);
// append to list of cherrypicks so we know a PR is required
state.framework.cherrypicks.add(pb.Cherrypick(
appliedRevision: revision,
state: pb.CherrypickState.COMPLETED,
));
}

stdio.printStatus('Rolling new engine hash $engineRevision to framework checkout...');
final bool needsCommit = await framework.updateEngineRevision(engineRevision);
if (needsCommit) {
Expand Down
42 changes: 0 additions & 42 deletions dev/conductor/core/lib/src/repository.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -844,46 +844,4 @@ class CiYaml {
/// This is not cached as the contents can be written to while the conductor
/// is running.
YamlMap get contents => loadYaml(stringContents) as YamlMap;

List<String> get enabledBranches {
final YamlList yamlList = contents['enabled_branches'] as YamlList;
return yamlList.map<String>((dynamic element) {
return element as String;
}).toList();
}

static final RegExp _enabledBranchPattern = RegExp(r'enabled_branches:');

/// Update this .ci.yaml file with the given branch name.
///
/// The underlying [File] is written to, but not committed to git. This method
/// will throw a [ConductorException] if the [branchName] is already present
/// in the file or if the file does not have an "enabled_branches:" field.
void enableBranch(String branchName) {
final List<String> newStrings = <String>[];
if (enabledBranches.contains(branchName)) {
throw ConductorException('${file.path} already contains the branch $branchName');
}
if (!_enabledBranchPattern.hasMatch(stringContents)) {
throw ConductorException(
'Did not find the expected string "enabled_branches:" in the file ${file.path}',
);
}
final List<String> lines = stringContents.split('\n');
bool insertedCurrentBranch = false;
for (final String line in lines) {
// Every existing line should be copied to the new Yaml
newStrings.add(line);
if (insertedCurrentBranch) {
continue;
}
if (_enabledBranchPattern.hasMatch(line)) {
insertedCurrentBranch = true;
// Indent two spaces
final String indent = ' ' * 2;
newStrings.add('$indent- ${branchName.trim()}');
}
}
file.writeAsStringSync(newStrings.join('\n'), flush: true);
}
}
4 changes: 2 additions & 2 deletions dev/conductor/core/lib/src/start.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ class StartContext extends Context {
}) : git = Git(processManager),
engine = EngineRepository(
checkouts,
initialRef: candidateBranch,
initialRef: 'upstream/$candidateBranch',

@godofredoc godofredoc Jan 11, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will we get an error during the checkout if the branch already exists in the fork?

@christopherfujino christopherfujino Jan 12, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By namespacing by the upstream remote name, git will disregard the fork's branches. This is an unambiguous reference, and will only error if the git cannot fetch the given branch from the upstream remote (either because the branch name that was input was wrong or because it couldn't reach the server).

if you only provide the branch name, git will check all remotes and find which has the branch in question, which was what happened in #96425.

@godofredoc godofredoc Jan 12, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks for the explanation.

upstreamRemote: Remote(
name: RemoteName.upstream,
url: engineUpstream,
Expand All @@ -246,7 +246,7 @@ class StartContext extends Context {
),
), framework = FrameworkRepository(
checkouts,
initialRef: candidateBranch,
initialRef: 'upstream/$candidateBranch',
upstreamRemote: Remote(
name: RemoteName.upstream,
url: frameworkUpstream,
Expand Down
81 changes: 4 additions & 77 deletions dev/conductor/core/test/next_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ void main() {
const String remoteUrl = 'https://github.com/org/repo.git';
const String revision1 = 'd3af60d18e01fcb36e0c0fa06c8502e4935ed095';
const String revision2 = 'f99555c1e1392bf2a8135056b9446680c2af4ddf';
const String revision3 = '98a5ca242b9d270ce000b26309b8a3cdc9c89df5';
const String revision4 = '280e23318a0d8341415c66aa32581352a421d974';
const String releaseVersion = '1.2.0-3.0.pre';
const String releaseChannel = 'beta';
Expand Down Expand Up @@ -82,12 +81,7 @@ void main() {

group('APPLY_ENGINE_CHERRYPICKS to CODESIGN_ENGINE_BINARIES', () {
test('does not prompt user and updates currentPhase if there are no engine cherrypicks', () async {
final FakeProcessManager processManager = FakeProcessManager.list(<FakeCommand>[
const FakeCommand(command: <String>['git', 'fetch', 'upstream']),
const FakeCommand(
command: <String>['git', 'checkout', workingBranch],
),
]);
final FakeProcessManager processManager = FakeProcessManager.empty();
final FakePlatform platform = FakePlatform(
environment: <String, String>{
'HOME': <String>['path', 'to', 'home'].join(localPathSeparator),
Expand Down Expand Up @@ -145,24 +139,7 @@ void main() {
final File ciYaml = fileSystem.file('$checkoutsParentDirectory/engine/.ci.yaml')
..createSync(recursive: true);
_initializeCiYamlFile(ciYaml);
final FakeProcessManager processManager = FakeProcessManager.list(<FakeCommand>[
const FakeCommand(command: <String>['git', 'fetch', 'upstream']),
const FakeCommand(command: <String>['git', 'checkout', workingBranch]),
const FakeCommand(
command: <String>['git', 'status', '--porcelain'],
stdout: 'MM blah',
),
const FakeCommand(command: <String>['git', 'add', '--all']),
const FakeCommand(command: <String>[
'git',
'commit',
"--message='add branch $candidateBranch to enabled_branches in .ci.yaml'",
]),
const FakeCommand(
command: <String>['git', 'rev-parse', 'HEAD'],
stdout: revision2,
),
]);
final FakeProcessManager processManager = FakeProcessManager.empty();
final FakePlatform platform = FakePlatform(
environment: <String, String>{
'HOME': <String>['path', 'to', 'home'].join(localPathSeparator),
Expand Down Expand Up @@ -228,16 +205,6 @@ void main() {
_initializeCiYamlFile(file);
},
),
const FakeCommand(
command: <String>['git', 'status', '--porcelain'],
stdout: 'MM .ci.yaml',
),
const FakeCommand(command: <String>['git', 'add', '--all']),
const FakeCommand(command: <String>['git', 'commit', "--message='add branch $candidateBranch to enabled_branches in .ci.yaml'"]),
const FakeCommand(
command: <String>['git', 'rev-parse', 'HEAD'],
stdout: revision2,
),
const FakeCommand(command: <String>['git', 'push', 'mirror', 'HEAD:refs/heads/$workingBranch']),
]);
final FakePlatform platform = FakePlatform(
Expand Down Expand Up @@ -355,6 +322,7 @@ void main() {
fileSystem.file(stateFile),
);

expect(processManager, hasNoRemainingExpectations);
expect(stdio.stdout, contains('Has CI passed for the engine PR and binaries been codesigned? (y/n) '));
expect(finalState.currentPhase, ReleasePhase.CODESIGN_ENGINE_BINARIES);
expect(stdio.error.contains('Aborting command.'), true);
Expand Down Expand Up @@ -392,6 +360,7 @@ void main() {
fileSystem.file(stateFile),
);

expect(processManager, hasNoRemainingExpectations);
expect(stdio.stdout, contains('Has CI passed for the engine PR and binaries been codesigned? (y/n) '));
expect(finalState.currentPhase, ReleasePhase.APPLY_FRAMEWORK_CHERRYPICKS);
});
Expand Down Expand Up @@ -524,20 +493,6 @@ void main() {
_initializeCiYamlFile(file);
},
),
const FakeCommand(
command: <String>['git', 'status', '--porcelain'],
stdout: 'MM /path/to/.ci.yaml',
),
const FakeCommand(command: <String>['git', 'add', '--all']),
const FakeCommand(command: <String>[
'git',
'commit',
"--message='add branch $candidateBranch to enabled_branches in .ci.yaml'",
]),
const FakeCommand(
command: <String>['git', 'rev-parse', 'HEAD'],
stdout: revision3,
),
const FakeCommand(
command: <String>['git', 'status', '--porcelain'],
stdout: 'MM /path/to/engine.version',
Expand Down Expand Up @@ -614,20 +569,6 @@ void main() {
),
const FakeCommand(command: <String>['git', 'fetch', 'upstream']),
const FakeCommand(command: <String>['git', 'checkout', workingBranch]),
const FakeCommand(
command: <String>['git', 'status', '--porcelain'],
stdout: 'MM path/to/.ci.yaml',
),
const FakeCommand(command: <String>['git', 'add', '--all']),
const FakeCommand(command: <String>[
'git',
'commit',
"--message='add branch $candidateBranch to enabled_branches in .ci.yaml'",
]),
const FakeCommand(
command: <String>['git', 'rev-parse', 'HEAD'],
stdout: revision3,
),
const FakeCommand(
command: <String>['git', 'status', '--porcelain'],
stdout: 'MM path/to/engine.version',
Expand Down Expand Up @@ -697,20 +638,6 @@ void main() {
stdout: 'MM path/to/.ci.yaml',
),
const FakeCommand(command: <String>['git', 'add', '--all']),
const FakeCommand(command: <String>[
'git',
'commit',
"--message='add branch $candidateBranch to enabled_branches in .ci.yaml'",
]),
const FakeCommand(
command: <String>['git', 'rev-parse', 'HEAD'],
stdout: revision3,
),
const FakeCommand(
command: <String>['git', 'status', '--porcelain'],
stdout: 'MM path/to/engine.version',
),
const FakeCommand(command: <String>['git', 'add', '--all']),
const FakeCommand(command: <String>[
'git',
'commit',
Expand Down
124 changes: 0 additions & 124 deletions dev/conductor/core/test/repository_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,130 +384,6 @@ vars = {
);
});

test('ciYaml.enableBranch() will prepend the given branch to the yaml list of enabled_branches', () async {
const String commit1 = 'abc123';
final File ciYaml = fileSystem.file('/flutter_conductor_checkouts/framework/.ci.yaml');
processManager.addCommands(<FakeCommand>[
FakeCommand(
command: <String>[
'git',
'clone',
'--origin',
'upstream',
'--',
FrameworkRepository.defaultUpstream,
fileSystem.path
.join(rootDir, 'flutter_conductor_checkouts', 'framework'),
],
onRun: () {
ciYaml.createSync(recursive: true);
ciYaml.writeAsStringSync('''
# Friendly note

enabled_branches:
- ${FrameworkRepository.defaultBranch}
- dev
- beta
- stable
''');
}),
const FakeCommand(command: <String>[
'git',
'checkout',
FrameworkRepository.defaultBranch,
]),
const FakeCommand(command: <String>[
'git',
'rev-parse',
'HEAD',
], stdout: commit1),
]);
final Checkouts checkouts = Checkouts(
fileSystem: fileSystem,
parentDirectory: fileSystem.directory(rootDir),
platform: platform,
processManager: processManager,
stdio: stdio,
);

final FrameworkRepository framework = FrameworkRepository(checkouts);
expect(
(await framework.ciYaml).enabledBranches,
<String>[FrameworkRepository.defaultBranch, 'dev', 'beta', 'stable'],
);

(await framework.ciYaml).enableBranch('foo');
expect(
(await framework.ciYaml).enabledBranches,
<String>['foo', FrameworkRepository.defaultBranch, 'dev', 'beta', 'stable'],
);

expect(
(await framework.ciYaml).stringContents,
'''
# Friendly note

enabled_branches:
- foo
- ${FrameworkRepository.defaultBranch}
- dev
- beta
- stable
'''
);
});

test('ciYaml.enableBranch() will throw if the input branch is already present in the yaml file', () {
const String commit1 = 'abc123';
final File ciYaml = fileSystem.file('/flutter_conductor_checkouts/framework/.ci.yaml');
processManager.addCommands(<FakeCommand>[
FakeCommand(
command: <String>[
'git',
'clone',
'--origin',
'upstream',
'--',
FrameworkRepository.defaultUpstream,
fileSystem.path
.join(rootDir, 'flutter_conductor_checkouts', 'framework'),
],
onRun: () {
ciYaml.createSync(recursive: true);
ciYaml.writeAsStringSync('''
enabled_branches:
- ${FrameworkRepository.defaultBranch}
- dev
- beta
- stable
''');
}),
const FakeCommand(command: <String>[
'git',
'checkout',
FrameworkRepository.defaultBranch,
]),
const FakeCommand(command: <String>[
'git',
'rev-parse',
'HEAD',
], stdout: commit1),
]);
final Checkouts checkouts = Checkouts(
fileSystem: fileSystem,
parentDirectory: fileSystem.directory(rootDir),
platform: platform,
processManager: processManager,
stdio: stdio,
);

final FrameworkRepository framework = FrameworkRepository(checkouts);
expect(
() async => (await framework.ciYaml).enableBranch(FrameworkRepository.defaultBranch),
throwsExceptionWith('.ci.yaml already contains the branch ${FrameworkRepository.defaultBranch}'),
);
});

test('framework repo set as localUpstream ensures requiredLocalBranches exist locally', () async {
const String commit = 'deadbeef';
const String candidateBranch = 'flutter-1.2-candidate.3';
Expand Down
Loading