[web] Add the flutter-gold check in web engine PRs #1505

mdebbar · 2021-12-07T17:59:02Z

Add a cron entry to send gold status to flutter/engine PRs that run web engine tests.

Piinks

This will need to also update the URLs/messages in push_gold_status_to_github.dart and config.dart, currently they point to the framework instance of Gold, I think you will want them to point to the engine instance.

The pullRequestChecksQuery is also specific to flutter/flutter. I think this needs a bit more refactoring.

Tests should be easy, you can probably just copy-paste all of the framework version of these tests, and change the repo to flutter/engine to make sure everything is working: https://github.com/flutter/cocoon/blob/main/app_dart/test/request_handlers/push_gold_status_to_github_test.dart

Piinks · 2021-12-07T18:26:34Z

app_dart/cron.yaml

+- description: sends pr-specific gold status to GitHub to annotate engine PRs and commits
+  url: /api/push-gold-status-to-github?repo=flutter/engine
+  schedule: every 5 minutes


Having separate cron jobs are probably less ideal.
Gold already has a very tight budget for the requests we make to it, so if these were running simultaneously, we might go over.
We should do this more like check_for_waiting_pull_requests.dart, with one cron job running through the supported repos.

Here is where that is happening:
https://github.com/flutter/cocoon/blob/main/app_dart/lib/src/request_handlers/check_for_waiting_pull_requests.dart#L44

We send a separate request to Gold for each PR, right? If that's the case, then the number of cron jobs doesn't matter. If there are 10 PRs in flutter and 10 PRs in the engine, we'll be sending 20 requests to Gold regardless of whether we have a single cron job or separate ones. I might be missing something here, so please correct me if I'm wrong :)

We send a separate request to Gold for each PR, right?

We do, but one at a time.

There is a strict time threshold on those Gold endpoints. So the quantity does not matter, 10 framework + 10 engine PRs = 20, but if they are running in parallel like this, they could max out that rate limit.

