Support branching for cirrus status #688
Conversation
There was a problem hiding this comment.
So this will only apply the flutter-gold status to pre-submit checks that will be landing on master?
When Gold is able to support branching in the future (still TBD), I think we will want the flutter-gold check to work on these branched pre-submit checks. Right now individual pre-submit tests with golden files will not fail since they rely on this flutter-gold check.
Can you add a TODO here to explain that?
There was a problem hiding this comment.
Yes. It will check only master branch based on the above logic.
I see your comment: flutter/flutter#51807 (comment)
Do you mind create corresponding issue, and then I will add a TODO pointing to that issue?
There was a problem hiding this comment.
Should we use a branch passed as a parameter instead of fixing it to master?
There was a problem hiding this comment.
Same question as above.
There was a problem hiding this comment.
I wonder if this logic can be abstracted to a utility function that can be used everywhere(I've seen this logic replicated in several places).
Ideally something like:
List branches = await getSupportedBranches();
for (Branch branch in branches) { ...
There was a problem hiding this comment.
Rewrite this part as suggested. Now multiple APIs call this shared logic defined in utils.dart. Additionally merged the /request_handler/utils.dart with /foundation/utils.dart.
There was a problem hiding this comment.
Should we add a constant for this limit? it can make it easier to change in the future.
There was a problem hiding this comment.
Seems like we have too much logic in a single method, can we split the functionality into multiple methods? This will also simplify tests for specific functionality.
There was a problem hiding this comment.
Added two sub functions: _getStatuses and _getNewTaskStatus to simply.
There was a problem hiding this comment.
Maybe add the retries wrapper? everything related with datastore has a high failure rate.
There was a problem hiding this comment.
Added. I filed an issue to track retry implementation for all cocoon backend days ago: flutter/flutter#52427.
There was a problem hiding this comment.
Can we also tests that used a branch different than master? My assumption is that master is hardcoded everywhere and we may miss some places that need changes if we use master.
There was a problem hiding this comment.
The reason we fix master here is that check for waiting pull requests applied to master only. As suggested, I have added one more test to consider the case with a different branch name, for which we will ignore cirrus statues.
There was a problem hiding this comment.
What about adding a test for a matching branch different than master?
b728edb to
4626cbe
Compare
|
|
||
| final HttpClient client = branchHttpClientProvider(); | ||
| try { | ||
| for (int attempt = 0; attempt < 3; attempt++) { |
There was a problem hiding this comment.
We can simplify this function removing the for loop and the try..catch by using "retry" with retryIf.
There was a problem hiding this comment.
This is a TODO as mentioned in the above issue: flutter/flutter#52427. Since this PR is big already, do you think it would be good to finish this in a separate PR later? I am a little bit concerned about that too many changes in a single shot may make it easy to break and difficult to triage/debug if any issues.
There was a problem hiding this comment.
Can we add the TODO here to ensure we don't forget about it?
There was a problem hiding this comment.
Added, and pointed to the above issue.
| log.warning( | ||
| 'Attempt to download branch_regexps.txt failed (HTTP $status)'); | ||
| } | ||
| } catch (error, stackTrace) { |
There was a problem hiding this comment.
Is is possible to use specific errors rather than catching all?
| client.close(force: true); | ||
| } | ||
| log.error('GitHub not responding; giving up'); | ||
| return <String>['master']; |
There was a problem hiding this comment.
This can potentially hide errors, I'd suggest throwing an error rather than logging and then defaulting to master.
| return Body.empty; | ||
| } | ||
|
|
||
| String _getNewTaskStatus(List<String> statuses, FullTask task) { |
There was a problem hiding this comment.
Some docs will be great.
| return newTaskStatus; | ||
| } | ||
|
|
||
| List<String> _getStatuses( |
There was a problem hiding this comment.
Some docs will be great.
godofredoc
left a comment
There was a problem hiding this comment.
LGTM after adding TODO to simplify loadBranchRegExps
Thank you for all the detailed comments! |
This is part of flutter/flutter#51807
Instead of assuming only one branch exists for each commit in cirrus, we here consider all branches. When updating cirrus status in cocoon, we consider the branch property as well.