Enable filtering builders based on changed files in PRs#917
Enable filtering builders based on changed files in PRs#917keyonghan merged 3 commits intoflutter:masterfrom
Conversation
There was a problem hiding this comment.
Update? given that functions in this library are made for reusability let's add complete docs. E.g parameters received, return value and an example.
There was a problem hiding this comment.
How do we support * and **?
There was a problem hiding this comment.
Existing cirrus seems to use ** only, do we have the use case of a single *?
There was a problem hiding this comment.
should we call it glob instead of dirs? dirs is confusing as it can be a file a folder or a glob, glob matches all of them.
There was a problem hiding this comment.
How do we differentiate from a full path and wildcards, e.g:
/a/b file in a
and
/a/bc/a ?
Should we implement the functionality using regexes? e.g * replaced with '[a-zA-Z_]*' and ** as[a-zA-Z_/]?
There was a problem hiding this comment.
Isn't this list of files already available in the PR event?
There was a problem hiding this comment.
Doesn't seem so. Based on CheckSuiteEvent, we can track down to CheckSuite, then PullRequest, where changedFilesCount is available, but there is no changedFiles though.
There was a problem hiding this comment.
we'll need tests for the wildcards * and **
There was a problem hiding this comment.
It would be really helpful to fix the unreadable, non-type-safe Map<String, dynamic> everywhere with a @JsonSerializable annotated class.
There was a problem hiding this comment.
+1 a list of map of maps is prone to errors.
There was a problem hiding this comment.
Agree. Updating to use annotated class will affect quite some files. Created TODO (flutter/flutter#64286) to update that in a separate PR.
0cd591b to
1a4e7f4
Compare
There was a problem hiding this comment.
+1 a list of map of maps is prone to errors.
| /// "repo":"flutter", | ||
| /// "taskName":"zzz", | ||
| /// "enabled":true, | ||
| /// "run_if":["a/b/", "c/d/**"] |
There was a problem hiding this comment.
What about single files?
There was a problem hiding this comment.
The case of single files has been covered. Please refer to the test below. Updated the doc to include a case of a single file and a case of single *
There was a problem hiding this comment.
should we call it glob instead of dirs? dirs is confusing as it can be a file a folder or a glob, glob matches all of them.
| }); | ||
|
|
||
| test('returns builders when run_if matches files with both * and **', () async { | ||
| builders = |
There was a problem hiding this comment.
We need one more test for file and folder with the same name "a" and a/b/c. E.g a run_if = ["a"] does not match a files changes "a/b/c"
godofredoc
left a comment
There was a problem hiding this comment.
LGTM, let's land and monitor to ensure builders are triggered correctly.
Will keep an eye on this tomorrow morning. |
This is to solve flutter/flutter#62841