Looking at the code, I see the requests being made sequentially anyway (unless I'm misunderstanding that code):

cocoon/app_dart/lib/src/request_handlers/push_gold_status_to_github.dart

Lines 53 to 55 in c9a5c6d

await for (PullRequest pr in gitHubClient.pullRequests.list(slug)) {

// Get last known Gold status from datastore.

final GithubGoldStatusUpdate lastUpdate = await datastore.queryLastGoldUpdate(slug, pr);

I'm happy to unify the cron jobs into one. Just trying to fully understand the reason behind it :)

Yes, they are done sequentially. But if you have two cron jobs, you will have two running simultaneously.

It is the requests we make to the Gold service itself (not github or the datastore) that are so restricted.

cocoon/app_dart/lib/src/request_handlers/push_gold_status_to_github.dart

Line 203 in c9a5c6d

Uri.parse('https://flutter-gold.skia.org/json/v1/changelist_summary/github/${pr.number}');

Since it is a separate Gold instance, it may not be a problem, but it causes a lot of issues if we hit that rate limit.
I never noticed the build status were separate cron jobs, so if it fits the pattern and does not put us over the limit, I am fine with keeping them separate. :)

Done.

Makes perfect sense now, thanks for explaining!

app_dart/lib/src/request_handlers/push_gold_status_to_github.dart

mdebbar · 2021-12-07T19:12:29Z

Tests should be easy, you can probably just copy-paste all of the framework version of these tests, and change the repo to flutter/engine to make sure everything is working: https://github.com/flutter/cocoon/blob/main/app_dart/test/request_handlers/push_gold_status_to_github_test.dart

I wanted to first send the PR to make sure I'm on the right path before I add/update tests.

mdebbar · 2021-12-07T19:25:10Z

@Piinks there's a check for master here:

cocoon/app_dart/lib/src/request_handlers/push_gold_status_to_github.dart

Line 68 in c9a5c6d

if (pr.base!.ref != 'master') {

I'm gonna change it to:

if (pr.base!.ref != 'master' || pr.base!.ref != 'main') {

Does that sound good to you?

Piinks · 2021-12-07T19:25:23Z

I wanted to first send the PR to make sure I'm on the right path before I add/update tests.

For sure! I was hoping to save you some time. 😜

Piinks · 2021-12-07T19:26:12Z

if (pr.base!.ref != 'master' || pr.base!.ref != 'main') {

Does that sound good to you?

SGTM! I think the framework will make that change to main eventually. I hope. :) Thanks!

CaseyHillers · 2021-12-08T19:27:02Z

app_dart/lib/src/request_handlers/push_gold_status_to_github.dart

-      if (pr.base!.ref != 'master') {
-        log.fine('This change is not staged to land on master, skipping.');
+      final String? baseRef = pr.base!.ref;
+      if (baseRef != 'master' && baseRef != 'main') {


Suggested change

if (baseRef != 'master' && baseRef != 'main') {

if (baseRef != Config.defaultBranch(pr.base!.repo!.slug())) {

Nice! I'm assuming you'll update the defaultBranch method when flutter/flutter switches to the main branch?

Yep! Infra is maintaining this API for the default branch instead of hard coding it everywhere

mdebbar

@Piinks I had to adjust some things in the tests to be able to test for multiple repos. Could you please take a look to make sure I didn't mess things up?

mdebbar · 2021-12-16T20:23:14Z

app_dart/lib/src/request_handlers/push_gold_status_to_github.dart

      commits(last: 1) {
        nodes {
          commit {
            # (appId: 64368) == flutter-dashboard. We only care about


@Piinks Do we need to change the appId for the engine?

I do not think so, it is the same in check_for_waiting_pull_requests_queries.dart, which is used by the merge bot across multiple repositories. @godofredoc would know for sure though.

Piinks · 2021-12-22T18:32:00Z

app_dart/lib/src/request_handlers/push_gold_status_to_github.dart


-      if (pr.base!.ref != 'master') {
-        log.fine('This change is not staged to land on master, skipping.');
+      final String defaultBranch = Config.defaultBranch(slug);


Piinks · 2021-12-22T18:36:07Z

app_dart/lib/src/request_handlers/push_gold_status_to_github.dart


-  String _getTriageUrl(int? number) {
-    return 'https://flutter-gold.skia.org/cl/github/$number';
+  String _getTriageUrl(RepositorySlug slug, int? number) {


We should probably change this to be non nullable. It would be invalid if this got a null PR number.

Piinks · 2021-12-22T18:37:31Z

app_dart/lib/src/request_handlers/push_gold_status_to_github.dart

+Future<Map<String, dynamic>?> _queryGraphQL(
+  GraphQLClient client,
+  RepositorySlug slug,
+  int? prNumber,


Same here. I do not recall why these would ever be null.

Probably because the PullRequest class has the number property as nullable.

Yeah I was thinking that may be so. In that case, we should probably assert somewhere that we have gotten a PR number to work with, otherwise this all breaks.

Piinks

LGTM with the null safety nits.
This repo is frozen until 1/4/22, so we should be able to land this then.

mdebbar · 2022-01-04T16:23:59Z

@Piinks ping 🙂

Piinks

Can you add the assertions that the PR numbers aren't null from the last review?

mdebbar · 2022-01-04T20:05:15Z

Can you add the assertions that the PR numbers aren't null from the last review?

Ah, thanks for the reminder. Done.

Piinks

mdebbar · 2022-01-05T15:34:13Z

@Piinks I'm unable to merge this PR because only "authorized users" are allowed to. Is this something you can do, or do I have to add my name somewhere?

Piinks · 2022-01-05T15:45:22Z

I am not authorized either. @CaseyHillers can you merge this PR?

CaseyHillers · 2022-01-10T18:31:01Z

I am not authorized either. @CaseyHillers can you merge this PR?

As FYI, you'll need to use the waiting for tree to green label. That has the bot handle merges, which will be the defacto way across all repos in a week.

This reverts commit f7e5d0a.

[web] Add the flutter-gold check in web engine PRs

ad43efc

mdebbar requested review from CaseyHillers and Piinks December 7, 2021 17:59

Piinks reviewed Dec 7, 2021

View reviewed changes

CaseyHillers reviewed Dec 7, 2021

View reviewed changes

app_dart/lib/src/request_handlers/push_gold_status_to_github.dart Outdated Show resolved Hide resolved

mdebbar added 3 commits December 8, 2021 10:36

update messages

bcc50c8

use the same cron job for flutter and engine

9306a74

update graphql query

4bafcea

CaseyHillers reviewed Dec 8, 2021

View reviewed changes

mdebbar added 2 commits December 16, 2021 12:20

adjust tests to check for the engine repo

1dd43e7

dart format

3bc25db

mdebbar requested a review from Piinks December 16, 2021 19:36

mdebbar commented Dec 16, 2021

View reviewed changes

mdebbar added 2 commits December 16, 2021 16:11

cleanup

7072775

Merge branch 'main' into flutter_gold_web_engine

1dc497b

Piinks reviewed Dec 22, 2021

View reviewed changes

make pr number non-nullable

7d5a313

Piinks reviewed Dec 22, 2021

View reviewed changes

Piinks reviewed Jan 4, 2022

View reviewed changes

assert pr.number is not null

e82a244

mdebbar requested a review from Piinks January 4, 2022 20:05

mdebbar mentioned this pull request Jan 4, 2022

[web] Use Flutter Gold instead of flutter/goldens flutter/flutter#62706

Closed

6 tasks

Piinks approved these changes Jan 4, 2022

View reviewed changes

mdebbar added the waiting for tree to go green Merge PR when tree becomes green via fluttergithubbot label Jan 6, 2022

fluttergithubbot merged commit f7e5d0a into flutter:main Jan 6, 2022

mdebbar added a commit that referenced this pull request Jan 10, 2022

Revert "[web] Add the flutter-gold check in web engine PRs (#1505)"

bc916bf

This reverts commit f7e5d0a.

This was referenced Jan 10, 2022

Revert "[web] Add the flutter-gold check in web engine PRs" #1541

Merged

Reland "[web] Add the flutter-gold check in web engine PRs" #1546

Merged

	await for (PullRequest pr in gitHubClient.pullRequests.list(slug)) {
	// Get last known Gold status from datastore.
	final GithubGoldStatusUpdate lastUpdate = await datastore.queryLastGoldUpdate(slug, pr);

	if (baseRef != 'master' && baseRef != 'main') {
	if (baseRef != Config.defaultBranch(pr.base!.repo!.slug())) {

[web] Add the flutter-gold check in web engine PRs #1505

[web] Add the flutter-gold check in web engine PRs #1505

Uh oh!

Conversation

mdebbar commented Dec 7, 2021

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Piinks Dec 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mdebbar commented Dec 7, 2021

Uh oh!

mdebbar commented Dec 7, 2021

Uh oh!

Piinks commented Dec 7, 2021

Uh oh!

Piinks commented Dec 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdebbar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

mdebbar commented Jan 4, 2022

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

mdebbar commented Jan 4, 2022

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

mdebbar commented Jan 5, 2022

Uh oh!

Piinks commented Jan 5, 2022

Uh oh!

CaseyHillers commented Jan 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Piinks Dec 7, 2021 •

edited

Loading