Refactor the Validation Service classes so they only process one type…

… of PR. (#2925) This is another part of the Revert implementation that will simplify the implementation and (later) separation while reducing interruption when revert is deployed as it will be deployed to it's own endpoint. While this does look like a lot of file changes, it is mostly existing logic refactored to accomadate the separate paths through the code. This change simply separates the regular pull request validation path and the revert validation path. It keeps the original logic as the updated Revert logic in a different branch that will rebased on this change. As it is currently Revert will need to be able to handle multiple passes through the Autosubmit due to the way Github generates events when labels are added, etc... *List which issues are fixed by this PR. You must list at least one issue.* Part of flutter/flutter#113867 *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].*
flutter · Jul 17, 2023 · cef706a · cef706a
1 parent 9081694
commit cef706a
Show file tree

Hide file tree

Showing 12 changed files with 909 additions and 495 deletions.
diff --git a/auto_submit/bin/server.dart b/auto_submit/bin/server.dart
@@ -8,6 +8,7 @@ import 'package:appengine/appengine.dart';
 import 'package:auto_submit/helpers.dart';
 import 'package:auto_submit/request_handling/authentication.dart';
 import 'package:auto_submit/requests/check_pull_request.dart';
+import 'package:auto_submit/requests/check_revert_request.dart';
 import 'package:auto_submit/requests/github_webhook.dart';
 import 'package:auto_submit/requests/readiness_check.dart';
 import 'package:auto_submit/service/config.dart';
@@ -43,6 +44,13 @@ Future<void> main() async {
           cronAuthProvider: authProvider,
         ).run,
       )
+      ..get(
+        '/check-revert-requests',
+        CheckRevertRequest(
+          config: config,
+          cronAuthProvider: authProvider,
+        ).run,
+      )
       ..get(
         '/readiness_check',
         ReadinessCheck(

diff --git a/auto_submit/lib/requests/check_pull_request.dart b/auto_submit/lib/requests/check_pull_request.dart
@@ -3,9 +3,14 @@
 // found in the LICENSE file.
 
 import 'dart:async';
+import 'dart:convert';
 
 import 'package:auto_submit/requests/check_request.dart';
 import 'package:auto_submit/service/approver_service.dart';
+import 'package:auto_submit/service/log.dart';
+import 'package:auto_submit/service/pull_request_validation_service.dart';
+import 'package:github/github.dart';
+import 'package:googleapis/pubsub/v1.dart' as pub;
 import 'package:shelf/shelf.dart';
 
 import '../request_handling/pubsub.dart';
@@ -30,4 +35,67 @@ class CheckPullRequest extends CheckRequest {
       config.kPullMesssageBatchSize,
     );
   }
+
+  ///TODO refactor this method out into the base class.
+  /// Process pull request messages from Pubsub.
+  Future<Response> process(
+    String pubSubSubscription,
+    int pubSubPulls,
+    int pubSubBatchSize,
+  ) async {
+    final Set<int> processingLog = <int>{};
+    final List<pub.ReceivedMessage> messageList = await pullMessages(
+      pubSubSubscription,
+      pubSubPulls,
+      pubSubBatchSize,
+    );
+    if (messageList.isEmpty) {
+      log.info('No messages are pulled.');
+      return Response.ok('No messages are pulled.');
+    }
+
+    log.info('Processing ${messageList.length} messages');
+
+    final PullRequestValidationService validationService = PullRequestValidationService(config);
+
+    final List<Future<void>> futures = <Future<void>>[];
+
+    for (pub.ReceivedMessage message in messageList) {
+      log.info(message.toJson());
+      assert(message.message != null);
+      assert(message.message!.data != null);
+      final String messageData = message.message!.data!;
+
+      final Map<String, dynamic> rawBody =
+          json.decode(String.fromCharCodes(base64.decode(messageData))) as Map<String, dynamic>;
+      log.info('request raw body = $rawBody');
+
+      final PullRequest pullRequest = PullRequest.fromJson(rawBody);
+
+      log.info('Processing message ackId: ${message.ackId}');
+      log.info('Processing mesageId: ${message.message!.messageId}');
+      log.info('Processing PR: $rawBody');
+      if (processingLog.contains(pullRequest.number)) {
+        // Ack duplicate.
+        log.info('Ack the duplicated message : ${message.ackId!}.');
+        await pubsub.acknowledge(
+          pubSubSubscription,
+          message.ackId!,
+        );
+
+        continue;
+      } else {
+        final ApproverService approver = approverProvider(config);
+        log.info('Checking auto approval of pull request: $rawBody');
+        await approver.autoApproval(pullRequest);
+        processingLog.add(pullRequest.number!);
+      }
+
+      futures.add(
+        validationService.processMessage(pullRequest, message.ackId!, pubsub),
+      );
+    }
+    await Future.wait(futures);
+    return Response.ok('Finished processing changes');
+  }
 }
