Support branching for push build status to github #702
Conversation
| final List<Branch> branches = await getBranches( | ||
| config, branchHttpClientProvider, log, gitHubBackoffCalculator); | ||
| for (Branch branch in branches) { | ||
| final BuildStatus buildStatus = await buildStatusProvider |
There was a problem hiding this comment.
This can be an expensive operation. I presume that we will only add release branches, and never remove any.
We should look into a way of only updating branches that had changes. I imagine once we have 100+ branches we do not want to be making this many Datastore reads on a cron job
There was a problem hiding this comment.
One marginal improvement I have is maybe we can update the build status provider to use the cache?
There was a problem hiding this comment.
This is a good point. I added this as a todo to improve the performance. Now prefer to have everything ready to run an e2e test first.
| for (Branch branch in branches) { | ||
| final BuildStatus buildStatus = await buildStatusProvider | ||
| .calculateCumulativeStatus(branch: branch.name); | ||
| final GitHub github = await config.createGitHubClient(); |
There was a problem hiding this comment.
Should this GitHub logic be moved into GitHubService? Seems repetitive to have GitHubService and a GitHub client in the same request handler.
There was a problem hiding this comment.
GitHubService is used to provide github queries that are not supported by GitHub client itself.
There was a problem hiding this comment.
Couldn't this get the github client from the service instead then?
There was a problem hiding this comment.
Hit some error in test by using github client instead before. Now managed to solve any hit issues.
| 'Updating status of ${slug.fullName}#${pr.number} from ${update.status}'); | ||
| final CreateStatus request = CreateStatus(buildStatus.githubStatus); | ||
| request.targetUrl = | ||
| 'https://flutter-dashboard.appspot.com/build.html'; |
There was a problem hiding this comment.
nit: This gets redirected to https://flutter-dashboard.appspot.com/#/build
| final CreateStatus request = CreateStatus(buildStatus.githubStatus); | ||
| request.targetUrl = | ||
| 'https://flutter-dashboard.appspot.com/build.html'; | ||
| request.context = 'flutter-build'; |
There was a problem hiding this comment.
nit: Can we move this and the description to the config?
| final int maxEntityGroups = config.maxEntityGroups; | ||
| for (int i = 0; i < updates.length; i += maxEntityGroups) { | ||
| await datastore.db | ||
| .withTransaction<void>((Transaction transaction) async { | ||
| transaction.queueMutations( | ||
| inserts: updates.skip(i).take(maxEntityGroups).toList()); | ||
| await transaction.commit(); | ||
| }); | ||
| } | ||
| log.debug('Committed all updates'); |
There was a problem hiding this comment.
I'm confused at what this is doing.
There was a problem hiding this comment.
This may help you understand: https://github.com/flutter/cocoon/blob/master/app_dart/lib/src/datastore/cocoon_config.dart#L61
There was a problem hiding this comment.
With such a nitty detail to datastore, it would be cleaner to move this code to the Datastore service and add documentation to why it's needed. Copying that explanation is a good start, but i'm still confused as to why pushing build status to github can handle Datastore writes
There was a problem hiding this comment.
We have a status entity in datastore to denote how many updates we have made so for for existing PRs. So whenever we push status to github, we write corresponding update index.
There was a problem hiding this comment.
Sounds like it would be good to document it in the code then :)
There was a problem hiding this comment.
Added a brief documentation. Thank you!
| expect(log.records.where(hasLevel(LogLevel.ERROR)), isEmpty); | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
There should be a test where there are multiple branches (e.g. master and dev) to make sure both get updates pushed to them
CaseyHillers
left a comment
There was a problem hiding this comment.
LGTM. Just some documentation left to be addressed
| final int maxEntityGroups = config.maxEntityGroups; | ||
| for (int i = 0; i < updates.length; i += maxEntityGroups) { | ||
| await datastore.db | ||
| .withTransaction<void>((Transaction transaction) async { | ||
| transaction.queueMutations( | ||
| inserts: updates.skip(i).take(maxEntityGroups).toList()); | ||
| await transaction.commit(); | ||
| }); | ||
| } | ||
| log.debug('Committed all updates'); |
There was a problem hiding this comment.
Sounds like it would be good to document it in the code then :)
|
|
||
| String get cqLabelName => 'CQ+1'; | ||
|
|
||
| String get flutterBuild => 'flutter-build'; |
There was a problem hiding this comment.
nit: Document what these are used for
This is part of flutter/flutter#51807
We will support commits following different branches. Existing logic to push build status to github doesn't recognize branch difference. It will calculate build status based on latest commits of
master(default), and push the status to all PRs in github.The change here fixes above issue. It will update PRs of a specific branch based on the build status calculated according to that branch's commits.