From 98c7dfe0b670660ac4fe4051e24e0a7815018f02 Mon Sep 17 00:00:00 2001 From: Casey Hillers Date: Mon, 15 Nov 2021 09:37:05 -0800 Subject: [PATCH 1/5] regex enabled branches --- app_dart/lib/src/model/ci_yaml/ci_yaml.dart | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app_dart/lib/src/model/ci_yaml/ci_yaml.dart b/app_dart/lib/src/model/ci_yaml/ci_yaml.dart index b3805d07e5..8a135c29f2 100644 --- a/app_dart/lib/src/model/ci_yaml/ci_yaml.dart +++ b/app_dart/lib/src/model/ci_yaml/ci_yaml.dart @@ -62,8 +62,8 @@ class CiYaml { /// Filter [targets] to only those that are expected to run for [branch]. /// /// A [Target] is expected to run if: - /// 1. [Target.enabledBranches] exists and contains [branch]. - /// 2. Otherwise, [config.enabledBranches] contains [branch]. + /// 1. [Target.enabledBranches] exists and matches [branch]. + /// 2. Otherwise, [config.enabledBranches] matches [branch]. List _filterEnabledTargets(Iterable targets) { final List filteredTargets = []; @@ -71,11 +71,11 @@ class CiYaml { final Iterable overrideBranchTargets = targets.where((Target target) => target.value.enabledBranches.isNotEmpty); final Iterable enabledTargets = - overrideBranchTargets.where((Target target) => target.value.enabledBranches.contains(branch)); + overrideBranchTargets.where((Target target) => RegExp(target.value.enabledBranches.join('|')).hasMatch(branch)); filteredTargets.addAll(enabledTargets); // 2. Add targets with global definition (this is the majority of targets) - if (config.enabledBranches.contains(branch)) { + if (RegExp(config.enabledBranches.join('|')).hasMatch(branch)) { final Iterable defaultBranchTargets = targets.where((Target target) => target.value.enabledBranches.isEmpty); filteredTargets.addAll(defaultBranchTargets); From d4ea22f5179bebf81156f8cf8ee1d05bb0fde300 Mon Sep 17 00:00:00 2001 From: Casey Hillers Date: Tue, 16 Nov 2021 13:26:35 -0800 Subject: [PATCH 2/5] [ci.yaml] Support branch regexes --- CI_YAML.md | 2 ++ app_dart/lib/src/model/ci_yaml/ci_yaml.dart | 12 +++++++----- app_dart/test/service/scheduler_test.dart | 19 +++++++++++++++++++ 3 files changed, 28 insertions(+), 5 deletions(-) diff --git a/CI_YAML.md b/CI_YAML.md index a9467bec70..92a413155d 100644 --- a/CI_YAML.md +++ b/CI_YAML.md @@ -17,6 +17,8 @@ Example config: # /.ci.yaml enabled_branches: - master + - flutter-\d+.\d+-candidate.\d+ + targets: # A Target is an individual unit of work that is scheduled by Flutter infra # Target's are composed of the following properties: diff --git a/app_dart/lib/src/model/ci_yaml/ci_yaml.dart b/app_dart/lib/src/model/ci_yaml/ci_yaml.dart index 8a135c29f2..4e79334f1d 100644 --- a/app_dart/lib/src/model/ci_yaml/ci_yaml.dart +++ b/app_dart/lib/src/model/ci_yaml/ci_yaml.dart @@ -28,13 +28,14 @@ class CiYaml { /// Gets all [Target] that run on presubmit for this config. List get presubmitTargets { - if (!config.enabledBranches.contains(branch)) { - throw Exception('$branch is not enabled for this .ci.yaml.\nAdd it to run tests against this PR.'); - } final Iterable presubmitTargets = _targets.where((Target target) => target.value.presubmit && !target.value.bringup); - return _filterEnabledTargets(presubmitTargets); + final List enabledTargets = _filterEnabledTargets(presubmitTargets); + if (enabledTargets.isEmpty) { + throw Exception('$branch is not enabled for this .ci.yaml.\nAdd it to run tests against this PR.'); + } + return enabledTargets; } /// Gets all [Target] that run on postsubmit for this config. @@ -75,7 +76,8 @@ class CiYaml { filteredTargets.addAll(enabledTargets); // 2. Add targets with global definition (this is the majority of targets) - if (RegExp(config.enabledBranches.join('|')).hasMatch(branch)) { + final RegExp globallyEnabledBranches = RegExp(config.enabledBranches.join('|')); + if (globallyEnabledBranches.hasMatch(branch)) { final Iterable defaultBranchTargets = targets.where((Target target) => target.value.enabledBranches.isEmpty); filteredTargets.addAll(defaultBranchTargets); diff --git a/app_dart/test/service/scheduler_test.dart b/app_dart/test/service/scheduler_test.dart index c04a65a33f..126c60db16 100644 --- a/app_dart/test/service/scheduler_test.dart +++ b/app_dart/test/service/scheduler_test.dart @@ -337,6 +337,25 @@ targets: throwsA(predicate((Exception e) => e.toString().contains('$branch is not enabled')))); }); + test('checks for release branch regex', () async { + const String branch = 'flutter-1.24-candidate.1'; + httpClient = MockClient((http.Request request) async { + if (request.url.path.contains('.ci.yaml')) { + return http.Response(''' +enabled_branches: + - main + - flutter-\\d+.\\d+-candidate.\\d+ +targets: + - name: Linux A + scheduler: luci + ''', 200); + } + throw Exception('Failed to find ${request.url.path}'); + }); + final List targets = await scheduler.getPresubmitTargets(generatePullRequest(branch: branch)); + expect(targets.single.value.name, 'Linux A'); + }); + test('triggers expected presubmit build checks', () async { await scheduler.triggerPresubmitTargets(pullRequest: pullRequest); expect( From 4aa9f18e69d3956293475022170dea7705d3c22c Mon Sep 17 00:00:00 2001 From: Casey Hillers Date: Tue, 16 Nov 2021 13:30:01 -0800 Subject: [PATCH 3/5] ci yaml doc fix --- CI_YAML.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CI_YAML.md b/CI_YAML.md index 92a413155d..7f6da7be5c 100644 --- a/CI_YAML.md +++ b/CI_YAML.md @@ -17,7 +17,7 @@ Example config: # /.ci.yaml enabled_branches: - master - - flutter-\d+.\d+-candidate.\d+ + - flutter-\\d+.\\d+-candidate.\\d+ targets: # A Target is an individual unit of work that is scheduled by Flutter infra From 827768219e23fc7782ce0fb75b54df66866a76f0 Mon Sep 17 00:00:00 2001 From: Casey Hillers Date: Fri, 19 Nov 2021 13:41:35 -0800 Subject: [PATCH 4/5] Add more tests --- CI_YAML.md | 7 +- .../validate_all_ci_configs_test.dart | 75 ++++++++++++++++++- app_dart/lib/src/model/ci_yaml/ci_yaml.dart | 20 ++++- app_dart/pubspec.lock | 14 ++++ app_dart/pubspec.yaml | 1 + app_dart/test/model/ci_yaml/ci_yaml_test.dart | 35 +++++++++ 6 files changed, 145 insertions(+), 7 deletions(-) create mode 100644 app_dart/test/model/ci_yaml/ci_yaml_test.dart diff --git a/CI_YAML.md b/CI_YAML.md index 7f6da7be5c..e21dc8e9e8 100644 --- a/CI_YAML.md +++ b/CI_YAML.md @@ -15,9 +15,12 @@ are welcome. Example config: ```yaml # /.ci.yaml + +# Enabled branches is a list of regexes, with the assumption that these are full line matches. +# Internally, Cocoon prefixes these with $ and suffixes with ^ to enable matches. enabled_branches: - - master - - flutter-\\d+.\\d+-candidate.\\d+ + - main + - flutter-\\d+\\.\\d+-candidate\\.\\d+ targets: # A Target is an individual unit of work that is scheduled by Flutter infra diff --git a/app_dart/integration_test/validate_all_ci_configs_test.dart b/app_dart/integration_test/validate_all_ci_configs_test.dart index 430690e0a2..a3158b4f4e 100644 --- a/app_dart/integration_test/validate_all_ci_configs_test.dart +++ b/app_dart/integration_test/validate_all_ci_configs_test.dart @@ -2,9 +2,15 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +import 'dart:core'; +import 'dart:io'; + import 'package:cocoon_service/cocoon_service.dart'; +import 'package:cocoon_service/src/model/ci_yaml/ci_yaml.dart'; +import 'package:cocoon_service/src/model/proto/internal/scheduler.pbserver.dart' as pb; import 'package:github/github.dart'; import 'package:http/http.dart' as http; +import 'package:process/process.dart'; import 'package:test/test.dart'; import 'package:yaml/yaml.dart'; @@ -13,7 +19,7 @@ import 'common.dart'; /// List of repositories that have supported .ci.yaml config files. final List configs = [ SupportedConfig(RepositorySlug('flutter', 'cocoon'), 'main'), - SupportedConfig(RepositorySlug('flutter', 'engine')), + SupportedConfig(RepositorySlug('flutter', 'engine'), 'main'), SupportedConfig(RepositorySlug('flutter', 'flutter')), SupportedConfig(RepositorySlug('flutter', 'packages')), SupportedConfig(RepositorySlug('flutter', 'plugins')), @@ -35,5 +41,72 @@ Future main() async { fail(e.message); } }); + + test('validate enabled branches of $config', () async { + final String configContent = await githubFileContent( + config.slug, + kCiYamlPath, + httpClientProvider: () => http.Client(), + ref: config.branch, + ); + final YamlMap configYaml = loadYaml(configContent) as YamlMap; + final pb.SchedulerConfig schedulerConfig = schedulerConfigFromYaml(configYaml); + final List githubBranches = getBranchesForRepository(config.slug); + + final Map validEnabledBranches = {}; + // Add config wide enabled branches + for (String enabledBranch in schedulerConfig.enabledBranches) { + validEnabledBranches[enabledBranch] = false; + } + // Add all target specific enabled branches + for (pb.Target target in schedulerConfig.targets) { + for (String enabledBranch in target.enabledBranches) { + validEnabledBranches[enabledBranch] = false; + } + } + + // N^2 scan to verify all enabled branch patterns match an exist branch on the repo. + for (String enabledBranch in validEnabledBranches.keys) { + for (String githubBranch in githubBranches) { + if (CiYaml.enabledBranchesMatchesCurrentBranch([enabledBranch], githubBranch)) { + validEnabledBranches[enabledBranch] = true; + } + } + } + + if (config.slug.name == 'engine') { + print(githubBranches); + print(validEnabledBranches); + } + + // Verify the enabled branches + for (String enabledBranch in validEnabledBranches.keys) { + expect(validEnabledBranches[enabledBranch], isTrue, + reason: '$enabledBranch does not match to a branch in ${config.slug.fullName}'); + } + }, skip: config.slug.name == 'flutter'); } } + +/// Gets all branches for [slug]. +/// +/// Internally, uses the git on path to get the branches from the remote for [slug]. +List getBranchesForRepository(RepositorySlug slug) { + const ProcessManager processManager = LocalProcessManager(); + final ProcessResult result = + processManager.runSync(['git', 'ls-remote', '--head', 'https://github.com/${slug.fullName}']); + final List lines = (result.stdout as String).split('\n'); + + final List githubBranches = []; + for (String line in lines) { + if (line.isEmpty) { + continue; + } + // Lines follow the format of `$sha\t$ref` + final String ref = line.split('\t')[1]; + final String branch = ref.replaceAll('refs/heads/', ''); + githubBranches.add(branch); + } + + return githubBranches; +} diff --git a/app_dart/lib/src/model/ci_yaml/ci_yaml.dart b/app_dart/lib/src/model/ci_yaml/ci_yaml.dart index 4e79334f1d..26327e8c2e 100644 --- a/app_dart/lib/src/model/ci_yaml/ci_yaml.dart +++ b/app_dart/lib/src/model/ci_yaml/ci_yaml.dart @@ -71,13 +71,12 @@ class CiYaml { // 1. Add targets with local definition final Iterable overrideBranchTargets = targets.where((Target target) => target.value.enabledBranches.isNotEmpty); - final Iterable enabledTargets = - overrideBranchTargets.where((Target target) => RegExp(target.value.enabledBranches.join('|')).hasMatch(branch)); + final Iterable enabledTargets = overrideBranchTargets + .where((Target target) => enabledBranchesMatchesCurrentBranch(target.value.enabledBranches, branch)); filteredTargets.addAll(enabledTargets); // 2. Add targets with global definition (this is the majority of targets) - final RegExp globallyEnabledBranches = RegExp(config.enabledBranches.join('|')); - if (globallyEnabledBranches.hasMatch(branch)) { + if (enabledBranchesMatchesCurrentBranch(config.enabledBranches, branch)) { final Iterable defaultBranchTargets = targets.where((Target target) => target.value.enabledBranches.isEmpty); filteredTargets.addAll(defaultBranchTargets); @@ -85,4 +84,17 @@ class CiYaml { return filteredTargets; } + + /// Whether any of the possible [RegExp] in [enabledBranches] match [branch]. + static bool enabledBranchesMatchesCurrentBranch(List enabledBranches, String branch) { + final List regexes = []; + for (String enabledBranch in enabledBranches) { + // Prefix with start of line and suffix with end of line + regexes.add('^$enabledBranch\$'); + } + final String rawRegexp = regexes.join('|'); + final RegExp regexp = RegExp(rawRegexp); + + return regexp.hasMatch(branch); + } } diff --git a/app_dart/pubspec.lock b/app_dart/pubspec.lock index e42efba231..a1f39510cf 100644 --- a/app_dart/pubspec.lock +++ b/app_dart/pubspec.lock @@ -477,6 +477,13 @@ packages: url: "https://pub.dartlang.org" source: hosted version: "1.11.1" + platform: + dependency: transitive + description: + name: platform + url: "https://pub.dartlang.org" + source: hosted + version: "3.0.2" pointycastle: dependency: transitive description: @@ -491,6 +498,13 @@ packages: url: "https://pub.dartlang.org" source: hosted version: "1.5.0" + process: + dependency: "direct dev" + description: + name: process + url: "https://pub.dartlang.org" + source: hosted + version: "4.2.4" protobuf: dependency: "direct main" description: diff --git a/app_dart/pubspec.yaml b/app_dart/pubspec.yaml index 5456eae530..5c0b3a1983 100644 --- a/app_dart/pubspec.yaml +++ b/app_dart/pubspec.yaml @@ -36,6 +36,7 @@ dev_dependencies: fake_async: ^1.2.0 json_serializable: ^6.0.0 mockito: ^5.0.14 + process: ^4.2.4 test: ^1.17.11 builders: diff --git a/app_dart/test/model/ci_yaml/ci_yaml_test.dart b/app_dart/test/model/ci_yaml/ci_yaml_test.dart new file mode 100644 index 0000000000..e3875a47fa --- /dev/null +++ b/app_dart/test/model/ci_yaml/ci_yaml_test.dart @@ -0,0 +1,35 @@ +// Copyright 2019 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:cocoon_service/src/model/ci_yaml/ci_yaml.dart'; +import 'package:test/test.dart'; + +void main() { + group('enabledBranchesMatchesCurrentBranch', () { + final List tests = [ + EnabledBranchesRegexTest('matches main', 'main', ['main']), + EnabledBranchesRegexTest( + 'matches candidate branch', 'flutter-2.4-candidate.3', ['flutter-\\d+\\.\\d+-candidate\\.\\d+']), + EnabledBranchesRegexTest('matches main when not first pattern', 'main', ['dev', 'main']), + EnabledBranchesRegexTest('does not do partial matches', 'super-main', ['main'], false), + ]; + + for (EnabledBranchesRegexTest regexTest in tests) { + test(regexTest.name, () { + expect(CiYaml.enabledBranchesMatchesCurrentBranch(regexTest.enabledBranches, regexTest.branch), + regexTest.expectation); + }); + } + }); +} + +/// Wrapper class for table driven design of [CiYaml.enabledBranchesMatchesCurrentBranch]. +class EnabledBranchesRegexTest { + EnabledBranchesRegexTest(this.name, this.branch, this.enabledBranches, [this.expectation = true]); + + final String branch; + final List enabledBranches; + final String name; + final bool expectation; +} From e38347db7c2dda16f823167f3a78d32ea4d6667e Mon Sep 17 00:00:00 2001 From: Casey Hillers Date: Mon, 22 Nov 2021 14:11:55 -0800 Subject: [PATCH 5/5] Revert "Retry ci.yaml checks" This reverts commit 4954801756992a86310272b020397ced634dbfb2. --- app_dart/lib/src/service/scheduler.dart | 54 +++++++++-------------- app_dart/test/service/scheduler_test.dart | 33 +++----------- 2 files changed, 27 insertions(+), 60 deletions(-) diff --git a/app_dart/lib/src/service/scheduler.dart b/app_dart/lib/src/service/scheduler.dart index 838551285a..898bd7ecc6 100644 --- a/app_dart/lib/src/service/scheduler.dart +++ b/app_dart/lib/src/service/scheduler.dart @@ -244,8 +244,13 @@ class Scheduler { Future triggerPresubmitTargets({ required github.PullRequest pullRequest, String reason = 'Newer commit available', - RetryOptions retryOptions = const RetryOptions(maxAttempts: 3), }) async { + // Always cancel running builds so we don't ever schedule duplicates. + log.fine('about to cancel presubmit targets'); + await cancelPreSubmitTargets( + pullRequest: pullRequest, + reason: reason, + ); final github.CheckRun ciValidationCheckRun = await githubChecksService.githubChecksUtil.createCheckRun( config, pullRequest.base!.repo!.slug(), @@ -257,10 +262,20 @@ class Scheduler { ), ); final github.RepositorySlug slug = pullRequest.base!.repo!.slug(); - final dynamic exception = await retryOptions.retry(() async => _triggerPresubmitTargets( - pullRequest: pullRequest, - reason: reason, - )); + dynamic exception; + try { + final List presubmitTargets = await getPresubmitTargets(pullRequest); + await luciBuildService.scheduleTryBuilds( + targets: presubmitTargets, + pullRequest: pullRequest, + ); + } on FormatException catch (error, backtrace) { + log.warning(backtrace.toString()); + exception = error; + } catch (error, backtrace) { + log.warning(backtrace.toString()); + exception = error; + } // Update validate ci.yaml check if (exception == null) { @@ -293,35 +308,6 @@ class Scheduler { 'Finished triggering builds for: pr ${pullRequest.number}, commit ${pullRequest.base!.sha}, branch ${pullRequest.head!.ref} and slug ${pullRequest.base!.repo!.slug()}}'); } - /// Internal wrapper for [triggerPresubmitTargets] retry logic. - Future _triggerPresubmitTargets({ - required github.PullRequest pullRequest, - String reason = 'Newer commit available', - }) async { - dynamic exception; - // Always cancel running builds so we don't ever schedule duplicates. - await cancelPreSubmitTargets( - pullRequest: pullRequest, - reason: reason, - ); - log.fine('Cancelled existing presubmit runs'); - try { - final List presubmitTargets = await getPresubmitTargets(pullRequest); - await luciBuildService.scheduleTryBuilds( - targets: presubmitTargets, - pullRequest: pullRequest, - ); - } on FormatException catch (error, backtrace) { - log.warning(backtrace.toString()); - exception = error; - } catch (error, backtrace) { - log.warning(backtrace.toString()); - exception = error; - } - - return exception; - } - /// Given a pull request event, retry all failed LUCI checks. /// /// 1. Aggregate .ci.yaml and try_builders.json presubmit builds. diff --git a/app_dart/test/service/scheduler_test.dart b/app_dart/test/service/scheduler_test.dart index 126c60db16..5e6c21c222 100644 --- a/app_dart/test/service/scheduler_test.dart +++ b/app_dart/test/service/scheduler_test.dart @@ -3,7 +3,6 @@ // found in the LICENSE file. import 'dart:convert'; -import 'dart:io'; import 'package:cocoon_service/src/model/appengine/commit.dart'; import 'package:cocoon_service/src/model/appengine/task.dart'; @@ -44,6 +43,7 @@ targets: - stable scheduler: luci - name: Google Internal Roll + postsubmit: true presubmit: false scheduler: google_internal '''; @@ -291,11 +291,13 @@ enabled_branches: - master targets: - name: Linux A + presubmit: true scheduler: luci - name: Linux B scheduler: luci enabled_branches: - stable + presubmit: true - name: Linux C scheduler: luci enabled_branches: @@ -304,10 +306,12 @@ targets: - name: Linux D scheduler: luci bringup: true + presubmit: true - name: Google-internal roll scheduler: google_internal enabled_branches: - master + presubmit: true ''', 200); } throw Exception('Failed to find ${request.url.path}'); @@ -327,6 +331,7 @@ enabled_branches: - master targets: - name: Linux A + presubmit: true scheduler: luci ''', 200); } @@ -385,34 +390,10 @@ targets: [CheckRunStatus.completed, CheckRunConclusion.success]); }); - test('ci.yaml validation passes with retry', () async { - bool retried = false; - httpClient = MockClient((http.Request request) async { - if (request.url.path.contains('.ci.yaml')) { - if (retried) { - return http.Response(singleCiYaml, HttpStatus.ok); - } - retried = true; - return http.Response('FAILURE', HttpStatus.internalServerError); - } - throw Exception('Failed to find ${request.url.path}'); - }); - when(mockGithubChecksUtil.getCheckRun(any, any, any)) - .thenAnswer((Invocation invocation) async => createCheckRun(id: 0)); - await scheduler.triggerPresubmitTargets(pullRequest: pullRequest); - expect( - verify(mockGithubChecksUtil.updateCheckRun(any, any, any, - status: captureAnyNamed('status'), - conclusion: captureAnyNamed('conclusion'), - output: anyNamed('output'))) - .captured, - [CheckRunStatus.completed, CheckRunConclusion.success]); - }); - test('ci.yaml validation fails with empty config', () async { httpClient = MockClient((http.Request request) async { if (request.url.path.contains('.ci.yaml')) { - return http.Response('', HttpStatus.ok); + return http.Response('', 200); } throw Exception('Failed to find ${request.url.path}'); });