-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[ci] Adds github action for creating batch release #10298
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
base: main
Are you sure you want to change the base?
Conversation
|
reason for override: ci only change |
| create_release_pr: | ||
| runs-on: ubuntu-latest | ||
| env: | ||
| BRANCH_NAME: ${{ github.event.client_payload.package }}-${{ github.run_id }}-${{ github.run_attempt }} |
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.
So the cron job is constantly creating new branches? What cleans up branches that don't actually end up published?
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 see that can be a problem. then I think the right thing to do will be re-use the same branch name and do a clean up before creating a new release
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.
filed flutter/flutter#177748
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.
If these files are going to be created as part of the PR process, then the tooling should be updated to understand that, and the override labels shouldn't be needed on this PR. If the PR isn't landable without overrides, then the normal commit path isn't working correctly.
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.
yes, that part of the work will be in flutter/flutter#176433.
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.
Usually if a change to repo structure requires updates to our checks we do those two things in the same PR rather than override our own tooling, so it's not clear to me why this is being done separately. That said, since it's isolated to this package and shouldn't affect any normal PRs for now I guess it's fine.
|
|
||
| print(' Pushing to remote...'); | ||
| final io.ProcessResult pushResult = | ||
| await repository.runCommand(<String>['push', 'origin', branchName, '--force']); |
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.
Here too, --force seems needlessly dangerous. I don't see why we would need to enable this command to be destructive.
| throw FormatException('Expected "changelog" to be a list, but found ${changelogYaml.runtimeType}.'); | ||
| } | ||
| final List<String> changelog = changelogYaml.nodes | ||
| .map((YamlNode node) => node.value as String) |
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.
What happened to allowing arbitrary markdown? How would someone make this changelog for instance?
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 can use | for arbitrary markdown in yaml will need to check whether dart yaml support this properly. now I think of it maybe the changelog part of the yaml shouldn't be a list, it should just be a single element that accept arbitrary markdown.
version: major
changelogs: |
my *markdown*
- item **one**
- item two
7be1e87 to
5853905
Compare
| @@ -0,0 +1,33 @@ | |||
| name: "Creates Batch Release for go_router package" | |||
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.
Isn't the script generic?
| token: ${{ secrets.GITHUB_TOKEN }} | ||
| commit-message: "Batch release PR for ${{ github.event.client_payload.package }} package" | ||
| title: "Batch Release PR for ${{ github.event.client_payload.package }} package" | ||
| body: "This PR was created automatically to batch release the ${{ github.event.client_payload.package }} package." |
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.
Nit for all three of these: remove " package" from the end (and "the" from this last one), as it's clear in the context of flutter/packages that PRs are about packages.
| token: ${{ secrets.GITHUB_TOKEN }} | ||
| commit-message: "Batch release PR for ${{ github.event.client_payload.package }} package" | ||
| title: "Batch Release PR for ${{ github.event.client_payload.package }} package" | ||
| body: "This PR was created automatically to batch release the ${{ github.event.client_payload.package }} package." |
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.
Let's put `` around the package name in the body, as we frequently do that to make the package name especially clear.
| uses: peter-evans/create-pull-request@v7 | ||
| with: | ||
| token: ${{ secrets.GITHUB_TOKEN }} | ||
| commit-message: "Batch release PR for ${{ github.event.client_payload.package }} package" |
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.
A commit isn't a PR, so the commit message shouldn't say "PR".
| with: | ||
| token: ${{ secrets.GITHUB_TOKEN }} | ||
| commit-message: "Batch release PR for ${{ github.event.client_payload.package }} package" | ||
| title: "Batch Release PR for ${{ github.event.client_payload.package }} package" |
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 should follow repo convention: [${{ github.event.client_payload.package }}] Batch release.
(Ideally it would include the version, but getting that information across steps is probably more trouble than it's worth.)
| package.directory.deleteSync(recursive: true); | ||
| }); | ||
|
|
||
| Future<void> testReleaseBranch({ |
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 find it almost impossible to understand what the tests are doing with a lot of scrolling back and forth because of this function. I would much, much rather this file be 50% longer (is it even that much?) if it means I can just read a test and trivially see what it is doing.
| final List<String> changelogs; | ||
| } | ||
|
|
||
| /// The type of version change for a release. |
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 should be very clearly documented that the order of these values is critical.
| ], | ||
| ); | ||
| }); | ||
|
|
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 would expect explicit tests that given various combinations of different version update types, the resulting version is correct. It doesn't have to be every combination, but at least a sampling (and they should be explicit tests focusing on just that, without a lot of extraneous validation of the changelog output merging, so that it's obvious what the test is about).
Right now, it looks like someone could re-order VersionChange in various ways that would cause severely incorrect behavior, without any tests failing.
| }); | ||
| }); | ||
|
|
||
| test('throw when github fails', () 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.
I'm confused by this description; git rm doesn't interact with GitHub.
| } | ||
|
|
||
| expect(error, isA<ToolExit>()); | ||
| }); |
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.
There are four different git commands with separate error codepaths; all of them should be tested.
5853905 to
48eb6ab
Compare
fixes flutter/flutter#176425
To set up a batch release cron job for a package, one needs to make a copy of packages/.github/workflows/go_router_batch.yml, and change the name and cron schedule.
The cron job does the following:
batch_release_pr:
Pre-Review Checklist
[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3