Revert will wait for required checks to complete. (#2199)
diff --git a/auto_submit/lib/exception/retryable_checkrun_exception.dart b/auto_submit/lib/exception/retryable_checkrun_exception.dart
new file mode 100644
index 0000000..43a9948
--- /dev/null
+++ b/auto_submit/lib/exception/retryable_checkrun_exception.dart
@@ -0,0 +1,12 @@
+// Copyright 2022 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.
+
+class RetryableCheckRunException implements Exception {
+ RetryableCheckRunException(this.cause);
+
+ final String cause;
+
+ @override
+ String toString() => cause;
+}
diff --git a/auto_submit/lib/service/config.dart b/auto_submit/lib/service/config.dart
index 40ae784..4806d3e 100644
--- a/auto_submit/lib/service/config.dart
+++ b/auto_submit/lib/service/config.dart
@@ -12,6 +12,7 @@
import 'package:http/http.dart' as http;
import 'package:neat_cache/cache_provider.dart';
import 'package:neat_cache/neat_cache.dart';
+import 'package:retry/retry.dart';
import '../foundation/providers.dart';
import '../service/secrets.dart';
@@ -36,9 +37,17 @@
static const String kAutosubmitLabel = 'autosubmit';
static const String kRevertLabel = 'revert';
- static const int backOfMultiplier = 200;
- static const int backoffAttempts = 3;
- static const int maxDelaySeconds = 1;
+ static const RetryOptions mergeRetryOptions = RetryOptions(
+ delayFactor: Duration(milliseconds: 200),
+ maxDelay: Duration(seconds: 1),
+ maxAttempts: 5,
+ );
+
+ static const RetryOptions requiredChecksRetryOptions = RetryOptions(
+ delayFactor: Duration(milliseconds: 500),
+ maxDelay: Duration(seconds: 5),
+ maxAttempts: 5,
+ );
static const String flutter = 'flutter';
static const String flutterGcpProjectId = 'flutter-dashboard';
diff --git a/auto_submit/lib/service/github_service.dart b/auto_submit/lib/service/github_service.dart
index 6cec27b..2affddc 100644
--- a/auto_submit/lib/service/github_service.dart
+++ b/auto_submit/lib/service/github_service.dart
@@ -24,6 +24,24 @@
return await github.checks.checkRuns.listCheckRunsForRef(slug, ref: ref).toList();
}
+ Future<List<CheckRun>> getCheckRunsFiltered({
+ required RepositorySlug slug,
+ required String ref,
+ String? checkName,
+ CheckRunStatus? status,
+ CheckRunFilter? filter,
+ }) async {
+ return await github.checks.checkRuns
+ .listCheckRunsForRef(
+ slug,
+ ref: ref,
+ checkName: checkName,
+ status: status,
+ filter: filter,
+ )
+ .toList();
+ }
+
/// Fetches the specified commit.
Future<RepositoryCommit> getCommit(RepositorySlug slug, String sha) async {
return await github.repositories.getCommit(slug, sha);
diff --git a/auto_submit/lib/service/validation_service.dart b/auto_submit/lib/service/validation_service.dart
index d60aba7..6c412c0 100644
--- a/auto_submit/lib/service/validation_service.dart
+++ b/auto_submit/lib/service/validation_service.dart
@@ -38,12 +38,7 @@
/// tested. The expectation is that the list of validation will grow overtime.
class ValidationService {
ValidationService(this.config, {RetryOptions? retryOptions})
- : retryOptions = retryOptions ??
- const RetryOptions(
- delayFactor: Duration(milliseconds: Config.backOfMultiplier),
- maxDelay: Duration(seconds: Config.maxDelaySeconds),
- maxAttempts: Config.backoffAttempts,
- ) {
+ : retryOptions = retryOptions ?? Config.mergeRetryOptions {
/// Validates a PR marked with the reverts label.
revertValidation = Revert(config: config);
approverService = ApproverService(config);
@@ -303,6 +298,10 @@
log.info(message);
}
+ } else if (!revertValidationResult.result && revertValidationResult.action == Action.IGNORE_TEMPORARILY) {
+ // if required check runs have not completed process again.
+ log.info('Some of the required checks have not completed. Requeueing.');
+ return;
} else {
// since we do not temporarily ignore anything with a revert request we
// know we will report the error and remove the label.
@@ -338,10 +337,9 @@
// The createGitHubGraphQLClient can throw Exception on github permissions
// errors.
final graphql.GraphQLClient client = await config.createGitHubGraphQLClient(slug);
-
graphql.QueryResult? result;
- await _runProcessMergeWithRetries(
+ await retryOptions.retry(
() async {
result = await _processMergeInternal(
client: client,
@@ -350,7 +348,7 @@
messagePullRequest: messagePullRequest,
);
},
- retryOptions,
+ retryIf: (Exception e) => e is RetryableMergeException,
);
if (result != null && result!.hasException) {
@@ -478,14 +476,6 @@
/// Function signature that will be executed with retries.
typedef RetryHandler = Function();
-/// Runs the internal processMerge with retries.
-Future<void> _runProcessMergeWithRetries(RetryHandler retryHandler, RetryOptions retryOptions) {
- return retryOptions.retry(
- retryHandler,
- retryIf: (Exception e) => e is RetryableMergeException,
- );
-}
-
/// Internal wrapper for the logic of merging a pull request into github.
Future<graphql.QueryResult> _processMergeInternal({
required graphql.GraphQLClient client,
diff --git a/auto_submit/lib/validations/required_check_runs.dart b/auto_submit/lib/validations/required_check_runs.dart
new file mode 100644
index 0000000..445b050
--- /dev/null
+++ b/auto_submit/lib/validations/required_check_runs.dart
@@ -0,0 +1,19 @@
+// Copyright 2022 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.
+
+// Note that we need this file because Github does not expose a field within the
+// checks that states whether or not a particular check is required or not.
+
+const String ciyamlValidation = 'ci.yaml validation';
+
+/// flutter, engine, cocoon, plugins, packages, buildroot and tests
+const Map<String, List<String>> requiredCheckRunsMapping = {
+ 'flutter': [ciyamlValidation],
+ 'engine': [ciyamlValidation],
+ 'cocoon': [ciyamlValidation],
+ 'plugins': [ciyamlValidation],
+ 'packages': [ciyamlValidation],
+ 'buildroot': [ciyamlValidation],
+ 'tests': [ciyamlValidation],
+};
diff --git a/auto_submit/lib/validations/revert.dart b/auto_submit/lib/validations/revert.dart
index c7e3932..0c2fee2 100644
--- a/auto_submit/lib/validations/revert.dart
+++ b/auto_submit/lib/validations/revert.dart
@@ -2,25 +2,33 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
+import 'package:auto_submit/exception/retryable_checkrun_exception.dart';
import 'package:auto_submit/model/auto_submit_query_result.dart' as auto;
+import 'package:auto_submit/service/config.dart';
import 'package:auto_submit/service/github_service.dart';
+import 'package:auto_submit/validations/required_check_runs.dart';
import 'package:auto_submit/validations/validation.dart';
import 'package:github/github.dart' as github;
+import 'package:retry/retry.dart';
import '../service/log.dart';
class Revert extends Validation {
Revert({
required super.config,
- });
+ RetryOptions? retryOptions,
+ }) : retryOptions = retryOptions ?? Config.requiredChecksRetryOptions;
static const Set<String> allowedReviewers = <String>{ORG_MEMBER, ORG_OWNER};
+ final RetryOptions retryOptions;
@override
Future<ValidationResult> validate(auto.QueryResult result, github.PullRequest messagePullRequest) async {
final auto.PullRequest pullRequest = result.repository!.pullRequest!;
final String authorAssociation = pullRequest.authorAssociation!;
final String? author = pullRequest.author!.login;
+ final auto.Commit commit = pullRequest.commits!.nodes!.single.commit!;
+ String? sha = commit.oid;
if (!isValidAuthor(author, authorAssociation)) {
String message = 'The author $author does not have permissions to make this request.';
@@ -47,6 +55,22 @@
github.RepositorySlug repositorySlug = _getSlugFromLink(revertLink);
GithubService githubService = await config.createGithubService(repositorySlug);
+
+ bool requiredChecksCompleted = await waitForRequiredChecks(
+ githubService: githubService,
+ slug: repositorySlug,
+ sha: sha!,
+ checkNames: requiredCheckRunsMapping[repositorySlug.name]!,
+ );
+
+ if (!requiredChecksCompleted) {
+ return ValidationResult(
+ false,
+ Action.IGNORE_TEMPORARILY,
+ 'Some of the required checks did not complete in time.',
+ );
+ }
+
int pullRequestId = _getPullRequestNumberFromLink(revertLink);
github.PullRequest requestToRevert = await githubService.getPullRequest(repositorySlug, pullRequestId);
@@ -100,4 +124,66 @@
List<String> linkSplit = link.split('#');
return int.parse(linkSplit.elementAt(1));
}
+
+ /// Wait for the required checks to complete, and if repository has no checks
+ /// true is returned.
+ Future<bool> waitForRequiredChecks({
+ required GithubService githubService,
+ required github.RepositorySlug slug,
+ required String sha,
+ required List<String> checkNames,
+ }) async {
+ List<github.CheckRun> targetCheckRuns = [];
+ for (var element in checkNames) {
+ targetCheckRuns.addAll(
+ await githubService.getCheckRunsFiltered(
+ slug: slug,
+ ref: sha,
+ checkName: element,
+ ),
+ );
+ }
+
+ bool checksCompleted = true;
+
+ try {
+ for (github.CheckRun checkRun in targetCheckRuns) {
+ await retryOptions.retry(
+ () async {
+ await _verifyCheckRunCompleted(
+ slug,
+ githubService,
+ checkRun,
+ );
+ },
+ retryIf: (Exception e) => e is RetryableCheckRunException,
+ );
+ }
+ } catch (e) {
+ log.warning('Required check has not completed in time. ${e.toString()}');
+ checksCompleted = false;
+ }
+
+ return checksCompleted;
+ }
+}
+
+/// Function signature that will be executed with retries.
+typedef RetryHandler = Function();
+
+/// Simple function to wait on completed checkRuns with retries.
+Future<void> _verifyCheckRunCompleted(
+ github.RepositorySlug slug,
+ GithubService githubService,
+ github.CheckRun targetCheckRun,
+) async {
+ List<github.CheckRun> checkRuns = await githubService.getCheckRunsFiltered(
+ slug: slug,
+ ref: targetCheckRun.headSha!,
+ checkName: targetCheckRun.name,
+ );
+
+ if (checkRuns.first.name != targetCheckRun.name || checkRuns.first.conclusion != github.CheckRunConclusion.success) {
+ throw RetryableCheckRunException('${targetCheckRun.name} has not yet completed.');
+ }
}
diff --git a/auto_submit/test/service/validation_service_test.dart b/auto_submit/test/service/validation_service_test.dart
index 3e9acfb..50c7b30 100644
--- a/auto_submit/test/service/validation_service_test.dart
+++ b/auto_submit/test/service/validation_service_test.dart
@@ -42,7 +42,10 @@
githubGraphQLClient = FakeGraphQLClient();
githubService = FakeGithubService(client: MockGitHub());
config = FakeConfig(githubService: githubService, githubGraphQLClient: githubGraphQLClient);
- validationService = ValidationService(config);
+ validationService = ValidationService(
+ config,
+ retryOptions: const RetryOptions(delayFactor: Duration.zero, maxDelay: Duration.zero, maxAttempts: 1),
+ );
slug = RepositorySlug('flutter', 'cocoon');
jobsResource = MockJobsResource();
@@ -291,12 +294,58 @@
assert(pubsub.messagesQueue.isEmpty);
});
+ test('Revert returns on in process required checkRuns.', () async {
+ githubGraphQLClient.mutateResultForOptions = (MutationOptions options) => createFakeQueryResult();
+ final PullRequestHelper flutterRequest = PullRequestHelper(
+ prNumber: 0,
+ lastCommitHash: oid,
+ reviews: <PullRequestReviewHelper>[],
+ );
+
+ githubService.checkRunsData = inProgressCheckRunsMock;
+ githubService.createCommentData = createCommentMock;
+ githubService.throwOnCreateIssue = true;
+ githubService.useRealComment = true;
+ final FakePubSub pubsub = FakePubSub();
+ final PullRequest pullRequest = generatePullRequest(
+ prNumber: 0,
+ repoName: slug.name,
+ authorAssociation: 'OWNER',
+ labelName: 'revert',
+ body: 'Reverts flutter/flutter#1234',
+ );
+
+ final FakeRevert fakeRevert = FakeRevert(config: config);
+ fakeRevert.validationResult =
+ ValidationResult(false, Action.IGNORE_TEMPORARILY, 'Some of the required checks did not complete in time.');
+ validationService.revertValidation = fakeRevert;
+ final FakeApproverService fakeApproverService = FakeApproverService(config);
+ validationService.approverService = fakeApproverService;
+
+ unawaited(pubsub.publish('auto-submit-queue-sub', pullRequest));
+ final auto.QueryResult queryResult = createQueryResult(flutterRequest);
+
+ await validationService.processRevertRequest(
+ config: config,
+ result: queryResult,
+ messagePullRequest: pullRequest,
+ ackId: 'test',
+ pubsub: pubsub,
+ );
+
+ // if the merge is successful we do not remove the label and we do not add a comment to the issue.
+ expect(githubService.issueComment, isNull);
+ expect(githubService.labelRemoved, false);
+ // We acknowledge the issue.
+ assert(pubsub.messagesQueue.isNotEmpty);
+ });
+
test('Exhaust retries on merge on retryable error.', () async {
validationService = ValidationService(
config,
retryOptions: const RetryOptions(
- delayFactor: Duration(milliseconds: 1),
- maxDelay: Duration(milliseconds: 1),
+ delayFactor: Duration.zero,
+ maxDelay: Duration.zero,
maxAttempts: 1,
),
);
@@ -481,8 +530,7 @@
test('Merge fails the first time but then succeeds after retry.', () async {
validationService = ValidationService(
config,
- retryOptions:
- const RetryOptions(delayFactor: Duration(milliseconds: 50), maxDelay: Duration(seconds: 1), maxAttempts: 3),
+ retryOptions: const RetryOptions(delayFactor: Duration.zero, maxDelay: Duration.zero, maxAttempts: 3),
);
// Create a result that will trigger a retry.
diff --git a/auto_submit/test/src/service/fake_github_service.dart b/auto_submit/test/src/service/fake_github_service.dart
index 0a15c0a..3148b27 100644
--- a/auto_submit/test/src/service/fake_github_service.dart
+++ b/auto_submit/test/src/service/fake_github_service.dart
@@ -107,6 +107,27 @@
}
@override
+ Future<List<CheckRun>> getCheckRunsFiltered({
+ required RepositorySlug slug,
+ required String ref,
+ String? checkName,
+ CheckRunStatus? status,
+ CheckRunFilter? filter,
+ }) async {
+ List<CheckRun> checkRuns = await getCheckRuns(slug, ref);
+ if (checkName != null) {
+ List<CheckRun> checkRunsFilteredByName = [];
+ for (CheckRun checkRun in checkRuns) {
+ if (checkRun.name == checkName) {
+ checkRunsFilteredByName.add(checkRun);
+ }
+ }
+ return checkRunsFilteredByName;
+ }
+ return checkRuns;
+ }
+
+ @override
Future<RepositoryCommit> getCommit(RepositorySlug slug, String sha) async {
final RepositoryCommit commit = RepositoryCommit.fromJson(jsonDecode(commitMock!) as Map<String, dynamic>);
return commit;
diff --git a/auto_submit/test/validations/revert_test.dart b/auto_submit/test/validations/revert_test.dart
index 3d8ff93..c81766e 100644
--- a/auto_submit/test/validations/revert_test.dart
+++ b/auto_submit/test/validations/revert_test.dart
@@ -7,11 +7,12 @@
import 'package:auto_submit/model/auto_submit_query_result.dart';
import 'package:auto_submit/validations/validation.dart';
import 'package:github/github.dart' as github;
+import 'package:retry/retry.dart';
+import 'package:test/test.dart';
import 'revert_test_data.dart';
import 'package:auto_submit/validations/revert.dart';
-import 'package:test/scaffolding.dart';
import '../utilities/mocks.dart';
import '../src/service/fake_config.dart';
@@ -20,16 +21,20 @@
void main() {
late FakeConfig config;
- FakeGithubService githubService = FakeGithubService();
+ late FakeGithubService githubService;
late FakeGraphQLClient githubGraphQLClient;
MockGitHub gitHub = MockGitHub();
late Revert revert;
/// Setup objects needed across test groups.
setUp(() {
+ githubService = FakeGithubService();
githubGraphQLClient = FakeGraphQLClient();
config = FakeConfig(githubService: githubService, githubGraphQLClient: githubGraphQLClient, githubClient: gitHub);
- revert = Revert(config: config);
+ revert = Revert(
+ config: config,
+ retryOptions: const RetryOptions(delayFactor: Duration.zero, maxDelay: Duration.zero, maxAttempts: 1),
+ );
});
group('Author validation tests.', () {
@@ -142,6 +147,40 @@
);
});
+ test('Validation returns on checkRun that has not completed.', () async {
+ Map<String, dynamic> pullRequestJsonMap = jsonDecode(revertPullRequestJson) as Map<String, dynamic>;
+ github.PullRequest revertPullRequest = github.PullRequest.fromJson(pullRequestJsonMap);
+ final Map<String, dynamic> queryResultJsonDecode =
+ jsonDecode(queryResultRepositoryOwnerJson) as Map<String, dynamic>;
+ final QueryResult queryResult = QueryResult.fromJson(queryResultJsonDecode);
+
+ Map<String, dynamic> originalPullRequestJsonMap = jsonDecode(originalPullRequestJson) as Map<String, dynamic>;
+ github.PullRequest originalPullRequest = github.PullRequest.fromJson(originalPullRequestJsonMap);
+ githubService.pullRequestData = originalPullRequest;
+
+ // code gets the original file list then the current file list.
+ githubService.usePullRequestFilesList = true;
+ githubService.pullRequestFilesMockList.add(originalPullRequestFilesJson);
+ githubService.pullRequestFilesMockList.add(revertPullRequestFilesJson);
+
+ // Need to set the mock checkRuns for required CheckRun validation
+ githubService.checkRunsData = ciyamlCheckRunNotComplete;
+
+ revert = Revert(
+ config: config,
+ retryOptions: const RetryOptions(
+ delayFactor: Duration.zero,
+ maxDelay: Duration.zero,
+ maxAttempts: 1,
+ ),
+ );
+ ValidationResult validationResult = await revert.validate(queryResult, revertPullRequest);
+
+ expect(validationResult.result, isFalse);
+ expect(validationResult.action, Action.IGNORE_TEMPORARILY);
+ expect(validationResult.message, 'Some of the required checks did not complete in time.');
+ });
+
test('Validation fails on pull request file lists not matching.', () async {
Map<String, dynamic> pullRequestJsonMap = jsonDecode(revertPullRequestJson) as Map<String, dynamic>;
github.PullRequest revertPullRequest = github.PullRequest.fromJson(pullRequestJsonMap);
@@ -158,6 +197,9 @@
githubService.pullRequestFilesMockList.add(originalPullRequestFilesSubsetJson);
githubService.pullRequestFilesMockList.add(revertPullRequestFilesJson);
+ // Need to set the mock checkRuns for required CheckRun validation
+ githubService.checkRunsData = ciyamlCheckRun;
+
ValidationResult validationResult = await revert.validate(queryResult, revertPullRequest);
assert(!validationResult.result);
assert(validationResult.action == Action.REMOVE_LABEL);
@@ -183,6 +225,9 @@
githubService.pullRequestFilesMockList.add(originalPullRequestFilesJson);
githubService.pullRequestFilesMockList.add(revertPullRequestFilesJson);
+ // Need to set the mock checkRuns for required CheckRun validation
+ githubService.checkRunsData = ciyamlCheckRun;
+
ValidationResult validationResult = await revert.validate(queryResult, revertPullRequest);
assert(validationResult.result);
assert(validationResult.message == 'Revert request has been verified and will be queued for merge.');
diff --git a/auto_submit/test/validations/revert_test_data.dart b/auto_submit/test/validations/revert_test_data.dart
index 4a681bd..a5e2bb3 100644
--- a/auto_submit/test/validations/revert_test_data.dart
+++ b/auto_submit/test/validations/revert_test_data.dart
@@ -510,3 +510,42 @@
}
]
""";
+
+const String ciyamlCheckRun = '''
+{
+ "total_count": 1,
+ "check_runs": [
+ {
+ "id": 8752872923,
+ "name": "ci.yaml validation",
+ "head_sha": "60612a38d705d00a234e0aabba08247fc0dda1ac",
+ "status": "completed",
+ "conclusion": "success",
+ "started_at": "2022-10-06T20:50:57Z",
+ "completed_at": "2022-10-06T20:51:40Z",
+ "check_suite": {
+ "id": 8654966141
+ }
+ }
+ ]
+}
+''';
+
+const String ciyamlCheckRunNotComplete = '''
+{
+ "total_count": 1,
+ "check_runs": [
+ {
+ "id": 8752872923,
+ "name": "ci.yaml validation",
+ "head_sha": "60612a38d705d00a234e0aabba08247fc0dda1ac",
+ "status": "in_progress",
+ "started_at": "2022-10-06T20:50:57Z",
+ "completed_at": "2022-10-06T20:51:40Z",
+ "check_suite": {
+ "id": 8654966141
+ }
+ }
+ ]
+}
+''';
