Adding gold status check to framework PRs #651
Conversation
app_dart/lib/src/request_handlers/push_gold_status_to_github.dart
Outdated
Show resolved
Hide resolved
|
fyi @dnfield and @kjlubick to address flutter/flutter#48744 |
0daab04 to
cf45ccf
Compare
keyonghan
left a comment
There was a problem hiding this comment.
Do you think it is easy to simplify the main function body by replacing with some more sub-functions? It might be easier to follow.
| const List<String> cirrusInProgressStates = <String>[ | ||
| 'EXECUTING', | ||
| 'CREATED', | ||
| 'TRIGGERED', | ||
| 'NEEDS_APPROVAL' | ||
| ]; |
There was a problem hiding this comment.
Please refer to the latest status change: https://github.com/flutter/cocoon/blob/master/app_dart/lib/src/request_handlers/refresh_cirrus_status.dart#L26
There was a problem hiding this comment.
Ah! Thank you. I've made those a constant now, if we are going to use them in multiple places I am sure we don't want to have to track down all of them for updates. :)
app_dart/lib/src/request_handlers/push_gold_status_to_github.dart
Outdated
Show resolved
Hide resolved
app_dart/lib/src/request_handlers/push_gold_status_to_github.dart
Outdated
Show resolved
Hide resolved
| 'Found Cirrus build status for pull request ${pr.number}, commit ' | ||
| '${pr.head.sha}: $taskName ($status)'); | ||
|
|
||
| if (taskName.contains('framework')) { |
There was a problem hiding this comment.
nit: this is liable to get broken in some way down the road if, for example, web is using goldens and doesn't use framework in the task name.
Not sure how to fix that - maybe adopting a convention that tasks using gold have gold in the task name?
There was a problem hiding this comment.
I've removed this. I thought some folks might be bothered by an irrelevant check on their PR since we try to keep the checks lean, but if they aren't running a tryjob then it will just go green on the first run.
| log.debug('Last known Gold status for ${pr.number} was with sha ' | ||
| '${lastUpdate?.head ?? 'initial'}, status: ${lastUpdate?.status ?? 'initial'}'); | ||
|
|
||
| if (lastUpdate.head == null || lastUpdate.head != pr.head.sha) { |
There was a problem hiding this comment.
nit: This whole if/else block would read more clearly if it was broken up into a few functions.
There was a problem hiding this comment.
I have refactored, I think it is much simpler now. :)
This change will add a Gold status check to all framework PRs that reflects the state of the current tryjobs run through Flutter Gold for the pr and commit.
This was originally attempted in flutter/flutter#49370 as a Cirrus check. That implementation was subject to timeouts from Cirrus, so it will be implemented here instead.
Adding this check will do a couple of things for improved workflow and productivity.