-
Notifications
You must be signed in to change notification settings - Fork 14
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
Auto version bump #202
Auto version bump #202
Conversation
Even if this receives reviews, please wait for my review before merging due to the potentially disruptive impact of this change |
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.
Typo in the function name, otherwise looks good.
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.
See the main inline comment for the reason why I'm requesting changes
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 few more changes then I'll review the actual files containing the new logic
@davideroffo can you please review this? |
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.
LGTM, I left a few minor comments.
@sameer-coder I noticed that the |
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 few comments
README.md
Outdated
@@ -195,7 +200,7 @@ Please note that in case of a prerelease the `sync-semver-tags` input will be tr | |||
| Input | Required | Description | | |||
| --- | --- | --- | | |||
| `github-token` | No | This is your GitHub token, it's [already available](https://docs.github.com/en/actions/security-guides/automatic-token-authentication#about-the-github_token-secret) to your GitHub action | | |||
| `semver` | Yes | The version you want to bump (`patch|minor|major`). | | |||
| `semver` | Yes | The version you want to bump (`auto|patch|minor|major`). | |
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 isn't actually up to date. when we introduced support for prereleases we included more options. maybe, instead of listing them, we should just link the npm docs page
src/index.js
Outdated
let latestTag = null | ||
if (!baseTag) { | ||
const allTags = await run('git', ['tag', '--sort=-creatordate']) | ||
const tags = allTags.split('\n') | ||
latestTag = tags[0] || null | ||
} |
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.
all this explicit null handling isn't very intuitive, and probably unnecessary (including the default value of this function's arg)
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.
also prefer avoiding mutable state please, there's most certainly a more elegant way to write this that doesn't require let variables
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.
LGTM
This PR introduces the auto version bump feature.
The user can now choose the "auto" option for semver version updates. The action will attempt to determine the new version number based on the commit messages. For this option to work, the repository must use the conventional commits standard.
Closes #193