-
Notifications
You must be signed in to change notification settings - Fork 260
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Updated!
e4c2c77
to
891d392
Compare
src/index.ts
Outdated
createGithubReleases: JSON.parse( | ||
getOptionalInput("createGithubReleases") ?? "true" | ||
), |
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'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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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") : true
Regardless, I have updated to use this new syntax
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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