Remove revert bot from autosubmit. (#5091)

This functionality has been replaced by a GitHub action, so we're going to remove this feature from cocoon.

Author: eyebrowsoffire

app_dart/lib/src/request_handlers/github/webhook_subscription.dart

@@ -388,30 +388,6 @@ final class GithubWebhookSubscription extends SubscriptionHandler {
 
     log.debug('Pull request ${pr.number} was closed and merged.');
 
-    // To avoid polluting the repo with temporary revert branches, delete the
-    // branch after the reverted PR is merged.
-    //
-    // This can be done no ealier than the event declaring the PR both
-    // 'closed' and merged, because:
-    //
-    // * If the branch is deleted before the PR reaches 'closed', then GitHub
-    //   will force-close the PR because the branch is the source of all the
-    //   code changes in the PR. In a previous iteration, Cocoon used to
-    //   delete the branch immediately after merging it. However, with merge
-    //   queues a PR is not merged by Cocoon anymore. It stays open while in
-    //   the merge queue. Deleting the branch while in the queue would close
-    //   the PR and not merge it.
-    // * If a PR is closed but not merged, the author may still want to reopen
-    //   the PR. That would not be possible if the source branch was deleted.
-    final isRevertPullRequest =
-        pr.labels?.any((label) => label.name == Config.revertOfLabel) == true;
-    if (isRevertPullRequest) {
-      log.info('Revert merged successfully, deleting branch ${pr.head!.ref!}');
-      final slug = pullRequestEvent.repository!.slug();
-      final githubService = await config.createGithubService(slug);
-      await githubService.deleteBranch(slug, pr.head!.ref!);
-    }
-
     if (await _commitExistsInGob(pr)) {
       log.debug(
         'Merged commit was found on GoB mirror. Scheduling postsubmit tasks...',

app_dart/lib/src/service/config.dart

@@ -152,13 +152,6 @@ interface class Config extends DynamicallyUpdatedConfig {
     return defaultBranches[slug] ?? kDefaultBranchName;
   }
 
-  // The name of the bot that generates automated revert requests.
-  String get autosubmitBot => 'auto-submit[bot]';
-
-  // The name of the label that the bot uses to identify the automatically
-  // created pull request.
-  static const String revertOfLabel = 'revert of';
-
   /// Memorystore subcache name to store [CocoonConfig] values in.
   static const String configCacheName = 'config';
 

app_dart/lib/src/service/scheduler.dart

@@ -391,18 +391,6 @@ class Scheduler {
           unlockMergeGroup = true;
         }
 
-        // Both the author and label should be checked to make sure that no one is
-        // attempting to get a pull request without check through.
-        if (pullRequest.user!.login == _config.autosubmitBot &&
-            pullRequest.labels!.any(
-              (element) => element.name == Config.revertOfLabel,
-            )) {
-          log.info(
-            'Skipping generating the full set of checks for revert request.',
-          );
-          unlockMergeGroup = true;
-          break;
-        }
         // Feature request: skip engine builds and tests in monorepo if the PR only contains framework related
         // files.
         // NOTE: This creates an empty staging doc for the engine builds as staging is handled on check_run completion
@@ -1415,84 +1403,73 @@ detailsUrl: $detailsUrl
     required _FlutterRepoTestsToRun testsToRun,
   }) async {
     try {
-      // Both the author and label should be checked to make sure that no one is
-      // attempting to get a pull request without check through.
-      if (pullRequest.user!.login == _config.autosubmitBot &&
-          pullRequest.labels!.any(
-            (element) => element.name == Config.revertOfLabel,
-          )) {
-        log.info(
-          '$logCrumb: skipping generating the full set of checks for revert request.',
-        );
-      } else {
-        // Schedule the tests that would have run in a call to triggerPresubmitTargets - but for both the
-        // engine and the framework.
-        var presubmitTargets = await _getTestsForStage(
-          pullRequest,
-          CiStage.fusionTests,
-          skipEngine:
-              testsToRun != _FlutterRepoTestsToRun.engineTestsAndFrameworkTests,
-        );
-
-        // Create the document for tracking test check runs.
-        final List<String> tasks;
-        if (testsToRun == _FlutterRepoTestsToRun.frameworkFlutterAnalyzeOnly) {
-          const linuxAnalyze = 'Linux analyze';
-          final singleTarget = presubmitTargets.firstWhereOrNull(
-            (t) => t.name == linuxAnalyze,
-          );
-          if (singleTarget == null) {
-            log.warn('No target found named "$linuxAnalyze"');
-            tasks = [];
-            presubmitTargets = [];
-          } else {
-            log.info('Only running target "$linuxAnalyze"');
-            tasks = [linuxAnalyze];
-            presubmitTargets = [singleTarget];
-          }
-        } else {
-          tasks = [...presubmitTargets.map((t) => t.name)];
-        }
+      // Schedule the tests that would have run in a call to triggerPresubmitTargets - but for both the
+      // engine and the framework.
+      var presubmitTargets = await _getTestsForStage(
+        pullRequest,
+        CiStage.fusionTests,
+        skipEngine:
+            testsToRun != _FlutterRepoTestsToRun.engineTestsAndFrameworkTests,
+      );
 
-        await UnifiedCheckRun.initializeCiStagingDocument(
-          firestoreService: _firestore,
-          slug: pullRequest.base!.repo!.slug(),
-          sha: pullRequest.head!.sha!,
-          stage: CiStage.fusionTests,
-          tasks: tasks,
-          config: _config,
-          pullRequest: pullRequest,
-          checkRun: checkRunGuard,
+      // Create the document for tracking test check runs.
+      final List<String> tasks;
+      if (testsToRun == _FlutterRepoTestsToRun.frameworkFlutterAnalyzeOnly) {
+        const linuxAnalyze = 'Linux analyze';
+        final singleTarget = presubmitTargets.firstWhereOrNull(
+          (t) => t.name == linuxAnalyze,
         );
-
-        // Here is where it gets fun: how do framework tests* know what engine
-        // artifacts to fetch and use on CI? For presubmits on flutter/flutter;
-        // see https://github.com/flutter/flutter/issues/164031.
-        //
-        // *In theory, also engine tests, but engine tests build from the engine
-        // from source and rely on remote-build execution (RBE) for builds to
-        // fast and cached.
-        final EngineArtifacts engineArtifacts;
-        if (testsToRun != _FlutterRepoTestsToRun.engineTestsAndFrameworkTests) {
-          // Use the engine that this PR was branched off of.
-          engineArtifacts = EngineArtifacts.usingExistingEngine(
-            commitSha: pullRequest.base!.sha!,
-          );
+        if (singleTarget == null) {
+          log.warn('No target found named "$linuxAnalyze"');
+          tasks = [];
+          presubmitTargets = [];
         } else {
-          // Use the engine that was built from source *for* this PR.
-          engineArtifacts = EngineArtifacts.builtFromSource(
-            commitSha: pullRequest.head!.sha!,
-          );
+          log.info('Only running target "$linuxAnalyze"');
+          tasks = [linuxAnalyze];
+          presubmitTargets = [singleTarget];
         }
+      } else {
+        tasks = [...presubmitTargets.map((t) => t.name)];
+      }
 
-        await _luciBuildService.scheduleTryBuilds(
-          targets: presubmitTargets,
-          pullRequest: pullRequest,
-          engineArtifacts: engineArtifacts,
-          checkRunGuard: checkRunGuard,
-          stage: CiStage.fusionTests,
+      await UnifiedCheckRun.initializeCiStagingDocument(
+        firestoreService: _firestore,
+        slug: pullRequest.base!.repo!.slug(),
+        sha: pullRequest.head!.sha!,
+        stage: CiStage.fusionTests,
+        tasks: tasks,
+        config: _config,
+        pullRequest: pullRequest,
+        checkRun: checkRunGuard,
+      );
+
+      // Here is where it gets fun: how do framework tests* know what engine
+      // artifacts to fetch and use on CI? For presubmits on flutter/flutter;
+      // see https://github.com/flutter/flutter/issues/164031.
+      //
+      // *In theory, also engine tests, but engine tests build from the engine
+      // from source and rely on remote-build execution (RBE) for builds to
+      // fast and cached.
+      final EngineArtifacts engineArtifacts;
+      if (testsToRun != _FlutterRepoTestsToRun.engineTestsAndFrameworkTests) {
+        // Use the engine that this PR was branched off of.
+        engineArtifacts = EngineArtifacts.usingExistingEngine(
+          commitSha: pullRequest.base!.sha!,
+        );
+      } else {
+        // Use the engine that was built from source *for* this PR.
+        engineArtifacts = EngineArtifacts.builtFromSource(
+          commitSha: pullRequest.head!.sha!,
         );
       }
+
+      await _luciBuildService.scheduleTryBuilds(
+        targets: presubmitTargets,
+        pullRequest: pullRequest,
+        engineArtifacts: engineArtifacts,
+        checkRunGuard: checkRunGuard,
+        stage: CiStage.fusionTests,
+      );
     } on FormatException catch (e, s) {
       log.warn(
         '$logCrumb: FormatException encountered when scheduling presubmit '

app_dart/test/request_handlers/github/webhook_subscription_test.dart

@@ -342,10 +342,6 @@ void main() {
           merged: true,
           baseSha: 'abc',
           mergeCommitSha: 'cde',
-
-          // Just spelling this out here, because this test specifically tests a
-          // non-revert PR.
-          withRevertOf: false,
         );
 
         await tester.post(webhook);
@@ -358,52 +354,6 @@ void main() {
       },
     );
 
-    test('Removes temporary revert branches upon merging the PR', () async {
-      const issueNumber = 123;
-
-      firestore.putDocument(generateFirestoreCommit(1));
-
-      tester.message = generateGithubWebhookMessage(
-        action: 'closed',
-        number: issueNumber,
-        baseRef: 'dev',
-        merged: true,
-        baseSha: 'abc',
-        mergeCommitSha: 'cde',
-        withRevertOf: true,
-        headRef: 'test/headref',
-      );
-
-      await tester.post(webhook);
-
-      // This was a merged revert PR. The temp branch should be deleted.
-      expect(githubService.deletedBranches, [
-        (RepositorySlug('flutter', 'flutter'), 'test/headref'),
-      ]);
-    });
-
-    test(
-      'Does NOT remove temporary revert branches upon closing a revert PR because the PR may be manually reopened',
-      () async {
-        const issueNumber = 123;
-
-        tester.message = generateGithubWebhookMessage(
-          action: 'closed',
-          number: issueNumber,
-          baseRef: 'dev',
-          merged: false,
-          baseSha: 'sha1',
-          mergeCommitSha: 'sha2',
-          withRevertOf: true,
-        );
-
-        await tester.post(webhook);
-
-        // This was a closed (not merged) revert PR, so no branches deleted.
-        expect(githubService.deletedBranches, isEmpty);
-      },
-    );
-
     test('Acts on opened against master when default is main', () async {
       const issueNumber = 123;
 

app_dart/test/service/scheduler_test.dart

@@ -2991,83 +2991,6 @@ targets:
         },
       );
 
-      test('Do not schedule other targets on revert request.', () async {
-        final releasePullRequest = generatePullRequest(
-          labels: [IssueLabel(name: 'revert of')],
-        );
-
-        releasePullRequest.user = User(login: 'auto-submit[bot]');
-
-        await scheduler.triggerPresubmitTargets(
-          pullRequest: releasePullRequest,
-        );
-        expect(
-          verify(
-            mockGithubChecksUtil.createCheckRun(
-              any,
-              any,
-              any,
-              captureAny,
-              output: captureAnyNamed('output'),
-            ),
-          ).captured,
-          <Object?>[
-            Config.kMergeQueueLockName,
-            const CheckRunOutput(
-              title: Config.kMergeQueueLockName,
-              summary: Scheduler.kMergeQueueLockDescription,
-            ),
-            Config.kDashboardCheckName,
-            const CheckRunOutput(
-              title: Config.kDashboardCheckName,
-              summary: Scheduler.kDashboardChecksDescription,
-            ),
-            Config.kCiYamlCheckName,
-            // No other targets should be created.
-            const CheckRunOutput(
-              title: Config.kCiYamlCheckName,
-              summary:
-                  'If this check is stuck pending, push an empty commit to retrigger the checks',
-            ),
-          ],
-        );
-      });
-
-      test('Unlocks merge group on revert request.', () async {
-        final releasePullRequest = generatePullRequest(
-          labels: [IssueLabel(name: 'revert of')],
-        );
-
-        releasePullRequest.user = User(login: 'auto-submit[bot]');
-
-        await scheduler.triggerPresubmitTargets(
-          pullRequest: releasePullRequest,
-        );
-        expect(
-          verify(
-            mockGithubChecksUtil.updateCheckRun(
-              any,
-              any,
-              any,
-              status: captureAnyNamed('status'),
-              conclusion: captureAnyNamed('conclusion'),
-              output: captureAnyNamed('output'),
-            ),
-          ).captured,
-          <Object?>[
-            CheckRunStatus.completed,
-            CheckRunConclusion.success,
-            null,
-            CheckRunStatus.completed,
-            CheckRunConclusion.success,
-            null,
-            CheckRunStatus.completed,
-            CheckRunConclusion.success,
-            null,
-          ],
-        );
-      });
-
       test(
         'filters out presubmit targets that do not exist in main and do not filter targets not in main',
         () async {

auto_submit/bin/server.dart

@@ -8,7 +8,6 @@ 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';
@@ -40,10 +39,6 @@ Future<void> main() async {
         '/check-pull-request',
         CheckPullRequest(config: config, cronAuthProvider: authProvider).run,
       )
-      ..get(
-        '/check-revert-requests',
-        CheckRevertRequest(config: config, cronAuthProvider: authProvider).run,
-      )
       ..get('/readiness_check', ReadinessCheck(config: config).run);
     await serveHandler(router.call);
   });