feat: add CI for auto publishing CLI #10
Conversation
WalkthroughA new GitHub Actions workflow file named Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
- .github/workflow/npm-publish.yaml (1 hunks)
- scripts/is-release-needed.sh (1 hunks)
🧰 Additional context used
🪛 Shellcheck
scripts/is-release-needed.sh
[info] 7-7: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 7-7: Double quote to prevent globbing and word splitting.
(SC2086)
🔇 Additional comments (4)
.github/workflow/npm-publish.yaml (4)
1-7: LGTM: Workflow name and trigger events are well-defined.The workflow name "release-cli-if-necessary" accurately describes its purpose. The trigger events (push to main and manual dispatch) are appropriate for an automated release process, allowing for both automatic and manual initiation when needed.
25-34: 🧹 Nitpick (assertive)LGTM with suggestions: Release check steps are well-implemented.
The release check steps are logically structured:
- Making the script executable ensures it can be run.
- Using a separate script for the release check logic improves maintainability.
- Storing the result as a GitHub Actions output enables conditional execution of subsequent steps.
However, there are some potential improvements:
- Consider including the
is-release-needed.shscript in the repository with the correct permissions (755) to avoid the need for thechmodstep.- The
echostatements in the "Check if version number has already been released" step could be more informative. Consider updating them to provide more context about what's being checked and the result.Here's a suggested improvement for the echo statements:
- echo "Checking if request-injector is already published..." + echo "Checking if a new release of request-injector is needed..." - echo "is-release-needed=$IS_RELEASE_NEEDED" + echo "Release needed: $IS_RELEASE_NEEDED"Let's verify the existence and permissions of the
is-release-needed.shscript:#!/bin/bash # Description: Check the existence and permissions of is-release-needed.sh # Test: Verify the script exists and check its permissions ls -l ./scripts/is-release-needed.sh 2>/dev/null || echo "Script not found in ./scripts/"
36-44: 🧹 Nitpick (assertive)LGTM with suggestions: Build and publish steps are correctly implemented.
The build and publish steps are well-structured:
- Conditional execution based on the release check prevents unnecessary builds and publishes.
- Building the package before publishing ensures the latest changes are included.
- Using a secret for npm authentication is a secure practice.
However, there are some potential improvements:
- Consider adding a step to update the package version before building and publishing. This could be done using
npm versioncommand.- It might be beneficial to add a step to generate or update a changelog before publishing.
- Consider adding a step to create a git tag for the new version.
Here's a suggested addition before the build step:
- name: Update package version if: steps.is-release-needed.outputs.is-release-needed == 'true' run: | npm version patch -m "Bump version to %s [skip ci]" git push --follow-tagsLet's verify if there's a script for updating the changelog:
#!/bin/bash # Description: Check for the existence of a changelog update script # Test: Look for a changelog update script in the scripts directory ls ./scripts/*changelog* 2>/dev/null || echo "No changelog script found in ./scripts/"
1-44: 🧹 Nitpick (assertive)Overall, the workflow effectively automates the CLI release process.
This workflow successfully addresses the main objective of automating the release process for the CLI, as outlined in the PR objectives. It eliminates the need for manual intervention in the publishing workflow, making it more efficient and less prone to human error.
Key strengths:
- Conditional execution prevents unnecessary builds and publishes.
- Secure handling of npm authentication using GitHub secrets.
- Abstraction of release check logic into a separate script for maintainability.
Potential enhancements to consider:
- Add a step to update the package version automatically.
- Implement changelog generation or updating as part of the release process.
- Create a git tag for each new release.
- Consider adding a step to run tests before building and publishing.
- Implement a notification system (e.g., Slack, email) to alert the team of successful releases.
These enhancements would further streamline the release process and improve transparency and traceability of releases.
Let's check if there are any test scripts defined in the package.json:
#!/bin/bash # Description: Check for test scripts in package.json # Test: Look for test scripts in package.json jq '.scripts | keys | map(select(. | startswith("test")))' package.json
| #!/usr/bin/env bash | ||
|
|
||
| # This script checks if the current version of the package is already published on npm | ||
| PACKAGE_VERSION="$(node -p -e "require('./package.json').version")" | ||
| PACKAGE_NAME="$(node -p -e "require('./package.json').name")" | ||
|
|
||
| FOUND_VERSION=$(npm view $PACKAGE_NAME versions | grep $PACKAGE_VERSION) | ||
|
|
||
| if [ -z "$FOUND_VERSION" ]; then | ||
| echo 'true' # release needed | ||
| else | ||
| echo 'false' # release not needed | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance overall script structure and error handling
While the individual parts of the script have been addressed in previous comments, there's an opportunity to improve the overall structure and error handling of the script.
Consider the following improvements:
- Add a main function to encapsulate the script logic.
- Implement proper error handling and logging.
- Use set options to make the script more robust.
Here's a suggested restructure of the entire script:
#!/usr/bin/env bash
set -euo pipefail
log_error() {
echo "ERROR: $*" >&2
}
check_prerequisites() {
if ! command -v node &> /dev/null; then
log_error "Node.js is required but not installed."
exit 1
fi
if [ ! -f "./package.json" ]; then
log_error "package.json not found in the current directory."
exit 1
fi
}
get_package_info() {
PACKAGE_VERSION=$(node -p -e "require('./package.json').version") || { log_error "Failed to extract package version."; exit 1; }
PACKAGE_NAME=$(node -p -e "require('./package.json').name") || { log_error "Failed to extract package name."; exit 1; }
}
check_npm_version() {
if npm view "$PACKAGE_NAME" versions --json 2>/dev/null | grep -q "\"$PACKAGE_VERSION\""; then
echo "Release not needed: Version $PACKAGE_VERSION of $PACKAGE_NAME already exists on npm"
exit 1
else
echo "Release needed: Version $PACKAGE_VERSION of $PACKAGE_NAME not found on npm"
exit 0
fi
}
main() {
check_prerequisites
get_package_info
check_npm_version
}
main "$@"This restructured version:
- Uses a main function to organize the script flow.
- Implements proper error handling and logging.
- Uses
set -euo pipefailfor safer script execution. - Breaks down the logic into smaller, focused functions.
These changes make the script more robust, easier to maintain, and more aligned with best practices for Bash scripting.
🧰 Tools
🪛 Shellcheck
[info] 7-7: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 7-7: Double quote to prevent globbing and word splitting.
(SC2086)
| if [ -z "$FOUND_VERSION" ]; then | ||
| echo 'true' # release needed | ||
| else | ||
| echo 'false' # release not needed | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing the output format for better CI/CD integration
The current logic for determining if a release is needed is correct. However, the output format can be improved for better integration with CI/CD pipelines and increased clarity.
Consider the following enhancements:
- Use exit codes along with the output for easier script integration.
- Provide more informative output messages.
Here's an improved version:
if [ -z "$FOUND_VERSION" ]; then
- echo 'true' # release needed
+ echo "Release needed: Version $PACKAGE_VERSION of $PACKAGE_NAME not found on npm"
+ exit 0 # Exit code 0 indicates a release is needed
else
- echo 'false' # release not needed
+ echo "Release not needed: Version $PACKAGE_VERSION of $PACKAGE_NAME already exists on npm"
+ exit 1 # Exit code 1 indicates a release is not needed
fiThis change provides more context in the output and uses standard exit codes, making it easier to integrate this script into CI/CD workflows and interpret its results.
📝 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.
| if [ -z "$FOUND_VERSION" ]; then | |
| echo 'true' # release needed | |
| else | |
| echo 'false' # release not needed | |
| fi | |
| if [ -z "$FOUND_VERSION" ]; then | |
| echo "Release needed: Version $PACKAGE_VERSION of $PACKAGE_NAME not found on npm" | |
| exit 0 # Exit code 0 indicates a release is needed | |
| else | |
| echo "Release not needed: Version $PACKAGE_VERSION of $PACKAGE_NAME already exists on npm" | |
| exit 1 # Exit code 1 indicates a release is not needed | |
| fi |
| PACKAGE_VERSION="$(node -p -e "require('./package.json').version")" | ||
| PACKAGE_NAME="$(node -p -e "require('./package.json').name")" | ||
|
|
||
| FOUND_VERSION=$(npm view $PACKAGE_NAME versions | grep $PACKAGE_VERSION) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance npm version check reliability
The current implementation checks for the package version on npm, but it can be improved for better reliability and to address potential issues flagged by Shellcheck.
Consider the following improvements:
-FOUND_VERSION=$(npm view $PACKAGE_NAME versions | grep $PACKAGE_VERSION)
+FOUND_VERSION=$(npm view "$PACKAGE_NAME" versions --json 2>/dev/null | grep -q "\"$PACKAGE_VERSION\"" && echo "found" || echo "")This change:
- Adds double quotes to prevent globbing and word splitting, as suggested by Shellcheck.
- Uses the
--jsonflag for more reliable parsing. - Redirects stderr to /dev/null to suppress potential warnings.
- Uses grep with
-qfor a quiet check and sets FOUND_VERSION based on the grep result.
These modifications make the version check more robust and less prone to errors from unexpected npm output or special characters in the package name or version.
📝 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.
| FOUND_VERSION=$(npm view $PACKAGE_NAME versions | grep $PACKAGE_VERSION) | |
| FOUND_VERSION=$(npm view "$PACKAGE_NAME" versions --json 2>/dev/null | grep -q "\"$PACKAGE_VERSION\"" && echo "found" || echo "") |
🧰 Tools
🪛 Shellcheck
[info] 7-7: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 7-7: Double quote to prevent globbing and word splitting.
(SC2086)
| #!/usr/bin/env bash | ||
|
|
||
| # This script checks if the current version of the package is already published on npm | ||
| PACKAGE_VERSION="$(node -p -e "require('./package.json').version")" | ||
| PACKAGE_NAME="$(node -p -e "require('./package.json').name")" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve robustness of package information extraction
The script correctly uses the shebang for Bash and extracts package information using Node.js. However, to enhance robustness:
- Consider adding error handling for cases where Node.js is not installed or
package.jsonis not found. - Quote the Node.js commands to handle potential spaces or special characters in package names or versions.
Here's an improved version:
#!/usr/bin/env bash
# This script checks if the current version of the package is already published on npm
-PACKAGE_VERSION="$(node -p -e "require('./package.json').version")"
-PACKAGE_NAME="$(node -p -e "require('./package.json').name")"
+if ! command -v node &> /dev/null; then
+ echo "Error: Node.js is required but not installed." >&2
+ exit 1
+fi
+
+if [ ! -f "./package.json" ]; then
+ echo "Error: package.json not found in the current directory." >&2
+ exit 1
+fi
+
+PACKAGE_VERSION="$(node -p -e "require('./package.json').version")" || { echo "Error: Failed to extract package version." >&2; exit 1; }
+PACKAGE_NAME="$(node -p -e "require('./package.json').name")" || { echo "Error: Failed to extract package name." >&2; exit 1; }This change adds error handling and ensures the script fails gracefully if prerequisites are not met.
📝 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.
| #!/usr/bin/env bash | |
| # This script checks if the current version of the package is already published on npm | |
| PACKAGE_VERSION="$(node -p -e "require('./package.json').version")" | |
| PACKAGE_NAME="$(node -p -e "require('./package.json').name")" | |
| #!/usr/bin/env bash | |
| # This script checks if the current version of the package is already published on npm | |
| if ! command -v node &> /dev/null; then | |
| echo "Error: Node.js is required but not installed." >&2 | |
| exit 1 | |
| fi | |
| if [ ! -f "./package.json" ]; then | |
| echo "Error: package.json not found in the current directory." >&2 | |
| exit 1 | |
| fi | |
| PACKAGE_VERSION="$(node -p -e "require('./package.json').version")" || { echo "Error: Failed to extract package version." >&2; exit 1; } | |
| PACKAGE_NAME="$(node -p -e "require('./package.json').name")" || { echo "Error: Failed to extract package name." >&2; exit 1; } |
| jobs: | ||
| release-cli-if-necessary: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout repository 🛎️ | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Set up Node.js | ||
| uses: actions/setup-node@v3 | ||
| with: | ||
| node-version: '18' | ||
| registry-url: 'https://registry.npmjs.org' | ||
|
|
||
| - name: Install dependencies 📥 | ||
| run: npm ci |
There was a problem hiding this comment.
💡 Codebase verification
Action Required: Node.js version not specified in project
The workflow specifies Node.js v18, but the project does not specify which Node.js version to use. To ensure consistency and clarity, please specify the Node.js version in either:
- The
enginesfield ofpackage.json, for example:"engines": { "node": ">=18" }
- A
.nvmrcfile at the root of the project with the desired Node.js version, e.g.,18.
🔗 Analysis chain
LGTM: Job setup and initial steps are well-configured.
The job configuration and initial steps are appropriate for a Node.js project:
- Using the latest Ubuntu ensures good security and feature support.
- The repository checkout and Node.js setup are correctly implemented.
- Using
npm cifor installing dependencies is the right choice for CI environments.
Please verify that Node.js v18 is the intended version for this project. If a different version is required, update the node-version field in the Node.js setup step.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Node.js version specified in the project
# Test: Check for Node.js version specification in package.json or .nvmrc
rg --type json '"node":' package.json
cat .nvmrc 2>/dev/null || echo "No .nvmrc file found"
Length of output: 265
Script:
#!/bin/bash
# Description: Verify the Node.js version specified in the project
# Check for Node.js version in package.json's "engines" field
rg '"engines"\s*:\s*{\s*"node"\s*:\s*"' package.json
# Check for .nvmrc file and display its content if it exists
if [ -f .nvmrc ]; then
cat .nvmrc
else
echo "No .nvmrc file found."
fi
Length of output: 261
Script:
#!/bin/bash
# Description: Verify the Node.js version specified in the project
# Check for Node.js version in package.json's "engines" field using jq
if command -v jq >/dev/null 2>&1; then
node_version=$(jq -r '.engines.node // empty' package.json)
if [ -n "$node_version" ]; then
echo "Node.js version specified in package.json: $node_version"
else
echo "No Node.js version specified in package.json."
fi
else
echo "jq is not installed. Cannot parse package.json."
fi
# Check for .nvmrc file and display its content if it exists
if [ -f .nvmrc ]; then
echo "Node.js version specified in .nvmrc:"
cat .nvmrc
else
echo "No .nvmrc file found."
fi
Length of output: 268
Resolves #4
Summary by CodeRabbit
New Features
Chores