Skip to content

fix: npm グローバルパッケージのdevcontainer対応を改善 #26

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.

Sign up for GitHub

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

Closed
keito4 wants to merge 1 commit into from
Closed

fix: npm グローバルパッケージのdevcontainer対応を改善 #26

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .devcontainer/devcontainer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
"remoteEnv": {
"HOMEBREW_NO_AUTO_UPDATE": "1"
},
"postCreateCommand": "cp /workspaces/config/dot/.zshrc.devcontainer ~/.zshrc && echo 'Devcontainer .zshrc configured' || echo 'Failed to copy .zshrc'",
"overrideCommand": true,
"updateRemoteUserUID": false,
"customizations": {
Expand Down
32 changes: 29 additions & 3 deletions .devcontainer/features/common/install.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,21 @@ if ! command -v npm >/dev/null 2>&1; then
exit 0
fi

# npm キャッシュ
# npm設定とキャッシュディレクトリ
NPM_CACHE_DIR="$HOME/.npm-cache"
mkdir -p "$NPM_CACHE_DIR"
NPM_GLOBAL_DIR="$HOME/.npm-global"
mkdir -p "$NPM_CACHE_DIR" "$NPM_GLOBAL_DIR"

# ディレクトリの権限確認と修正
[ -w "$NPM_CACHE_DIR" ] || chown -R "$(id -u):$(id -g)" "$NPM_CACHE_DIR" || true
[ -w "$NPM_GLOBAL_DIR" ] || chown -R "$(id -u):$(id -g)" "$NPM_GLOBAL_DIR" || true

Comment on lines +21 to +24

@coderabbitai coderabbitai Bot May 30, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Avoid silent failures when fixing ownership
The chown -R … || true pattern suppresses errors that may indicate deeper permission issues. If ownership cannot be corrected, it’s better to log and exit with failure to alert maintainers. For example:

-[ -w "$NPM_CACHE_DIR" ] || chown -R "$(id -u):$(id -g)" "$NPM_CACHE_DIR" || true
+[ -w "$NPM_CACHE_DIR" ] || { echo "ERROR: Cannot set write permissions on $NPM_CACHE_DIR"; exit 1; }

Repeat similarly for $NPM_GLOBAL_DIR.

📝 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.

Suggested change
# ディレクトリの権限確認と修正
[ -w "$NPM_CACHE_DIR" ] || chown -R "$(id -u):$(id -g)" "$NPM_CACHE_DIR" || true
[ -w "$NPM_GLOBAL_DIR" ] || chown -R "$(id -u):$(id -g)" "$NPM_GLOBAL_DIR" || true
# ディレクトリの権限確認と修正
[ -w "$NPM_CACHE_DIR" ] || { echo "ERROR: Cannot set write permissions on $NPM_CACHE_DIR"; exit 1; }
[ -w "$NPM_GLOBAL_DIR" ] || { echo "ERROR: Cannot set write permissions on $NPM_GLOBAL_DIR"; exit 1; }
🤖 Prompt for AI Agents 
In .devcontainer/features/common/install.sh around lines 21 to 24, the current
use of 'chown -R ... || true' suppresses errors when changing ownership of
$NPM_CACHE_DIR and $NPM_GLOBAL_DIR, potentially hiding permission issues. Modify
the script to check the result of each chown command, and if it fails, log an
error message indicating the failure and exit the script with a non-zero status
to alert maintainers instead of silently continuing.

# npm設定
export NPM_CONFIG_CACHE="$NPM_CACHE_DIR"
export NPM_CONFIG_PREFIX="$NPM_GLOBAL_DIR"

# グローバルパッケージのbinディレクトリをPATHに追加（後で.bashrcや.zshrcで設定される）
echo "NPM global bin directory: $NPM_GLOBAL_DIR/bin"

# global.json の探索
GLOBAL_JSON_PATHS=(
Expand Down Expand Up @@ -57,7 +67,23 @@ fi
packages=$(jq -r '.dependencies | keys[]' "$GLOBAL_JSON_FILE" 2>/dev/null || true)
if [ -n "$packages" ]; then
echo "Installing npm packages: $packages"
npm install -g $packages --prefer-offline --no-audit --no-fund --location=user

# パッケージを一つずつインストール
while IFS= read -r package; do
if [ -n "$package" ]; then
echo "Installing package: $package"
npm install -g "$package" --prefer-offline --no-audit --no-fund
if [ $? -eq 0 ]; then
echo "✓ Successfully installed: $package"
else
echo "✗ Failed to install: $package"
fi
fi
done <<< "$packages"

# インストール結果の確認
Comment on lines +71 to +84

@coderabbitai coderabbitai Bot May 30, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Fail-fast on package installation errors and verify prerequisites
While iterating installs provides per-package logs, failures do not abort the script, which may hide critical issues. Consider:

  1. Exiting immediately on any npm install failure.
  2. Checking that jq is installed before parsing global.json.

Example diff:

+command -v jq >/dev/null 2>&1 || { echo "ERROR: jq is required for parsing global.json"; exit 1; }
 while IFS= read -r package; do
-  npm install -g "$package" --prefer-offline --no-audit --no-fund
+  npm install -g "$package" --prefer-offline --no-audit --no-fund || exit 1
🤖 Prompt for AI Agents 
In .devcontainer/features/common/install.sh around lines 71 to 84, the script
currently continues installing packages even if an npm install fails, which can
hide critical errors. Modify the script to exit immediately if any npm install
command fails by checking the exit status and calling exit on failure.
Additionally, before parsing global.json with jq, add a check to verify that jq
is installed and exit with an error message if it is missing.

echo "Verifying installed packages:"
npm list -g --depth=0 2>/dev/null || true
else
echo "No packages found in global.json dependencies"
fi
Expand Down
5 changes: 5 additions & 0 deletions dot/.zshrc.devcontainer
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,11 @@ fi
# Set proper PATH for devcontainer
export PATH="$HOME/.local/bin:$PATH"

# NPM global packages support
if [[ -d "$HOME/.npm-global/bin" ]]; then
export PATH="$HOME/.npm-global/bin:$PATH"
fi

# Enable color support
export CLICOLOR=1
export LSCOLORS=ExFxBxDxCxegedabagacad
Expand Down