-
Notifications
You must be signed in to change notification settings - Fork 7
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
Remove the use-flutter configuration #284
base: main
Are you sure you want to change the base?
Conversation
Always set up the flutter SDK and use `flutter pub publish`, even for non-flutter Dart packages. The `flutter` command can publish non-flutter packages so no behavior changes are expected during the publish action. Directly remove the argument, configuration, and all references immediately. There is no plan for a deprecation/migration phase since this only impacts CI, and the fix is trivial.
PR HealthBreaking changes ✔️Details
Changelog Entry ✔️Details
Changes to files need to be accounted for in their respective changelogs. Coverage ✔️Details
This check for test coverage is informational (issues shown here will not fail the PR). API leaks ✔️DetailsThe following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
License Headers ✔️Details
All source files should start with a license header. Package publish validation ✔️Details
Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation. |
- uses: dart-lang/setup-dart@0a8a0fc875eb934c15d08629302413c671d3f672 | ||
if: ${{ !inputs.use-flutter }} | ||
with: | ||
sdk: ${{ inputs.sdk }} | ||
|
||
- name: Install firehose | ||
run: dart pub global activate firehose |
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.
@devoncarew @mosuem Have we made breaking changes to firehose
CLI before? How do we handle cases where a repo could be using an older publish.yaml
file than what dart pub global activate firehose
installs?
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.
We recommend using @main
, so as long as we publish shortly after landing this on the main branch, I think it should work out for most use cases.
Is it OK if we break configuration that are pinning the publish workflow?
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 don't believe we've made breaking CLI changes before.
Repos generally always reference the .github/workflows/publish.yaml
script at head (i.e., uses: dart-lang/ecosystem/.github/workflows/publish.yaml@main
). That then globally activates firehose - that latest published version. We update the various publish.yaml clients to remove any use of use-flutter
, but I don't think we'll have any other work to do.
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 breaks are ok; we should have some good text for the changelog, and likely rev. to a new 'major' version to help call out the change.
The `dart` command and SDK do come with the `flutter` SDK, so it should be OK to use the flutter SDK for health checks as well.
Package publishing
Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation. |
This will allow backwards compatibility for the few files which are passing it, and can be removed after they are cleaned up.
@@ -129,9 +129,7 @@ class Health { | |||
}; | |||
|
|||
Future<HealthCheckResult> validateCheck() async { | |||
//TODO: Add Flutter support for PR health 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.
Most of the uses of use-flutter:
were for health checks - and the health check firehose action ignores that.
I think I broke the |
Oh, now I see what's happening. This PR make @kevmoo - we still don't have a general "add extra setup actions" feature in mono_repo do we? |
@natebosch – I don't think so. It's been soooo long |
@natebosch did you test this by flipping some repo using auto-publishing and PR health checks to |
Always set up the flutter SDK and use
flutter pub publish
, even fornon-flutter Dart packages. The
flutter
command can publish non-flutterpackages so no behavior changes are expected during the publish action.
Directly remove the argument, configuration, and all references
immediately. There is no plan for a deprecation/migration phase since
this only impacts CI, and the fix is trivial.