Skip to content
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

Adds npm token config to the automatic release process #103

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

luislard
Copy link
Contributor

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes/features)
  • Docs have been added/updated (for bug fixes/features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Exposes a config to be able to define the NPM_TOKEN used by the release workflow to publish to NPM registry

What is the current behavior? (You can also link to an open issue here)
There is no way to set this value

What is the new behavior (if this is a feature change)?
A new secret config is available

Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
No

Other information:

@@ -11,6 +11,9 @@ on:
GITHUB_USER_TOKEN:
description: Authentication token with write permission needed by the release bot (falls back to GITHUB_TOKEN).
required: false
NPM_TOKEN:
Copy link
Member

Choose a reason for hiding this comment

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

Isn't that the same as the NPM_REGISTRY_TOKEN - see for example https://github.com/inpsyde/reusable-workflows/blob/main/.github/workflows/wp-scripts-lint.yml#L56-L58 which is put into NODE_AUTH_TOKEN: ${{ secrets.NPM_REGISTRY_TOKEN }} - see https://github.com/inpsyde/reusable-workflows/blob/main/.github/workflows/wp-scripts-lint.yml#L78 ?

The Github action "setup-node" will automatically read this token from env.NODE_AUTH_TOKEN and set it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is aligned to use the NPM_REGISTRY_TOKEN secret and internally we use it to set NPM_TOKEN since this is what it is consumed by the underlying implementation see: https://github.com/semantic-release/semantic-release/blob/master/docs/usage/ci-configuration.md#authentication-for-plugins

Copy link
Member

@Chrico Chrico Apr 30, 2024

Choose a reason for hiding this comment

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

That's fine if NPM_TOKEN is used for semantic-release, but we always name our inputs/secrets the same. The input should be named NPM_REGISTRY_TOKEN, how you use it internally is up to you or the consuming action/step.

@luislard luislard requested a review from Chrico April 29, 2024 11:43
@Chrico Chrico requested a review from tyrann0us April 30, 2024 08:42
Copy link
Member

@tyrann0us tyrann0us left a comment

Choose a reason for hiding this comment

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

Can we align everything npm token-related to the other workflows and documentations?
Please ensure to update the documentation afterward (e.g., new table for inputs, etc.). Thanks!

Comment on lines 5 to +10
GITHUB_USER_TOKEN:
description: Authentication token with write permission needed by the release bot (falls back to GITHUB_TOKEN).
required: false
NPM_REGISTRY_TOKEN:
description: Authentication token with write permission needed by NPM to release a package (falls back to GITHUB_TOKEN).
required: false
Copy link
Member

Choose a reason for hiding this comment

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

Can we move this above GITHUB_USER_TOKEN to align with other workflows that put it above any GITHUB_* secret?

Suggested change
GITHUB_USER_TOKEN:
description: Authentication token with write permission needed by the release bot (falls back to GITHUB_TOKEN).
required: false
NPM_REGISTRY_TOKEN:
description: Authentication token with write permission needed by NPM to release a package (falls back to GITHUB_TOKEN).
required: false
NPM_REGISTRY_TOKEN:
description: Authentication for the private npm registry.
required: false
GITHUB_USER_TOKEN:
description: Authentication token with write permission needed by the release bot (falls back to GITHUB_TOKEN).
required: false

I also updated the description to align it with other workflows.

@@ -69,4 +72,5 @@ jobs:
- name: Release
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_USER_TOKEN != '' && secrets.GITHUB_USER_TOKEN || secrets.GITHUB_TOKEN }}
NPM_TOKEN: ${{ secrets.NPM_REGISTRY_TOKEN != '' && secrets.NPM_REGISTRY_TOKEN || secrets.GITHUB_TOKEN }}
Copy link
Member

Choose a reason for hiding this comment

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

Let's align this with other workflows. Please

  1. move this to the job-wide env declaration above HAS_CONFIG (line 18)
  2. rename the env variable to NODE_AUTH_TOKEN (this is automatically read by actions/setup-node; see documentation)

Also, is falling back to GITHUB_TOKEN a real scenario? We don't do this in other workflows, so I wonder whether this is needed. An example that doesn't do this:

NODE_AUTH_TOKEN: ${{ secrets.NPM_REGISTRY_TOKEN }}

Copy link
Member

Choose a reason for hiding this comment

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

Please add the following input:

NPM_REGISTRY_DOMAIN:
  description: Domain of the private npm registry.
  default: https://npm.pkg.github.com/
  required: false
  type: string

Then, use this in the "Set up node" step like this:

- name: Set up node
  uses: actions/setup-node@v4
  with:
    node-version-file: semantic-release-repo/package.json
    registry-url: ${{ inputs.NPM_REGISTRY_DOMAIN }}

Note: I renamed the step name from "Setup Node.js" to align with other workflows.

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.

[Feature Request]: In automatic release support NPM_TOKEN to be able to publish to package registry
3 participants