Skip to content

allow skipping creation of Github releases#130

Merged
Andarist merged 6 commits intochangesets:mainfrom
akphi:skip-create-release
Jan 23, 2022
Merged

allow skipping creation of Github releases#130
Andarist merged 6 commits intochangesets:mainfrom
akphi:skip-create-release

Conversation

@akphi
Copy link
Copy Markdown

@akphi akphi commented Dec 26, 2021

Reopen #108 due to change in base branch to main

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Dec 26, 2021

🦋 Changeset detected

Latest commit: 5f21e12

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@changesets/action Minor

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
Comment on lines +78 to +83
options: {
createGithubReleases:
createGithubReleases !== undefined
? Boolean(createGithubReleases)
: true,
},
Copy link
Copy Markdown
Member

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:

Suggested change
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?

Copy link
Copy Markdown
Author

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?

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Author

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
Comment on lines +26 to +31
if (
options.createGithubReleases !== undefined &&
!options.createGithubReleases
) {
return;
}
Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Updated!

@akphi akphi force-pushed the skip-create-release branch from e4c2c77 to 891d392 Compare December 28, 2021 22:14
src/index.ts Outdated
Comment on lines +78 to +80
createGithubReleases: JSON.parse(
getOptionalInput("createGithubReleases") ?? "true"
),
Copy link
Copy Markdown
Member

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:

Suggested change
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.

Copy link
Copy Markdown
Author

@akphi akphi Jan 12, 2022

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.

https://github.com/actions/toolkit/blob/daf8bb00606d37ee2431d9b1596b88513dcf9c59/packages/core/src/core.ts#L171-L182

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

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Author

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 😅
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?

image

Copy link
Copy Markdown
Member

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.

@Andarist Andarist merged commit 5c0997b into changesets:main Jan 23, 2022
@github-actions github-actions bot mentioned this pull request Jan 23, 2022
@Andarist
Copy link
Copy Markdown
Member

@akphi thanks for being patient and bearing with me!

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.

2 participants