-
Notifications
You must be signed in to change notification settings - Fork 3.3k
[ci/tool] Add external dependency validation #3466
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
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.
optional nit, but seems good to me.
} | ||
|
||
// Checks whether a given dependency is allowed. | ||
bool _allowDependency(String name, Dependency dependency) { |
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 method name seems a little off from what I would expect. Something like _isDependancyAllowed
or something similar.
As is I would expect this to add the dep to an allow list. Up to you if you agree.
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.
Good call, changed to _shouldAllowDependency
Overrides: packages changes are to |
Can we turn on dependabot for this case? |
It's on my list to look into in general; currently we're not using it for Dart at all so I don't know how configurable it is. Ideally we'd use it for major versions of most dependencies, and now for other versions of these pinned items. (For cases like |
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
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! I have 2 deps to remove. (jwt_decoder and mocktail)
@@ -21,4 +21,4 @@ dev_dependencies: | |||
sdk: flutter | |||
integration_test: | |||
sdk: flutter | |||
mocktail: ^0.3.0 | |||
mocktail: 0.3.0 |
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.
Co-authored-by: Jenn Magder <[email protected]>
Co-authored-by: Jenn Magder <[email protected]>
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 makes me happier than I can express in a single lgtm.
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
auto label is removed for flutter/packages, pr: 3466, Failed to merge pr#: 3466 with Pull request could not be merged: Pull Request is not mergeable. |
* 7636eb7 [go_router_builder] Support default value for `Set`, `List` and `Iterable` route parameters (flutter/packages#3391) * 26c95da [image_picker] Move HashSet construction within if-statement (flutter/packages#3448) * f5687b2 [image_picker] fix typos in comments (flutter/packages#3413) * 84afba7 [image_picker] Migrate Android to Pigeon (flutter/packages#3476) * 42927fc [image_picker]: Bump androidx.exifinterface:exifinterface from 1.3.3 to 1.3.6 in /packages/image_picker/image_picker_android/android (flutter/packages#3238) * 9a44bdf Require Dart SDK 2.14, because of using APIs. (flutter/packages#3468) * 12609a2 [ci] Clean up iOS simulators (flutter/packages#3458) * 9b136a9 [ci/tool] Add external dependency validation (flutter/packages#3466) * 11aab47 Manual roll Flutter from fb7e828 to 67e5f66 (8 revisions) (flutter/packages#3482) * 784291b Update goldens (flutter/packages#3442) * 43a42d1 [webview_flutter_android] Updates Dart and Java InstanceManagers (flutter/packages#3282) * d0de136 [camera] Reland android flip/change camera while recording (flutter/packages#3460) * 74fd094 [image_picker_android] Adjust file extension in FileUtils when it does not match its mime type (flutter/packages#3409) * d2f1d2d [flutter_adaptive_scaffold] : 🐛 [FIX] : Issue: 121135. (flutter/packages#3253) * 3d078b5 Roll Flutter from 67e5f66 to 53dfd23 (39 revisions) (flutter/packages#3484) * 3b3a09d [url_launcher] Convert iOS to Pigeon (flutter/packages#3481) * 80cd50a Roll Flutter from 53dfd23 to 6bd2b3c (17 revisions) (flutter/packages#3486) * 998bb29 [webview_flutter] Updates the README with the migration of `WebView.initialCookies` and Hybrid Composition on (flutter/packages#3470) * bbf86a7 Roll Flutter from 6bd2b3c to 3e4e092 (7 revisions) (flutter/packages#3490)
[ci/tool] Add external dependency validation
We've had a general policy of avoiding external dependencies for at least the last year, but it hadn't really been communicated or enforced. It's now documented in the wiki, and this adds tooling and CI that validates that every package we depend on here is either a) local to this repository, b) from the SDK, or c) in an explicit allow list config file.
This follows the general trend in the repository away from "hope people know/remember/check for all of the policies" to "validate in CI and require explicit opt-out for exceptions". Once this lands, I'll update the wiki to point to the relevant config files where exceptions should be added when we choose to make exceptions.
In most cases, this just allows everything we're already using. For some example and dev-only dependencies, this moves to pinning since that's a simple change to make.
See flutter/flutter#122713
Pre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style.///
).