-
Notifications
You must be signed in to change notification settings - Fork 9.8k
Make run-on-changed-packages flag handle repo-level changes #3946
Make run-on-changed-packages flag handle repo-level changes #3946
Conversation
Changes to some files (e.g., CI scripts) have the potential to cause failures in any packages, without changes to those packages themselves. This updates the --run-on-changed-packages to consider all packages as changed if any of those files are changed, to avoid issues where a change that changes both some repo-level files and some package-specific files only run presubmit tests on the packages that are directly changed, causing post-submit-only failures. Fixes flutter/flutter#82965
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, modulo one small nit
Co-authored-by: Maurits van Beusekom <[email protected]>
@@ -1,8 +1,10 @@ | |||
## NEXT | |||
## 0.1.2 |
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.
Do we need to version this now that it's not published separately in the plugin_tools?
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.
It's still published for flutter/packages.
|
||
// Returns true if one or more files changed that have the potential to affect | ||
// any plugin (e.g., CI script changes). | ||
Future<bool> _changesRequireFullTest() async { |
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.
Another thought would be to let Cirrus trigger tasks based on what files changed since it already has that functionality and we don't need to reinvent the wheel. So have a "Run all tests" and "Run changed packages" respective tasks that run inverse to each other
skip: !changesInclude('.cirrus.yml', '.ci.yaml', '.clang-format', ...)
skip: changesInclude('.cirrus.yml', '.ci.yaml', '.clang-format', ...)
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.
The primary issue there is that we have some tasks (Windows) that run on LUCI, and over time expect to have more move to LUCI as part of the overall Flutter infra changes, so anything we implement using Cirrus-specific logic would need to be replicated in LUCI, which is a maintenance headache.
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'm going to land this on red since while it won't fix the tree, it will at least make it easier for us to see what we're doing while fixing it. |
…3946) Changes to some files (e.g., CI scripts) have the potential to cause failures in any packages, without changes to those packages themselves. This updates the --run-on-changed-packages to consider all packages as changed if any of those files are changed, to avoid issues where a change that changes both some repo-level files and some package-specific files only run presubmit tests on the packages that are directly changed, causing post-submit-only failures. Fixes flutter/flutter#82965
Changes to some files (e.g., CI scripts) have the potential to cause
failures in any packages, without changes to those packages themselves.
This updates the --run-on-changed-packages to consider all packages as
changed if any of those files are changed, to avoid issues where a
change that changes both some repo-level files and some package-specific
files only run presubmit tests on the packages that are directly
changed, causing post-submit-only failures.
Fixes flutter/flutter#82965
Pre-launch Checklist
dart format
. See plugin_tool format)[shared_preferences]
///
).