Skip to content
This repository was archived by the owner on May 15, 2025. It is now read-only.

fix(devcontainers-cli): add PNPM_HOME for pnpm package manager #433

Merged
merged 3 commits into from
Apr 18, 2025
Merged

fix(devcontainers-cli): add PNPM_HOME for pnpm package manager #433

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9a0af2c
add PNPM_HOME on pnpm
defelmnq Apr 17, 2025
1adf7aa
format
defelmnq Apr 17, 2025
5e147bb
update script with maf suggestions
defelmnq Apr 18, 2025
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
4 changes: 2 additions & 2 deletions devcontainers-cli/main.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ const executeScriptInContainerWithPackageManager = async (
await execContainer(id, [
shell,
"-c",
"apk add nodejs npm && npm install -g pnpm",
`wget -qO- https://get.pnpm.io/install.sh | ENV="$HOME/.shrc" SHELL="$(which sh)" sh -`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Purely testing purpose, done to not rely on NPM.

]);
} else if (packageManager === "yarn") {
await execContainer(id, [
Expand Down Expand Up @@ -86,7 +86,7 @@ describe("devcontainers-cli", async () => {
expect(output.stdout[output.stdout.length - 1]).toEqual(
"🥳 @devcontainers/cli has been installed into /usr/local/bin/devcontainer!",
);
});
}, 15000);

it("installs devcontainers-cli with yarn", async () => {
const state = await runTerraformApply(import.meta.dir, {
Expand Down
15 changes: 11 additions & 4 deletions devcontainers-cli/run.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,12 @@ if ! command -v docker > /dev/null 2>&1; then
fi

# Determine the package manager to use: npm, pnpm, or yarn
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the ordering to use PNPM as a last option, if NPM is installed too.

if command -v pnpm > /dev/null 2>&1; then
PACKAGE_MANAGER="pnpm"
elif command -v yarn > /dev/null 2>&1; then
if command -v yarn > /dev/null 2>&1; then
PACKAGE_MANAGER="yarn"
elif command -v npm > /dev/null 2>&1; then
PACKAGE_MANAGER="npm"
elif command -v pnpm > /dev/null 2>&1; then
PACKAGE_MANAGER="pnpm"
else
echo "ERROR: No supported package manager (npm, pnpm, yarn) is installed. Please install one first." 1>&2
exit 1
Expand All @@ -26,9 +26,16 @@ fi
echo "Installing @devcontainers/cli using $PACKAGE_MANAGER..."

# Install @devcontainers/cli using the selected package manager
if [ "$PACKAGE_MANAGER" = "npm" ] || [ "$PACKAGE_MANAGER" = "pnpm" ]; then
if [ "$PACKAGE_MANAGER" = "npm" ]; then
$PACKAGE_MANAGER install -g @devcontainers/cli \
&& echo "🥳 @devcontainers/cli has been installed into $(which devcontainer)!"
elif [ "$PACKAGE_MANAGER" = "pnpm" ]; then
# if PNPM_HOME is not set, set it to the bin directory of the script
Copy link
Member

Choose a reason for hiding this comment

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

This comment could better explain why we need to do this, for posterity.

if [ -z "$PNPM_HOME" ]; then
export PNPM_HOME="$CODER_SCRIPT_BIN_DIR"
fi
Copy link
Member

Choose a reason for hiding this comment

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

Just for my education, how does that work? What is CODER_SCRIPT_BIN_DIR?

Copy link
Member

Choose a reason for hiding this comment

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

Typically /tmp/coder-script-data/bin, a place where all scripts can place binaries and they will be part of PATH (guaranteed by agent).

$PACKAGE_MANAGER add -g @devcontainers/cli \
&& echo "🥳 @devcontainers/cli has been installed into $(which devcontainer)!"
elif [ "$PACKAGE_MANAGER" = "yarn" ]; then
$PACKAGE_MANAGER global add @devcontainers/cli \
&& echo "🥳 @devcontainers/cli has been installed into $(which devcontainer)!"
Copy link
Member

Choose a reason for hiding this comment

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

It'd be nice to factor out this repetition, might I suggest the following?

install() {
  if [ "$PACKAGE_MANAGER" = "npm" ]; then
    npm install -g @devcontainers/cli
  elif [ "$PACKAGE_MANAGER" = "pnpm" ]; then
    # <reason we set this>
    if [ -z "$PNPM_HOME" ]; then
      PNPM_HOME="$CODER_SCRIPT_BIN_DIR"
      export PNPM_HOME
    fi
    pnpm add -g @devcontainers/cli
  elif [ "$PACKAGE_MANAGER" = "yarn" ]; then
    yarn global add @devcontainers/cli
  fi
}

if ! install; then
  echo "Failed to install @devcontainers/cli" >&2
  exit 1
fi

if ! command -v devcontainer > /dev/null 2>&1; then
  echo "Installation completed but 'devcontainer' command not found in PATH" >&2
  exit 1
fi

echo "🥳 @devcontainers/cli has been installed into $(which devcontainer)!"
exit 0

Adds a bit more detail and validates the binary is present.

Expand Down