Gold Pre-submit flow for contributors without permissions#47551
Merged
fluttergithubbot merged 20 commits intoflutter:masterfrom Jan 9, 2020
Merged
Gold Pre-submit flow for contributors without permissions#47551fluttergithubbot merged 20 commits intoflutter:masterfrom
fluttergithubbot merged 20 commits intoflutter:masterfrom
Conversation
Piinks
commented
Dec 30, 2019
| } | ||
|
|
||
| /// Creates an animation controller with no upper or lower bound for its value. | ||
| /// Creates an animation controller with no upper or lower bound for its |
Contributor
Author
There was a problem hiding this comment.
Trivial framework change to trigger the appropriate test shards.
Piinks
commented
Dec 30, 2019
Piinks
commented
Jan 2, 2020
Contributor
Author
|
This is ready for review. @dnfield, this is the full solution for users without the proper permissions on Cirrus, and replaces the workaround I put in place before the holiday. |
dnfield
reviewed
Jan 8, 2020
| return FlutterPreSubmitFileComparator(baseDirectory.uri, goldens); | ||
|
|
||
| final bool hasWritePermission = platform.environment['CIRRUS_USER_PERMISSION'] == 'admin' | ||
| || platform.environment['CIRRUS_USER_PERMISSION'] == 'write'; |
Contributor
There was a problem hiding this comment.
Is this how we check elsewhere?
I think it would be better if we check the actual env variable that has the service account key and see if it starts with ENCRYPTED[ or something like that. That's what we really care about, right?
bkonyi
pushed a commit
that referenced
this pull request
Jan 9, 2020
This was referenced Feb 4, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
There is currently a workaround in place to un-block first time/un-privileged contributors that cannot execute pre-submit tryjobs, but it does not allow these contributors to land new or updated golden file changes.
This is a more complete solution that removes that work around and provides a workflow for these contributors.
For permission-prohibited contributors, the presubmit comparator will instead use the
goldctl imgtest checkfeature to lookup image hashes to confirm results. If a check fails, it will retrieve and use the raw expectations to confirm if it is a new test, allowing it to pass through, or if it is an expected change via Gold's ignore feature.This very similar to the pre-tryjob implementation, without requesting images and manually comparing, and without the need to access Gold's details API.
Better error messages have been included in this change to provide more informative and actionable messaging in all of these use cases.
Related Issues
Fixes #46687
Fixes #47898
Tests
As an authorized contributor in this case, I reversed the logic on an earlier commit (and since reverted) to ensure that the presubmit checks completed correctly for the unauthorized use case.
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Did any tests fail when you ran them? Please read Handling breaking changes.