Support for private registry NPM packages by using NPM_TOKEN and .npmrc (#1390) #1405
Conversation
|
WalkthroughThe pull request introduces several modifications to enhance the repository's build and release processes. Key changes include the addition of an Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (6)
.npmrc (1)
3-3: LGTM! Consider documenting the Prisma hoisting pattern.The public-hoist-pattern for Prisma packages is correctly maintained. This is important for managing Prisma dependencies in a monorepo setup.
Consider adding a comment explaining the purpose of this hoisting pattern, especially if it's crucial for your project setup. For example:
+ # Hoist Prisma packages to the root to avoid multiple instances in monorepo public-hoist-pattern[]=*prisma*docker/scripts/build-extension.sh (1)
1-5: LGTM with a minor suggestion for improvement.The initial setup looks good. The shebang is correct, and the script provides feedback about copying the .npmrc file. However, consider adding error handling to ensure the .npmrc file exists and the copy operation succeeds.
Here's a suggested improvement:
#!/bin/bash +set -e # Exit immediately if a command exits with a non-zero status + # Copy the .npmrc file into the build directory echo "Copying .npmrc to build context..." -cp .npmrc /app/.npmrc +cp .npmrc /app/.npmrc || { echo "Error: Failed to copy .npmrc file"; exit 1; }This change will make the script exit if the copy operation fails, preventing further execution with an incomplete setup.
.github/workflows/release.yml (1)
Line range hint
1-105: Request clarification on the overall release strategy.The changes in this file introduce npm authentication using
NPM_TOKEN, which aligns with the goal of supporting private registry NPM packages. However, the disabling of automatic publishing seems to contradict the overall objective of enhancing the build and release processes.Could you please provide a comprehensive explanation of the intended release strategy? This should cover:
- How the
NPM_TOKENwill be used throughout the workflow.- The reasoning behind disabling automatic publishing.
- The planned process for managing releases in the absence of automatic publishing.
- Any interim steps or manual processes that will be used.
- The timeline or conditions for re-enabling automatic publishing.
This information will help ensure that the changes align with the project's long-term goals and that all team members understand the new release process.
docker/Dockerfile (3)
31-32: Redundant Copy of.npmrcindev-depsStageCopying
.npmrcagain in thedev-depsstage may be unnecessary since it's already included in thebasestage. This redundancy can be eliminated to simplify the Dockerfile.If the copy is required due to layer scoping in multi-stage builds, consider adding a comment to explain the necessity for future maintainers.
41-42: Redundant Copy of.npmrcinproduction-depsStageSimilar to the
dev-depsstage, copying.npmrcagain in theproduction-depsstage appears redundant if it's already present from thebasestage.Clarify if this duplication is required due to Docker's layer caching or stage isolation. Adding an explanatory comment can improve maintainability.
17-19: Simplify the Conditional.npmrcModificationSince
.npmrcis being copied explicitly, the check for its existence might be unnecessary. Simplifying the conditional can make the Dockerfile cleaner.You can remove the
[ -f .npmrc ]condition:RUN if [ -n "${NPM_TOKEN}" ]; then \ echo "//registry.npmjs.org/:_authToken=${NPM_TOKEN}" >> .npmrc; \ fiThis assumes
.npmrcexists due to the earlierCOPYcommand.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
- .github/workflows/release.yml (1 hunks)
- .npmrc (1 hunks)
- .nvmrc (1 hunks)
- docker/Dockerfile (3 hunks)
- docker/scripts/build-extension.sh (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .nvmrc
🧰 Additional context used
🔇 Additional comments (5)
.npmrc (1)
1-1: LGTM! Verify NPM_TOKEN in CI/CD environment.The addition of the auth token configuration aligns with the PR objective of supporting private registry NPM packages. Using an environment variable for the token is a good security practice.
Please ensure that the
NPM_TOKENenvironment variable is properly set in your CI/CD environment. You can verify this by running the following command in your CI/CD pipeline:docker/scripts/build-extension.sh (1)
1-12: Overall improvements and security considerationThe script achieves its main objectives of setting up the npm environment and installing dependencies. However, there are a few overall improvements and a security consideration to address:
- Add error handling throughout the script to catch and report failures at each step.
- Consider adding a cleanup step to remove sensitive information after installation.
- Use set -e at the beginning of the script to exit on any command failure.
Here's a suggested structure for the improved script:
#!/bin/bash set -e cleanup() { echo "Cleaning up..." rm -f /app/.npmrc unset NPM_TOKEN } trap cleanup EXIT # ... (rest of the script with improvements from previous comments) # After pnpm install echo "Installation completed successfully."This structure ensures cleanup of sensitive information even if the script exits prematurely.
To verify the impact of these changes on the build process, you may want to run:
This will help ensure that NPM_TOKEN is not hardcoded anywhere else in the codebase, which could pose a security risk.
✅ Verification successful
Verification Successful
The script does not contain any hardcoded
NPM_TOKENvalues and appropriately uses environment variables to manage sensitive information.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any hardcoded NPM_TOKEN in the codebase # Test: Search for NPM_TOKEN assignments rg 'NPM_TOKEN\s*=' --type shLength of output: 84
Script:
#!/bin/bash # Description: Check for any hardcoded tokens or sensitive information in .npmrc files # Test: Search for token-related entries in .npmrc rg '(_token|auth|_auth)' --type markdown --file '.npmrc'Length of output: 281
Script:
#!/bin/bash # Description: Check for any hardcoded tokens or sensitive information in .npmrc files # Test: Search for token-related entries in .npmrc rg '(_authToken|authToken|NPM_TOKEN)' .npmrcLength of output: 92
.github/workflows/release.yml (2)
Line range hint
103-105: Clarify the rationale for disabling automatic publishing.The addition of
if: falseeffectively disables the automatic publishing process. This seems to contradict the PR objective of enhancing the build and release processes.Could you please provide more information on:
- The reasoning behind disabling automatic publishing at this time?
- The plans and timeline for re-enabling this feature?
- How releases will be managed in the interim?
To verify the impact of this change, we can run the following script to check for any manual publishing steps that might have been added elsewhere:
#!/bin/bash # Description: Check for manual publishing steps in workflow files echo "Searching for manual publishing steps in workflow files:" rg --type yaml "npm publish" .github/workflows -C 3 echo "Checking for other jobs that might handle publishing:" rg --type yaml "jobs:.*publish" .github/workflows -C 5
45-46: Approve the addition of NPM_TOKEN, but clarification needed.The addition of the
NPM_TOKENenvironment variable is a good step towards supporting private registry NPM packages. However, it's not immediately clear how pnpm utilizes this environment variable during the dependency installation process.Could you please clarify how pnpm makes use of the
NPM_TOKENenvironment variable? If it's not directly used by pnpm, consider moving this environment variable to a more relevant step where it's actively utilized.To verify the usage of NPM_TOKEN in the project, we can run the following script:
✅ Verification successful
NPM_TOKEN Usage Verified
The
NPM_TOKENenvironment variable is correctly utilized in the.npmrcfile for npm authentication. No further issues detected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for NPM_TOKEN usage in project files # Search for NPM_TOKEN usage in project files echo "Searching for NPM_TOKEN usage:" rg --type yaml --type javascript --type typescript "NPM_TOKEN" -C 3 # Check if .npmrc file exists and its contents echo "Checking .npmrc file:" if [ -f ".npmrc" ]; then cat .npmrc else echo ".npmrc file not found" fiLength of output: 445
Script:
#!/bin/bash # Description: Check for NPM_TOKEN usage in project files # Search for NPM_TOKEN usage in project files echo "Searching for NPM_TOKEN usage:" rg --type yaml --type javascript --type typescript "NPM_TOKEN" -C 3 # Check if .npmrc file exists and its contents echo "Checking .npmrc file:" if [ -f ".npmrc" ]; then cat .npmrc else echo ".npmrc file not found" fiLength of output: 367
docker/Dockerfile (1)
15-19: Add.npmrcto.dockerignoreIncluding
.npmrcin the build context can inadvertently copy sensitive information into the image if the file contains other settings or credentials.Ensure that
.npmrcis listed in your.dockerignorefile to prevent unintentional inclusion. Since you're copying it explicitly in the Dockerfile, the build context doesn't need to include it implicitly.
.npmrc
Outdated
| //registry.npmjs.org/:_authToken=${NPM_TOKEN} | ||
| link-workspace-packages=false | ||
| public-hoist-pattern[]=*prisma* No newline at end of file | ||
| public-hoist-pattern[]=*prisma* |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding common npm configurations.
The current .npmrc file covers the essential configurations for your use case. However, you might want to consider adding some common npm configurations that could be beneficial for your project:
- Set a default save prefix:
save-exact=true
- Configure package-lock:
package-lock=true
- Set a default node version:
engine-strict=true
- Configure npm to use a specific registry for certain scopes:
@myorg:registry=https://registry.myorg.com/
These are just suggestions. Please add only the configurations that make sense for your project's needs.
| # Run the npm install command | ||
| echo "Running npm install with NPM_TOKEN..." | ||
| pnpm install |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize and improve error handling for package installation.
The use of pnpm for package installation is fine if it's the intended package manager. However, there are a few improvements we can make:
- Consider adding the
--productionflag to avoid installing dev dependencies if this is for a production build. - Add error handling to catch installation failures.
- Optionally, add a
--frozen-lockfileflag to ensure consistent installations.
Here's a suggested improvement:
# Run the npm install command
echo "Running npm install with NPM_TOKEN..."
-pnpm install
+pnpm install --production --frozen-lockfile || { echo "Error: pnpm install failed"; exit 1; }This change will:
- Only install production dependencies
- Ensure consistent installations across environments
- Exit the script if the installation fails
Note: If dev dependencies are needed, remove the --production flag.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Run the npm install command | |
| echo "Running npm install with NPM_TOKEN..." | |
| pnpm install | |
| # Run the npm install command | |
| echo "Running npm install with NPM_TOKEN..." | |
| pnpm install --production --frozen-lockfile || { echo "Error: pnpm install failed"; exit 1; } |
| # Set the NPM_TOKEN environment variable for the build | ||
| export NPM_TOKEN=$1 |
There was a problem hiding this comment.
Enhance security and robustness of NPM_TOKEN handling.
While setting the NPM_TOKEN is necessary, the current implementation has some potential issues:
- It assumes the token is always provided as the first argument.
- There's no validation of the token.
- The token is exposed in the environment variables.
Consider implementing these improvements:
# Set the NPM_TOKEN environment variable for the build
-export NPM_TOKEN=$1
+if [ -z "$1" ]; then
+ echo "Error: NPM_TOKEN not provided"
+ exit 1
+fi
+
+# Basic validation of the token (adjust regex as needed)
+if ! [[ $1 =~ ^[a-zA-Z0-9_-]+$ ]]; then
+ echo "Error: Invalid NPM_TOKEN format"
+ exit 1
+fi
+
+# Use read -s to avoid showing the token in process list
+read -r -s NPM_TOKEN <<< "$1"
+export NPM_TOKENThis change adds error checking, basic token validation, and uses read -s to reduce token visibility.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Set the NPM_TOKEN environment variable for the build | |
| export NPM_TOKEN=$1 | |
| # Set the NPM_TOKEN environment variable for the build | |
| if [ -z "$1" ]; then | |
| echo "Error: NPM_TOKEN not provided" | |
| exit 1 | |
| fi | |
| # Basic validation of the token (adjust regex as needed) | |
| if ! [[ $1 =~ ^[a-zA-Z0-9_-]+$ ]]; then | |
| echo "Error: Invalid NPM_TOKEN format" | |
| exit 1 | |
| fi | |
| # Use read -s to avoid showing the token in process list | |
| read -r -s NPM_TOKEN <<< "$1" | |
| export NPM_TOKEN |
docker/Dockerfile
Outdated
| ARG NPM_TOKEN | ||
| COPY .npmrc .npmrc | ||
| RUN if [ -f .npmrc ]; then \ | ||
| echo "//registry.npmjs.org/:_authToken=${NPM_TOKEN}" >> .npmrc; \ | ||
| fi |
There was a problem hiding this comment.
Security Concern: Avoid Exposing NPM_TOKEN in Build Layers
Including NPM_TOKEN directly in the Dockerfile can inadvertently expose it in the image layers, which poses a security risk if the image is ever shared publicly. Consider using Docker's build-time secrets to handle sensitive information securely.
You can modify the Dockerfile to use build-time secrets with Docker BuildKit:
# Use the secret in the RUN command without storing it in a layer
RUN --mount=type=secret,id=npm_token \
echo "//registry.npmjs.org/:_authToken=$(cat /run/secrets/npm_token)" >> .npmrcThen, build the image with the secret:
DOCKER_BUILDKIT=1 docker build --secret id=npm_token,src=npm_token.txt .This approach ensures that NPM_TOKEN is not written to any layers in the final image.
Conditional Check for NPM_TOKEN
If NPM_TOKEN is not provided, the script appends an empty token to .npmrc, which might cause authentication issues with npm. It's important to check if NPM_TOKEN is set before appending it.
Update the conditional to verify that NPM_TOKEN is not empty:
RUN if [ -f .npmrc ] && [ -n "${NPM_TOKEN}" ]; then \
echo "//registry.npmjs.org/:_authToken=${NPM_TOKEN}" >> .npmrc; \
fiThis ensures the authentication token is only added when it exists.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
docker/Dockerfile (2)
Line range hint
45-53: Optimize production dependency installation and Prisma setup
- Copying .npmrc again might be redundant if it's already in the base stage.
- The pnpm install command uses the
--no-frozen-lockfileflag, which might lead to inconsistent builds.- There's a commented out line for adding @prisma/client.
Consider the following changes:
- Remove the redundant COPY of .npmrc if it's not necessary.
- Modify the pnpm install command to ensure reproducible builds:
RUN pnpm install --prod --frozen-lockfile
- Remove the commented out line:
# Remove this line if it's no longer needed # RUN pnpm add @prisma/client@5.1.1 -wThese changes will improve build consistency and clean up the Dockerfile.
Line range hint
74-93: LGTM: Runner stage is well-structured with a minor suggestionThe runner stage effectively combines the necessary components from previous stages to create the final image. Good practices observed include:
- Using dumb-init for proper signal handling
- Setting the user to node for improved security
For consistency with previous stages, consider using the same format for the FROM instruction:
FROM node:20.11.1-bullseye-slim@sha256:5a5a92b3a8d392691c983719dbdc65d9f30085d6dcd65376e7a32e6fe9bf4cbe AS runnerThis change ensures that the exact same base image is used across all stages.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- docker/Dockerfile (3 hunks)
🧰 Additional context used
🔇 Additional comments (3)
docker/Dockerfile (3)
Line range hint
1-6: LGTM: Pruner stage is well-structuredThe pruner stage effectively reduces the build context size by using
turbo pruneand removingnode_modules. This approach optimizes the subsequent stages of the build process.
Line range hint
56-71: LGTM: Builder stage is well-structuredThe builder stage effectively combines the necessary files and dependencies from previous stages to build the webapp. The use of
--filter=webapp...in the build command is a good practice for optimizing the build process by focusing only on the required packages.
Line range hint
1-93: Summary: Well-structured Dockerfile with room for optimizationOverall, the Dockerfile is well-organized with clear separation of concerns across multiple stages. Key strengths include the use of a pruner stage, proper handling of dependencies, and good security practices in the runner stage.
Main areas for improvement:
- Streamline NPM_TOKEN handling by using only Docker BuildKit secrets.
- Ensure consistent and reproducible builds by using
--frozen-lockfilewith pnpm install commands.- Remove redundant COPY instructions for .npmrc in dev-deps and production-deps stages.
- Clean up commented out lines and ensure consistency in FROM instructions across stages.
Addressing these points will further enhance the security, consistency, and efficiency of the build process.
|
|
||
| # Copy .npmrc and use the NPM_TOKEN for private registry | ||
| ARG NPM_TOKEN | ||
| COPY .npmrc .npmrc | ||
| RUN if [ -f .npmrc ] && [ -n "${NPM_TOKEN}" ]; then \ | ||
| echo "//registry.npmjs.org/:_authToken=${NPM_TOKEN}" >> .npmrc; \ | ||
| fi | ||
|
|
||
| # For better security, use Docker BuildKit secrets | ||
| RUN --mount=type=secret,id=npm_token \ | ||
| echo "//registry.npmjs.org/:_authToken=$(cat /run/secrets/npm_token)" >> .npmrc |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Streamline NPM_TOKEN handling
The current implementation has two methods for adding the NPM_TOKEN to .npmrc:
- Using an ARG and conditional check (lines 15-19)
- Using Docker BuildKit secrets (lines 22-23)
To simplify and improve security, consider using only the Docker BuildKit secrets method. This approach prevents the token from being stored in intermediate layers.
Replace the current NPM_TOKEN handling with:
# Remove these lines
ARG NPM_TOKEN
COPY .npmrc .npmrc
RUN if [ -f .npmrc ] && [ -n "${NPM_TOKEN}" ]; then \
echo "//registry.npmjs.org/:_authToken=${NPM_TOKEN}" >> .npmrc; \
fi
# Keep only this part
COPY .npmrc .npmrc
RUN --mount=type=secret,id=npm_token \
if [ -f /run/secrets/npm_token ]; then \
echo "//registry.npmjs.org/:_authToken=$(cat /run/secrets/npm_token)" >> .npmrc; \
fiThis change ensures that the NPM_TOKEN is only handled securely using Docker BuildKit secrets.
| ENV NODE_ENV development | ||
| # Copy .npmrc again for dev-deps stage | ||
| COPY .npmrc .npmrc | ||
| RUN pnpm install --ignore-scripts --no-frozen-lockfile |
There was a problem hiding this comment.
Improve dependency installation in dev-deps stage
- Copying .npmrc again might be redundant if it's already in the base stage.
- The pnpm install command uses flags that might lead to inconsistent builds:
--ignore-scriptsskips running scripts specified in package.json--no-frozen-lockfileallows installing newer versions of dependencies
Consider the following changes:
- Remove the redundant COPY of .npmrc if it's not necessary.
- Modify the pnpm install command to ensure reproducible builds:
RUN pnpm install --frozen-lockfileThis change ensures that the exact versions specified in the lockfile are installed, promoting build consistency across environments.
|
You've updated the server docker image, which is not the images that are built for trigger.dev workers. I'd suggest closing this PR and starting over. Maybe watch this to get a better idea how to contribute this feature: https://youtu.be/Ta8OI1q8Jvw?si=mYTsoZ4HYHisF6Td |
Closes #
✅ Checklist
Changelog
#Added functionality to copy the .npmrc file into the Docker build context.
#Added support for the NPM_TOKEN environment variable to authenticate with private NPM registries.
#Updated Dockerfile to ensure private dependencies are installed using npm i during the build phase.
[Short description of what has changed]
This PR adds support for installing private NPM packages by using the .npmrc file and NPM_TOKEN. It includes modifications to the Dockerfile and ensures that private packages are handled during the build phase.
💯
Summary by CodeRabbit
.npmrc..nvmrcfile.