fix revert enqueue cycle; ack pubsub (#4055)
Fixes https://github.com/flutter/flutter/issues/159518
diff --git a/auto_submit/lib/service/pull_request_validation_service.dart b/auto_submit/lib/service/pull_request_validation_service.dart
index 7a3f0dd..322c352 100644
--- a/auto_submit/lib/service/pull_request_validation_service.dart
+++ b/auto_submit/lib/service/pull_request_validation_service.dart
@@ -59,11 +59,14 @@
final github.RepositorySlug slug = messagePullRequest.base!.repo!.slug();
final int prNumber = messagePullRequest.number!;
+ // If a pull request is currently in the merge queue do not touch it. Let
+ // the merge queue merge it, or kick it out of the merge queue.
if (messagePullRequest.isMergeQueueEnabled) {
if (result.repository!.pullRequest!.isInMergeQueue) {
log.info(
'${slug.fullName}/$prNumber is already in the merge queue. Skipping.',
);
+ await pubsub.acknowledge(config.pubsubRevertRequestSubscription, ackId);
return;
}
}
@@ -140,7 +143,7 @@
await githubService.createComment(slug, prNumber, message);
log.info(message);
} else {
- log.info('Pull Request ${slug.fullName}/$prNumber was merged successfully!');
+ log.info('Pull Request ${slug.fullName}/$prNumber was ${processed.method.pastTenseLabel} successfully!');
log.info('Attempting to insert a pull request record into the database for $prNumber');
await insertPullRequestRecord(
config: config,
diff --git a/auto_submit/lib/service/revert_request_validation_service.dart b/auto_submit/lib/service/revert_request_validation_service.dart
index 125d69e..83d17a4 100644
--- a/auto_submit/lib/service/revert_request_validation_service.dart
+++ b/auto_submit/lib/service/revert_request_validation_service.dart
@@ -218,6 +218,18 @@
final GithubService githubService = await config.createGithubService(slug);
final int prNumber = messagePullRequest.number!;
+ // If a pull request is currently in the merge queue do not touch it. Let
+ // the merge queue merge it, or kick it out of the merge queue.
+ if (messagePullRequest.isMergeQueueEnabled) {
+ if (result.repository!.pullRequest!.isInMergeQueue) {
+ log.info(
+ '${slug.fullName}/$prNumber is already in the merge queue. Skipping.',
+ );
+ await pubsub.acknowledge(config.pubsubRevertRequestSubscription, ackId);
+ return;
+ }
+ }
+
// Check to make sure the repository allows review-less revert pull requests
// so that we can reassign if needed otherwise autoapprove the pull request.
final RepositoryConfiguration repositoryConfiguration = await config.getRepositoryConfiguration(slug);
@@ -304,7 +316,7 @@
final Message discordMessage = craftDiscordRevertMessage(messagePullRequest);
discordNotification.notifyDiscordChannelWebhook(jsonEncode(discordMessage.toJson()));
- log.info('Pull Request ${slug.fullName}/$prNumber was merged successfully!');
+ log.info('Pull Request ${slug.fullName}/$prNumber was ${processed.method.pastTenseLabel} successfully!');
log.info('Attempting to insert a pull request record into the database for $prNumber');
await insertPullRequestRecord(
config: config,
diff --git a/auto_submit/lib/service/validation_service.dart b/auto_submit/lib/service/validation_service.dart
index 9f1adfd..9e29565 100644
--- a/auto_submit/lib/service/validation_service.dart
+++ b/auto_submit/lib/service/validation_service.dart
@@ -101,10 +101,10 @@
} catch (e) {
final message = 'Failed to enqueue ${slug.fullName}/${restPullRequest.number} with $e';
log.severe(message);
- return (result: false, message: message);
+ return (result: false, message: message, method: SubmitMethod.enqueue);
}
- return (result: true, message: restPullRequest.title!);
+ return (result: true, message: restPullRequest.title!, method: SubmitMethod.enqueue);
}
Future<MergeResult> _mergePullRequest(int number, String commitMessage, github.RepositorySlug slug) async {
@@ -129,16 +129,16 @@
if (result != null && !merged) {
final String message = 'Failed to merge ${slug.fullName}/$number with ${result?.message}';
log.severe(message);
- return (result: false, message: message);
+ return (result: false, message: message, method: SubmitMethod.merge);
}
} catch (e) {
// Catch graphql client init exceptions.
final String message = 'Failed to merge ${slug.fullName}/$number with $e';
log.severe(message);
- return (result: false, message: message);
+ return (result: false, message: message, method: SubmitMethod.merge);
}
- return (result: true, message: commitMessage);
+ return (result: true, message: commitMessage, method: SubmitMethod.merge);
}
/// Insert a merged pull request record into the database.
@@ -183,9 +183,28 @@
}
}
+/// Method used to submit the PR for merging.
+enum SubmitMethod {
+ /// The PR is enqueued into the merge queue, and the merge queue is responsible
+ /// for merging the PR.
+ enqueue('enqueued'),
+
+ /// The PR is immediately merged into the target branch.
+ ///
+ /// This is the old method for merging PRs, used by repos where merge queues
+ /// are not (yet?) enabled.
+ merge('merged');
+
+ const SubmitMethod(this.pastTenseLabel);
+
+ /// The verb in past tense used to describe what happened to a PR when this
+ /// submit method was used, e.g. "merged".
+ final String pastTenseLabel;
+}
+
/// Small wrapper class to allow us to capture and create a comment in the PR with
/// the issue that caused the merge failure.
-typedef MergeResult = ({bool result, String message});
+typedef MergeResult = ({bool result, String message, SubmitMethod method});
/// Function signature that will be executed with retries.
typedef RetryHandler = Function();
diff --git a/auto_submit/test/service/pull_request_validation_service_test.dart b/auto_submit/test/service/pull_request_validation_service_test.dart
index 7e740bc..4124c67 100644
--- a/auto_submit/test/service/pull_request_validation_service_test.dart
+++ b/auto_submit/test/service/pull_request_validation_service_test.dart
@@ -645,6 +645,7 @@
mergeable: true,
);
+ unawaited(pubsub.publish(config.pubsubRevertRequestSubscription, pullRequest));
await validationService.processPullRequest(
config: config,
result: createQueryResult(flutterRequest),
@@ -658,6 +659,7 @@
logs,
contains('[INFO] auto_submit: flutter/flaux/0 is already in the merge queue. Skipping.'),
);
+ assert(pubsub.messagesQueue.isEmpty);
});
});
}
diff --git a/auto_submit/test/service/revert_request_validation_service_test.dart b/auto_submit/test/service/revert_request_validation_service_test.dart
index 87a3fe7..622cd16 100644
--- a/auto_submit/test/service/revert_request_validation_service_test.dart
+++ b/auto_submit/test/service/revert_request_validation_service_test.dart
@@ -9,6 +9,7 @@
import 'package:auto_submit/model/auto_submit_query_result.dart' as auto hide PullRequest;
import 'package:auto_submit/requests/github_pull_request_event.dart';
import 'package:auto_submit/model/discord_message.dart';
+import 'package:auto_submit/service/log.dart';
import 'package:auto_submit/service/revert_request_validation_service.dart';
import 'package:auto_submit/service/validation_service.dart';
import 'package:auto_submit/validations/validation.dart';
@@ -1233,6 +1234,59 @@
expect(githubService.labelRemoved, true);
assert(pubsub.messagesQueue.isEmpty);
});
+
+ test('Do not re-enqueue already enqueued pull requests', () async {
+ // Use a test slug that has MQ enabled
+ slug = RepositorySlug('flutter', 'flaux');
+
+ final logs = <String>[];
+ final logSub = log.onRecord.listen((record) {
+ logs.add(record.toString());
+ });
+
+ final FakePubSub pubsub = FakePubSub();
+
+ final PullRequestHelper flutterRequest = PullRequestHelper(
+ prNumber: 0,
+ lastCommitHash: oid,
+ reviews: <PullRequestReviewHelper>[],
+ isInMergeQueue: true,
+ );
+
+ final auto.QueryResult queryResult = createQueryResult(flutterRequest);
+
+ final PullRequest pullRequest = generatePullRequest(
+ prNumber: 0,
+ repoName: slug.name,
+ labelName: 'revert of',
+ body: sampleRevertBody.replaceAll('\n', ''),
+ );
+
+ final GithubPullRequestEvent githubPullRequestEvent = GithubPullRequestEvent(
+ pullRequest: pullRequest,
+ action: 'labeled',
+ sender: User(login: 'auto-submit[bot]'),
+ );
+
+ // Process the pull request
+ unawaited(pubsub.publish(config.pubsubRevertRequestSubscription, pullRequest));
+ await validationService.processRevertOfRequest(
+ result: queryResult,
+ githubPullRequestEvent: githubPullRequestEvent,
+ ackId: 'test',
+ pubsub: pubsub,
+ );
+ await logSub.cancel();
+
+ // Expectations
+ expect(
+ logs,
+ contains('[INFO] auto_submit: flutter/flaux/0 is already in the merge queue. Skipping.'),
+ );
+ expect(githubService.issueComment, isNull);
+ expect(githubService.labelRemoved, false);
+ assert(pubsub.messagesQueue.isEmpty);
+ });
});
group('Craft discord message', () {
