-
-
Notifications
You must be signed in to change notification settings - Fork 36
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
build: add automated releases and pre-commit hooks #289
Conversation
semantic-release will publish the correct version, so having the correct version number in the package.json is unnecessary.
This is required by semantic-release.
These plugins are used in .releaserc.yml.
semantic-release will just new versions on top.
🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎ To accept the risk, merge this PR and you will not be notified again. Next stepsWhat is network access?This module accesses the network. Packages should remove all network access that isn't functionally unnecessary. Consumers should audit network access to ensure legitimate use. Take a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with
|
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.
Thanks for your contribution, we appreciate it!
I left some comments to open the discussion. 😄
package.json
Outdated
@@ -1,7 +1,7 @@ | |||
{ | |||
"name": "ts-standard", | |||
"description": "TypeScript Standard Style based on StandardJS", | |||
"version": "12.0.2", | |||
"version": "12.0.3", |
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.
Now that we automate it, I think we should not increase the version ourselves.
I think we should use 0.0.0-development
instead.
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.
semantic-release
will bump the version for you, and @semantic-release/git
will commit and push the version bump to the git repo (This is why the accidental version bump seen here happened, as I was testing semantic-release). The version won't have to be increased manually. 😄
You can see this in action in a test repo I made; here's an example commit of what it will do:
https://github.com/Maytha8/ts-standard-test/commit/b00d5b3fefff87dac6bf0c4f0dccf3c11d116156#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519
Here you can see that it knew what the next version was and bumped it.
The only reason semantic-release
have their own version set to 0.0.0-development
is because they don't use @semantic-release/npm
on their own project.
If you'd like this behavior of leaving the version at 0.0.0-development
, then we could remove package.json from the assets
property of @semantic-release/git
in .releaserc.yml, thus not committing the package.json version bump.
- assets: | ||
- CHANGELOG.md | ||
- package.json |
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.
That might be best to not have any assets.
The changelog will be published with the GitHub Release, what do you think? 😄
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.
CHANGELOG.md
This file already exists in this repo, so I might as well setup semantic-release to update it. 😃
@semantic-release/changelog
will update the changelog automatically for us.
package.json
@semantic-release/npm
updates the package.json version, so might as well commit 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.
See #289 (comment) regarding package.json.
.github/workflows/nodejs.yml
Outdated
- run: 'npm run lint:ts-standard' | ||
- run: 'npm run test' | ||
- run: npm install | ||
- run: npm run lint:commit -- --from "7b4219a67b4fe94b1a1487b7d545d0cc7742f770" --to "${{ github.sha }}" |
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.
Why adding this from
flag with a hard-coded commit hash?
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 is because some old commits in the commit history don't follow the format and cause commitlint to error, as you can see below. (This log excludes the errors caused by dependabot's messages being too long, as body-max-line-len
was disabled at the time of running this.)
$ npm run lint:commit -- --to "79b63eb"
> ts-standard@12.0.3 lint:commit
> commitlint --to 79b63eb
⧗ input: feat(package): Allow multiple projects to be used (#163)
✖ subject must not be sentence-case, start-case, pascal-case, upper-case [subject-case]
✖ found 1 problems, 0 warnings
ⓘ Get help: https://github.com/conventional-changelog/commitlint/#what-is-commitlint
⧗ input: fix(badges): Fix readme file badges
✖ subject must not be sentence-case, start-case, pascal-case, upper-case [subject-case]
✖ found 1 problems, 0 warnings
ⓘ Get help: https://github.com/conventional-changelog/commitlint/#what-is-commitlint
⧗ input: fix(typescript): Try to make typescript a peerDependency
✖ subject must not be sentence-case, start-case, pascal-case, upper-case [subject-case]
✖ found 1 problems, 0 warnings
ⓘ Get help: https://github.com/conventional-changelog/commitlint/#what-is-commitlint
⧗ input: Initial commit
✖ subject may not be empty [subject-empty]
✖ type may not be empty [type-empty]
✖ found 2 problems, 0 warnings
ⓘ Get help: https://github.com/conventional-changelog/commitlint/#what-is-commitlint
I've hardcoded it from the latest commit on standard/ts-standard as that's the last commit before my commits, and this is when I've setup a precommit hook for commitlint.
New and updated dependency changes detected. Learn more about Socket for GitHub ↗︎
Footnotes |
@theoludwig What are your thoughts on:
Edit: the only issues with this are that manually releasing can be a security vulnerability and npm provenance doesn't apply here (consequences of publishing outside of a CI environment). |
commitlint's --from flag has anyways been hardcoded to ignore old dependabot messages, so this rule should be re-enabled.
This is to ensure only commits pushed to the repo are published, and not pull requests that have not yet been merged (we only want tests to run for these).
- run: npm install | ||
- run: npx semantic-release | ||
env: | ||
GITHUB_TOKEN: ${{ secrets.GH_TOKEN }} |
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.
@theoludwig what are your thoughts on using a Personal Access Token instead of the default GITHUB_TOKEN
secret that GitHub provides?
From a note in semantic-release's documentation:
Automatically populated
GITHUB_TOKEN
cannot be used if branch protection is enabled for the target branch. It is not advised to mitigate this limitation by overriding an automatically populatedGITHUB_TOKEN
variable with a Personal Access Token, as it poses a security risk. Since Secret Variables are available for Workflows triggered by any branch, it becomes a potential vector of attack, where a Workflow triggered from a non-protected branch can expose and use a token with elevated permissions, yielding branch protection insignificant. One can use Personal Access Tokens in trusted environments, where all developers should have the ability to perform administrative actions in the given repository and branch protection is enabled solely for convenience purposes, to remind about required reviews or CI checks.
I'm unsure to whether ts-standard has branch protection enabled on the master branch (I'm guessing yes).
What is the purpose of this pull request? (put an "X" next to item)
What changes did you make? (Give an overview)
semantic-release
.commitlint
,standard
, andts-standard
.Which issue (if any) does this pull request address?
Is there anything you'd like reviewers to focus on?
Please check that all the configuration is what is needed.
Before merging, make sure the GitHub repository secrets
GH_TOKEN
andNPM_TOKEN
are set to a GitHub Personal Access Token and NPM Access Token respectively.