allow skipping creation of Github releases#130
Conversation
🦋 Changeset detectedLatest commit: 5f21e12 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
src/index.ts
Outdated
| options: { | ||
| createGithubReleases: | ||
| createGithubReleases !== undefined | ||
| ? Boolean(createGithubReleases) | ||
| : true, | ||
| }, |
There was a problem hiding this comment.
I would refactor this to:
| options: { | |
| createGithubReleases: | |
| createGithubReleases !== undefined | |
| ? Boolean(createGithubReleases) | |
| : true, | |
| }, | |
| createGithubReleases: JSON.parse(getOptionalInput('createGithubReleases') ?? 'true'), |
Any particular reason why you have implemented this as an env variable (process.env.CREATE_GITHUB_RELEASE) and not as an input of the action?
There was a problem hiding this comment.
Not really, I just didn't learn about this way 😓
Fixed, but I start to wonder where we can document all these action inputs?
There was a problem hiding this comment.
It should be added and documented~ in this file: https://github.com/changesets/action/blob/f4afbea27e0f68b63e3e018d053582abee1817b6/action.yml
The additional documentation about it in the README would also be cool but we can do that later.
There was a problem hiding this comment.
Added document in the action.yml file
src/run.ts
Outdated
| if ( | ||
| options.createGithubReleases !== undefined && | ||
| !options.createGithubReleases | ||
| ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
I still kinda think that this should be moved outside of the createRelease function - if I call such a function I expect it to actually do what it is named after. Kinda a single-responsibility approach.
e4c2c77 to
891d392
Compare
src/index.ts
Outdated
| createGithubReleases: JSON.parse( | ||
| getOptionalInput("createGithubReleases") ?? "true" | ||
| ), |
There was a problem hiding this comment.
I've just learned about this helper function that can probably be used here:
| createGithubReleases: JSON.parse( | |
| getOptionalInput("createGithubReleases") ?? "true" | |
| ), | |
| createGithubReleases: core.getBooleanInput("createGithubReleases"), |
I believe that it takes the default declared in the .yml file into consideration but it would be great if we could confirm that.
There was a problem hiding this comment.
I don't think this is safe to do. That function (right from its type-def returns a boolean), but this is optional.
In case people don't provide this (which is pretty much for all current users), it would throw error.
What we need to do is probably:
getOptionalInput("createGithubReleases") ? core.getBooleanInput("createGithubReleases") : trueRegardless, I have updated to use this new syntax
There was a problem hiding this comment.
I think that if it's not safe to do then we've made a horrible mistake merging this: https://github.com/changesets/action/pull/131/files#diff-a2a171449d862fe29692ce031981047d7ab755ae7f84c707aef80701b3ea0c80R17 . While merging I didn't notice that it's using this getBooleanInput and I didn't check it back then. If this would throw then we'd already have some angry users in the issues about broken workflow runs 😅
I think that the defaults might be provided by the workflow runner using environment variables:
https://github.com/actions/toolkit/blob/daf8bb00606d37ee2431d9b1596b88513dcf9c59/packages/core/src/core.ts#L129-L130
I'll try to recheck this later in my private playground repo.
There was a problem hiding this comment.
Could also be because people are using changesets/action@v1 like in the README.md file
I've rechecked in my private github actions playground and it seems like a call to core.getBooleanInput("createGithubReleases") is enough :)
A bit unrelated, do you know why this action doesn't show up in Github action marketplace, i.e. why don't we have this in our repo page?
Not sure, I'm not sure what's the requirement for this.
# Conflicts: # action.yml
|
@akphi thanks for being patient and bearing with me! |

Reopen #108 due to change in base branch to
main