Skip to content
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

Add title argument to CLI release workflow #1349

Closed
wants to merge 3 commits into from

Conversation

banool
Copy link
Contributor

@banool banool commented Jun 10, 2022

Description

Adds the ability to specify a name for the release.

Test Plan

what is testing?

quote: Christian

In seriousness, testing a manually triggered action from a branch isn't possible, I have to test from a clone of the repo. Stay posted.

Updated test plan

See https://github.com/banool/aptos-core/actions/runs/2477553972. This is a run on my clone of the repo using this updated workflow. I confirmed that the title of the workflow ran successfully and the release had the given title. I also did a run where I didn't specify the title and it correctly fell back to the default title: https://github.com/banool/aptos-core/actions/runs/2477607202.

@banool banool requested a review from geekflyer June 10, 2022 21:02
@aptos-bot aptos-bot added this to In Review in bors Jun 10, 2022
@banool banool force-pushed the add_title_argument_to_cli_release branch 2 times, most recently from 0a22ddb to 50f58c4 Compare June 10, 2022 22:07
@@ -64,5 +68,6 @@ jobs:
repo_token: "${{ secrets.GITHUB_TOKEN }}"
automatic_release_tag: "${{ github.event.inputs.release_tag }}"
prerelease: false
title: "${{ github.event.inputs.release_title }}"
Copy link
Contributor

@geekflyer geekflyer Jun 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens if the user leaves title empty? Will this create a release with an empty title or just proceed as if no title had been passed to the action? Just wanna make sure this action does something reasonable if the title hasn't been provided at all or otherwise make the title required in all cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested that, it falls back to the default, not an empty string.

@geekflyer geekflyer self-requested a review June 10, 2022 22:15
@banool
Copy link
Contributor Author

banool commented Jun 10, 2022

See #1354 for the permissions fix.

id-token: "write"
contents: "write"
packages: "write"
pull-requests: "read"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you sure it needs all of these?
I think this workflow actually only needs contents: write.

Copy link
Contributor Author

@banool banool Jun 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not, I just yanked this from an issue on the repo for action-automatic-releases that says contents: "write" was insufficient. I can experiment with restricting the permissions. I couldn't find a good resource explaining what each scope actually gives you. I'll do this next week some time, each loop of testing here takes like 30 minutes.

Copy link
Contributor

@geekflyer geekflyer Jun 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, just read the issue (marvinpinto/actions#340 (comment)) and looked through the source code of the action to understand a bit better what it does.
I'm 95% confident that for us it only needs:

contents: write
pull-requests: read

The latter is because it queries PRs to get a changelog.
The id-token is definitely irrelevant here (as the issue from the other repo suggested) and packages seems also very unlikely. I guess @marvinpinto was publishing some package (npm, docker etc.) as part of the workflow and that's why he's added it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet yeah I would've thought the same. I'll test that change.

@davidiw
Copy link
Contributor

davidiw commented Jun 15, 2022

/land

@aptos-bot aptos-bot moved this from In Review to Queued in bors Jun 15, 2022
@aptos-bot aptos-bot moved this from Queued to Testing in bors Jun 15, 2022
@github-actions
Copy link
Contributor

Forge run: https://github.com/aptos-labs/aptos-core/actions/runs/2500106240
Forge test result: all up : 2213 TPS, 1960 ms latency, 2750 ms p99 latency,no expired txns

@aptos-bot aptos-bot force-pushed the add_title_argument_to_cli_release branch from f9964db to 6de4949 Compare June 15, 2022 06:14
@aptos-bot aptos-bot removed this from Testing in bors Jun 15, 2022
@aptos-bot aptos-bot closed this in 6de4949 Jun 15, 2022
@geekflyer geekflyer deleted the add_title_argument_to_cli_release branch June 15, 2022 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants