feat: setup trusted publisher #149
Conversation
|
see https://github.com/expressjs/codemod/settings/environments/9670255041/edit, the enviroment config for codemod |
sheplu
force-pushed
the
add-trusted-publisher
branch
from
d602cdf to
c374a35
Compare
.github/workflows/npm-release.yaml
Outdated
| @@ -0,0 +1,38 @@ | |||
| name: Publish package | |||
There was a problem hiding this comment.
Naming nit-picks:
- file name: could we go with
release.yaml? - worfklow name: could we go with just
Publish? - environment name: could we use
npm?
There was a problem hiding this comment.
updated, I am fine with the new name
There was a problem hiding this comment.
Oh, I reviewed first (missing old comments) and disagreed. I think these should all match to simplify debugging of action <> env <> file. So all should be publish or release. With three different names it's confusing for someone new or returning.
Edit: Personally I'd name it publish because it's npm publish and not npm release.
There was a problem hiding this comment.
It'd be like creating a ci.yaml called name: Test running in env node. Hard to debug/review.
There was a problem hiding this comment.
I will align everything to "publish" if everyone is fine with that :D
.github/workflows/release.yaml
Outdated
| npm version $VERSION --no-git-tag-version | ||
| else | ||
| echo "Version already set to $VERSION, skipping npm version command" | ||
| fi |
There was a problem hiding this comment.
I don't think this version logic is quite right. In order to create a release you first need a versioned commit with a tag so I think you either need to change the trigger or change this logic.
There was a problem hiding this comment.
this is a specific logic (working) that basically put the tag from the github release (suggesting that we tag the release with v1.1.1)
In the case of our project, as we are manually updating tags on the package.json file I can fully remove that? what would be the way you would like to see it?
There was a problem hiding this comment.
I think it should not automatically version, but instead exit if it doesn't match.
sheplu
force-pushed
the
add-trusted-publisher
branch
from
c374a35 to
5228ed5
Compare
.github/workflows/release.yaml
Outdated
| jobs: | ||
| publish: | ||
| runs-on: ubuntu-latest | ||
| environment: npm |
There was a problem hiding this comment.
Nit: Would name it Release (or both Publish) to make it easier to debug/find.
.github/workflows/release.yaml
Outdated
| @@ -0,0 +1,38 @@ | |||
| name: Publish | |||
There was a problem hiding this comment.
Nit: The name of the file is release.yaml but the script Publish, would be nicer to align them.
.github/workflows/release.yaml
Outdated
| name: Publish | ||
| on: | ||
| release: | ||
| types: [created] |
There was a problem hiding this comment.
I'd do on tag, because otherwise you are releasing without having a release actually available for anyone. This is doubly so with the environment approval and/or NPM staged publishing, it could be hours before the "release" is actually available.
|
Oh! I thought we wanted to address this approach first #142... but if you prefer to try this one we can go, but it will be hard (open a request to support) to revert it if we want to use CI publication again right? |
you mean on NPM? As we discussed yesterday about NPM trusted publisher and that we wanted something directly in the repo, I just opened this PR |
sheplu
force-pushed
the
add-trusted-publisher
branch
from
5228ed5 to
df526fd
Compare
blakeembrey
left a comment
blakeembrey
left a comment
There was a problem hiding this comment.
I’d also be for not pinning the official actions but that’s up to the maintainers.
| on: | ||
| push: | ||
| tags: | ||
| - '*.*.*' |
5 participants
Setup to be done on the repository:
npmhere, can be changed to publish)Setup to be done on npm: