-
Notifications
You must be signed in to change notification settings - Fork 100
Skip engine builds/tests for a framework-only PR (allow-listed) #4185
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Adding @yjbanov as a reviewer as well just for a second set of eyes (seeing that I helped with this pr) |
jtmcdole
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm; just some nits
| if (pullRequest.user!.login == config.autosubmitBot && | ||
| pullRequest.labels!.any((element) => element.name == Config.revertOfLabel)) { | ||
| log.info('$logCrumb: skipping generating the full set of checks for revert request.'); | ||
| log.info('$logCrumb: skipping generating the full set of checstagest.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this line got vscode-share-ifiied
|
I am going to submit, @yjbanov we'll be making more edits to this in the coming days so feedback is still welcome post-submit. |
| if (pullRequest.user!.login == config.autosubmitBot && | ||
| pullRequest.labels!.any((element) => element.name == Config.revertOfLabel)) { | ||
| log.info('$logCrumb: skipping generating the full set of checks for revert request.'); | ||
| log.info('$logCrumb: skipping generating the full set of changes.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not skipping "changes" though. It's skipping checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a mistake on the part of John and I, we paired and vscode made some weird edits.
I'll send a PR fixing this.
Towards flutter/flutter#162201.
If the following is true:
['jtmcdole', 'matanlurey', 'yjbanov'].contains(pullRequest.user!.login)DEPSnorengine/**sources... then we skip the entire engine build phase and the engine tests component of the tests phase.
If we like the outcome of this PR (after pushed, some manual testing), we can either open up the allow-list entirely or add additional names, and finally drop the allow-list entirely. Future work (i.e. expanding the file limit) need to be gated behind refactors and better test coverage, at least IMO.
Note this PR also regenerates
dart run build_runner buildsome generated files./cc @yjbanov