Skip to content

Commit 7cbf0ea

Browse files
eyebrowsoffireeyebrowsoffire
authored
Overhaul CICD label management for presubmits (#5028)
This change overhauls the state management for the `CICD` label to ensure trusted users can run CI seamlessly and external contributors require explicit approval. - PR Opened: Privileged users (non-draft) auto-trigger CI and get the label. Non-privileged or draft PRs go to awaiting state. - Changes Pushed (Synchronize): Privileged users trigger CI if label is present. Non-privileged users have the label removed and go to awaiting state. - Edits: Removed automatic label adding on base branch changes. - Consolidated usage of `_scheduleIfMergeable` to ensure consistent conflict checking and label processing. - Added flowchart documentation in `app_dart/docs/cicd_flowchart.md`. Once this is merged and deployed, we can remove the "remove CICD label" workflows from the other repos since that's all handled in cocoon now.
1 parent f7d5009 commit 7cbf0ea

3 files changed

Lines changed: 133 additions & 42 deletions

File tree

‎app_dart/docs/cicd_flowchart.md‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
# CICD Label Flowchart
2+
3+
This flowchart describes the logic for managing the `CICD` label in Cocoon for running presubmit CI.
4+
5+
```mermaid
6+
flowchart TD
7+
PR_Opened([Event: PR Opened]) --> Is_Draft{Is Draft?}
8+
9+
Is_Draft -- No --> Is_Privileged_Open{Is Privileged?}
10+
Is_Draft -- Yes --> Create_Awaiting[Create Awaiting Check Run]
11+
12+
Is_Privileged_Open -- Yes --> Add_Label[Add CICD Label]
13+
Add_Label --> Start_Pre[Start Presubmits]
14+
15+
Is_Privileged_Open -- No --> Create_Awaiting
16+
17+
Create_Awaiting --> State_Awaiting((State: Awaiting))
18+
Start_Pre --> State_Running((State: Running))
19+
20+
State_Awaiting --> Label_Added([Event: CICD Label Added])
21+
Label_Added --> Resolve_Awaiting[Resolve Awaiting Check Run]
22+
Resolve_Awaiting --> Start_Pre
23+
24+
State_Running --> Changes_Pushed([Event: Changes Pushed])
25+
Changes_Pushed --> Is_Privileged_Push{Is Privileged?}
26+
Remove_Label --> Create_Awaiting
27+
28+
Is_Privileged_Push -- No --> Remove_Label[Remove CICD Label]
29+
30+
Is_Privileged_Push -- Yes --> Label_Present{Has CICD Label?}
31+
Label_Present -- Yes --> Start_Pre
32+
Label_Present -- No --> Create_Awaiting
33+
```

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

Lines changed: 44 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,8 @@ final class GithubWebhookSubscription extends SubscriptionHandler {
173173
/// retried with exponential backoff within that time period. The GoB mirror
174174
/// should be caught up within that time frame via either the internal
175175
/// mirroring service or [VacuumGithubCommits].
176+
///
177+
/// See docs/cicd_flowchart.md for the flowchart of CICD label management.
176178
Future<Response> _handlePullRequest(String rawRequest) async {
177179
final pullRequestEvent = _getPullRequestEvent(rawRequest);
178180
if (pullRequestEvent == null) {
@@ -199,16 +201,20 @@ final class GithubWebhookSubscription extends SubscriptionHandler {
199201
final result = await _processPullRequestClosed(pullRequestEvent);
200202
return result.toResponse();
201203
case 'edited':
202-
if (pullRequestEvent.changes != null &&
203-
pullRequestEvent.changes!.base != null) {
204-
await _addCICDForRollersAndMembers(pullRequestEvent);
205-
}
206-
await scheduler.createAwaitingCicdLabelCheckRun(slug, pr.head!.sha!);
207204
await _checkForTests(pullRequestEvent);
208205
break;
209206
case 'opened':
210-
await _addCICDForRollersAndMembers(pullRequestEvent);
211-
await scheduler.createAwaitingCicdLabelCheckRun(slug, pr.head!.sha!);
207+
final isPrivileged = await _isPrivilegedUser(pr, slug);
208+
if (!pr.draft! && isPrivileged) {
209+
final gitHubClient = await config.createGitHubClient(pullRequest: pr);
210+
await gitHubClient.issues.addLabelsToIssue(slug, pr.number!, [
211+
'CICD',
212+
]);
213+
log.debug('Added CICD label to PR ${pr.number}');
214+
await _scheduleIfMergeable(pullRequestEvent);
215+
} else {
216+
await scheduler.createAwaitingCicdLabelCheckRun(slug, pr.head!.sha!);
217+
}
212218
await _checkForTests(pullRequestEvent);
213219
await _tryReleaseApproval(pullRequestEvent);
214220
break;
@@ -242,7 +248,31 @@ final class GithubWebhookSubscription extends SubscriptionHandler {
242248
await _respondToPullRequestDequeued(pullRequestEvent);
243249
break;
244250
case 'synchronize':
245-
await _addCICDForRollersAndMembers(pullRequestEvent);
251+
final isPrivileged = await _isPrivilegedUser(pr, slug);
252+
final hasLabel =
253+
pr.labels?.any((l) => l.name == Config.kCicdLabel) ?? false;
254+
final githubService = await config.createGithubService(slug);
255+
256+
if (isPrivileged) {
257+
if (hasLabel) {
258+
await _scheduleIfMergeable(pullRequestEvent);
259+
} else {
260+
await scheduler.createAwaitingCicdLabelCheckRun(
261+
slug,
262+
pr.head!.sha!,
263+
);
264+
}
265+
} else {
266+
if (hasLabel) {
267+
await githubService.removeLabel(
268+
slug,
269+
pr.number!,
270+
Config.kCicdLabel,
271+
);
272+
log.debug('Removed CICD label from PR ${pr.number}');
273+
}
274+
await scheduler.createAwaitingCicdLabelCheckRun(slug, pr.head!.sha!);
275+
}
246276
break;
247277
// Ignore the rest of the events.
248278
case 'ready_for_review':
@@ -580,27 +610,16 @@ final class GithubWebhookSubscription extends SubscriptionHandler {
580610
);
581611
}
582612

583-
Future<void> _addCICDForRollersAndMembers(
584-
PullRequestEvent pullRequestEvent,
585-
) async {
586-
final pr = pullRequestEvent.pullRequest!;
587-
final slug = pr.base!.repo!.slug();
588-
final author = pr.user!.login!;
589-
final githubService = await config.createGithubService(slug);
590-
613+
/// Returns true if the user is a roller or a member of flutter-hackers.
614+
Future<bool> _isPrivilegedUser(PullRequest pr, RepositorySlug slug) async {
591615
final isRoller = config.rollerAccounts.contains(pr.user!.login);
616+
final githubService = await config.createGithubService(slug);
592617
final isFlutterHacker = await githubService.isTeamMember(
593618
'flutter-hackers',
594-
author,
595-
slug.owner,
619+
pr.user!.login!,
620+
'flutter',
596621
);
597-
log.debug('isRoller=$isRoller, isFlutterHacker=$isFlutterHacker');
598-
599-
if (config.supportedRepos.contains(slug) && (isRoller || isFlutterHacker)) {
600-
final gitHubClient = await config.createGitHubClient(pullRequest: pr);
601-
await gitHubClient.issues.addLabelsToIssue(slug, pr.number!, ['CICD']);
602-
log.debug('Added CICD label to PR $pr.number');
603-
}
622+
return isRoller || isFlutterHacker;
604623
}
605624

606625
Future<void> _checkForTests(PullRequestEvent pullRequestEvent) async {

‎app_dart/test/request_handlers/github/webhook_subscription_test.dart‎

Lines changed: 56 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -3755,7 +3755,7 @@ void foo() {
37553755
});
37563756

37573757
test(
3758-
'opened PR with adds CICD label if author is member of flutter-hackers',
3758+
'opened PR with adds CICD label and schedules tests if author is member of flutter-hackers',
37593759
() async {
37603760
tester.message = generateGithubWebhookMessage(
37613761
action: 'opened',
@@ -3766,7 +3766,8 @@ void foo() {
37663766

37673767
verify(
37683768
issuesService.addLabelsToIssue(Config.flutterSlug, 123, ['CICD']),
3769-
);
3769+
).called(1);
3770+
expect(scheduler.triggerPresubmitTargetsCnt, 1);
37703771
},
37713772
);
37723773
test(
@@ -3861,50 +3862,87 @@ void foo() {
38613862
);
38623863

38633864
test(
3864-
'synchronize event with CICD label does not schedules tests',
3865+
'synchronize event by non-privileged user removes CICD label and creates awaiting checkrun',
38653866
() async {
3867+
githubService.checkRunsMock = '{"total_count": 0, "check_runs": [{}]}';
38663868
tester.message = generateGithubWebhookMessage(
38673869
action: 'synchronize',
38683870
withCicdLabel: true,
38693871
);
38703872

38713873
await tester.post(webhook);
3874+
3875+
expect(githubService.removedLabels, [
3876+
(Config.flutterSlug, 123, Config.kCicdLabel),
3877+
]);
3878+
verify(
3879+
mockGithubChecksUtil.createCheckRun(
3880+
any,
3881+
any,
3882+
any,
3883+
'Awaiting CICD label',
3884+
output: anyNamed('output'),
3885+
),
3886+
).called(1);
38723887
expect(scheduler.triggerPresubmitTargetsCnt, 0);
38733888
},
38743889
);
38753890

38763891
test(
3877-
'synchronize event adds CICD label if author is member of flutter-hackers',
3892+
'synchronize event by non-privileged user without label creates awaiting checkrun',
38783893
() async {
3894+
githubService.checkRunsMock = '{"total_count": 0, "check_runs": [{}]}';
38793895
tester.message = generateGithubWebhookMessage(
38803896
action: 'synchronize',
3881-
login: 'test-flutter-hacker',
3897+
withCicdLabel: false,
38823898
);
38833899

38843900
await tester.post(webhook);
3901+
38853902
verify(
3886-
issuesService.addLabelsToIssue(Config.flutterSlug, 123, ['CICD']),
3887-
);
3903+
mockGithubChecksUtil.createCheckRun(
3904+
any,
3905+
any,
3906+
any,
3907+
'Awaiting CICD label',
3908+
output: anyNamed('output'),
3909+
),
3910+
).called(1);
3911+
expect(scheduler.triggerPresubmitTargetsCnt, 0);
38883912
},
38893913
);
3914+
38903915
test(
3891-
'synchronize event does not add CICD label if author is not member of flutter-hackers',
3916+
'synchronize event by privileged user with label schedules tests',
38923917
() async {
3893-
tester.message = generateGithubWebhookMessage(action: 'synchronize');
3918+
tester.message = generateGithubWebhookMessage(
3919+
action: 'synchronize',
3920+
login: 'test-flutter-hacker',
3921+
withCicdLabel: true,
3922+
);
38943923

38953924
await tester.post(webhook);
3925+
3926+
expect(scheduler.triggerPresubmitTargetsCnt, 1);
38963927
verifyNever(
3897-
issuesService.addLabelsToIssue(Config.flutterSlug, 123, any),
3928+
mockGithubChecksUtil.createCheckRun(
3929+
any,
3930+
any,
3931+
any,
3932+
'Awaiting CICD label',
3933+
output: anyNamed('output'),
3934+
),
38983935
);
38993936
},
39003937
);
39013938

39023939
test(
3903-
'opened PR without CICD label creates "Awaiting CICD label" checkrun',
3940+
'synchronize event by privileged user without label creates awaiting checkrun',
39043941
() async {
39053942
githubService.checkRunsMock = '{"total_count": 0, "check_runs": [{}]}';
39063943
tester.message = generateGithubWebhookMessage(
3907-
action: 'opened',
3944+
action: 'synchronize',
3945+
login: 'test-flutter-hacker',
39083946
withCicdLabel: false,
39093947
);
39103948

@@ -3919,16 +3957,17 @@ void foo() {
39193957
output: anyNamed('output'),
39203958
),
39213959
).called(1);
3960+
expect(scheduler.triggerPresubmitTargetsCnt, 0);
39223961
},
39233962
);
39243963

39253964
test(
3926-
'opened PR with CICD label STILL creates "Awaiting CICD label" checkrun if not exists',
3965+
'opened PR without CICD label creates "Awaiting CICD label" checkrun',
39273966
() async {
39283967
githubService.checkRunsMock = '{"total_count": 0, "check_runs": [{}]}';
39293968
tester.message = generateGithubWebhookMessage(
39303969
action: 'opened',
3931-
withCicdLabel: true,
3970+
withCicdLabel: false,
39323971
);
39333972

39343973
await tester.post(webhook);
@@ -3946,12 +3985,12 @@ void foo() {
39463985
);
39473986

39483987
test(
3949-
'edited PR without CICD label creates "Awaiting CICD label" checkrun if not exists',
3988+
'opened PR with CICD label STILL creates "Awaiting CICD label" checkrun if not exists',
39503989
() async {
39513990
githubService.checkRunsMock = '{"total_count": 0, "check_runs": [{}]}';
39523991
tester.message = generateGithubWebhookMessage(
3953-
action: 'edited',
3954-
withCicdLabel: false,
3992+
action: 'opened',
3993+
withCicdLabel: true,
39553994
);
39563995

39573996
await tester.post(webhook);

0 commit comments

Comments
 (0)