diff --git a/auto_submit/lib/requests/check_request.dart b/auto_submit/lib/requests/check_request.dart
@@ -2,19 +2,13 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-import 'dart:convert';
-
 import 'package:auto_submit/server/authenticated_request_handler.dart';
 import 'package:auto_submit/service/approver_service.dart';
-import 'package:auto_submit/service/validation_service.dart';
-import 'package:github/github.dart';
 import 'package:googleapis/pubsub/v1.dart' as pub;
 import 'package:shelf/shelf.dart';
 
 import '../request_handling/pubsub.dart';
 
-import '../service/log.dart';
-
 abstract class CheckRequest extends AuthenticatedRequestHandler {
   const CheckRequest({
     required super.config,
@@ -29,72 +23,6 @@ abstract class CheckRequest extends AuthenticatedRequestHandler {
   @override
   Future<Response> get();
 
-  /// Process pull request messages from Pubsub.
-  Future<Response> process(
-    String pubSubSubscription,
-    int pubSubPulls,
-    int pubSubBatchSize,
-  ) async {
-    final Set<int> processingLog = <int>{};
-    final List<pub.ReceivedMessage> messageList = await pullMessages(
-      pubSubSubscription,
-      pubSubPulls,
-      pubSubBatchSize,
-    );
-    if (messageList.isEmpty) {
-      log.info('No messages are pulled.');
-      return Response.ok('No messages are pulled.');
-    }
-
-    log.info('Processing ${messageList.length} messages');
-    // TODO (ricardoamador): This validation service will be passed in by the calling class.
-    final ValidationService validationService = ValidationService(config);
-    final List<Future<void>> futures = <Future<void>>[];
-
-    for (pub.ReceivedMessage message in messageList) {
-      log.info(message.toJson());
-      assert(message.message != null);
-      assert(message.message!.data != null);
-      final String messageData = message.message!.data!;
-
-      final Map<String, dynamic> rawBody =
-          json.decode(String.fromCharCodes(base64.decode(messageData))) as Map<String, dynamic>;
-      log.info('request raw body = $rawBody');
-
-      final PullRequest pullRequest = PullRequest.fromJson(rawBody);
-
-      log.info('Processing message ackId: ${message.ackId}');
-      log.info('Processing mesageId: ${message.message!.messageId}');
-      log.info('Processing PR: $rawBody');
-      if (processingLog.contains(pullRequest.number)) {
-        // Ack duplicate.
-        log.info('Ack the duplicated message : ${message.ackId!}.');
-        await pubsub.acknowledge(
-          pubSubSubscription,
-          message.ackId!,
-        );
-
-        continue;
-      } else {
-        final ApproverService approver = approverProvider(config);
-        log.info('Checking auto approval of pull request: $rawBody');
-        await approver.autoApproval(pullRequest);
-        processingLog.add(pullRequest.number!);
-      }
-
-      futures.add(
-        validationService.processMessage(
-          // pullRequestMessage,
-          pullRequest,
-          message.ackId!,
-          pubsub,
-        ),
-      );
-    }
-    await Future.wait(futures);
-    return Response.ok('Finished processing changes');
-  }
-
   /// Pulls queued Pub/Sub messages.
   ///
   /// Pub/Sub pull request API doesn't guarantee returning all messages each time. This

diff --git a/auto_submit/lib/requests/check_revert_request.dart b/auto_submit/lib/requests/check_revert_request.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:convert';
+
 import 'package:auto_submit/request_handling/pubsub.dart';
 import 'package:auto_submit/requests/check_request.dart';
 import 'package:auto_submit/service/approver_service.dart';
+import 'package:auto_submit/service/log.dart';
+import 'package:auto_submit/service/revert_request_validation_service.dart';
+import 'package:github/github.dart';
+import 'package:googleapis/pubsub/v1.dart' as pub;
 import 'package:shelf/shelf.dart';
 
 // TODO (ricardoamador): provide implementation in https://github.com/flutter/flutter/issues/113867
@@ -30,4 +36,71 @@ class CheckRevertRequest extends CheckRequest {
       config.kPullMesssageBatchSize,
     );
   }
+
+  /// Process pull request messages from Pubsub.
+  Future<Response> process(
+    String pubSubSubscription,
+    int pubSubPulls,
+    int pubSubBatchSize,
+  ) async {
+    final Set<int> processingLog = <int>{};
+    final List<pub.ReceivedMessage> messageList = await pullMessages(
+      pubSubSubscription,
+      pubSubPulls,
+      pubSubBatchSize,
+    );
+    if (messageList.isEmpty) {
+      log.info('No messages are pulled.');
+      return Response.ok('No messages are pulled.');
+    }
+
+    log.info('Processing ${messageList.length} messages');
+
+    final RevertRequestValidationService validationService = RevertRequestValidationService(config);
+
+    final List<Future<void>> futures = <Future<void>>[];
+
+    for (pub.ReceivedMessage message in messageList) {
+      log.info(message.toJson());
+      assert(message.message != null);
+      assert(message.message!.data != null);
+      final String messageData = message.message!.data!;
+
+      final Map<String, dynamic> rawBody =
+          json.decode(String.fromCharCodes(base64.decode(messageData))) as Map<String, dynamic>;
+      log.info('request raw body = $rawBody');
+
+      final PullRequest pullRequest = PullRequest.fromJson(rawBody);
+
+      log.info('Processing message ackId: ${message.ackId}');
+      log.info('Processing mesageId: ${message.message!.messageId}');
+      log.info('Processing PR: $rawBody');
+      if (processingLog.contains(pullRequest.number)) {
+        // Ack duplicate.
+        log.info('Ack the duplicated message : ${message.ackId!}.');
+        await pubsub.acknowledge(
+          pubSubSubscription,
+          message.ackId!,
+        );
+
+        continue;
+      } else {
+        final ApproverService approver = approverProvider(config);
+        log.info('Checking auto approval of pull request: $rawBody');
+        await approver.autoApproval(pullRequest);
+        processingLog.add(pullRequest.number!);
+      }
+
+      futures.add(
+        validationService.processMessage(
+          // pullRequestMessage,
+          pullRequest,
+          message.ackId!,
+          pubsub,
+        ),
+      );
+    }
+    await Future.wait(futures);
+    return Response.ok('Finished processing changes');
+  }
 }
diff --git a/auto_submit/lib/service/github_service.dart b/auto_submit/lib/service/github_service.dart
@@ -106,11 +106,16 @@ class GithubService {
     return github.repositories.compareCommits(slug, refBase, refHead);
   }
 
-  /// Removes a lable for a pull request.
+  /// Removes a label from a pull request.
   Future<bool> removeLabel(RepositorySlug slug, int issueNumber, String label) async {
     return github.issues.removeLabelForIssue(slug, issueNumber, label);
   }
 
+  /// Add labels to a pull request.
+  Future<List<IssueLabel>> addLabels(RepositorySlug slug, int issueNumber, List<String> labels) async {
+    return github.issues.addLabelsToIssue(slug, issueNumber, labels);
+  }
+
   /// Create a comment for a pull request.
   Future<IssueComment> createComment(
     RepositorySlug slug,