fix: Added script to install patternfly-cli.#22
Merged
dlabaj merged 7 commits intopatternfly:mainfrom Apr 2, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds standalone install/uninstall shell scripts to provision Node (via nvm), enable Corepack, conditionally install GitHub CLI, and install/uninstall the scoped Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (shell)
participant Install as scripts/install.sh
participant NVM as nvm
participant Node as Node.js
participant Corepack as Corepack
participant GH as GitHub CLI
participant NPM as npm
User->>Install: run ./scripts/install.sh
Install->>Install: check `node`/`npm`
alt node missing
Install->>NVM: download & source nvm (curl/wget)
Install->>NVM: nvm install 24
Install->>NVM: nvm alias default 24
Install->>Node: verify node/npm available
end
Install->>Corepack: corepack enable
Install->>Install: detect OS/distro
alt gh missing
Install->>GH: install via Homebrew/apt/dnf/pacman/zypper/apk or vendor repo
end
Install->>NPM: npm install -g `@patternfly/patternfly-cli`
NPM-->>Install: success/failure
Install->>User: print success and usage hints
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/install.sh`:
- Line 92: The installer currently calls require_cmd sudo || error "sudo is
required..." which aborts even when running as root; modify the checks around
require_cmd sudo (used at the instances referencing apt invocations) to first
detect effective UID 0 (EUID or UID) and skip requiring sudo when true — i.e.,
only call require_cmd sudo or error if EUID != 0; update the occurrences where
sudo is enforced (the require_cmd sudo checks near the apt install branches) so
the script proceeds in root environments without sudo while preserving the
existing error for non-root/non-sudo cases.
- Line 126: Replace the pacman invocation that currently uses "sudo pacman -Sy
--noconfirm github-cli" with a full system sync and upgrade invocation to avoid
a partial upgrade; specifically change the "-Sy" usage to "-Syu" while keeping
the existing flags so the command becomes a full database refresh and system
upgrade before installing github-cli.
- Around line 62-67: The current ensure_node() only checks for node and returns
early, which lets hosts with missing npm or corepack skip nvm bootstrap and
later break enable_corepack_step() and install_patternfly_cli(); update
ensure_node() so it verifies all three commands (node, npm, corepack) via
require_cmd (or equivalent) and only returns success if all are present,
otherwise call install_node_via_nvm to provision the full toolchain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
🧹 Nitpick comments (1)
README.md (1)
13-29: Consider documenting shell compatibility and idempotency.The install script section is clear and accurate, but could be enhanced with:
- A note about which shells are supported (e.g., "Requires bash or a compatible shell")
- Confirmation that the script is safe to re-run if it's idempotent
These additions would improve the user experience for troubleshooting and re-running the script.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 13 - 29, Update the "Install script (macOS and Linux)" section to state shell compatibility and idempotency: add a short sentence after the header saying the script requires bash (or a POSIX-compatible shell) to run and will work when piped via curl to bash, and add a second sentence confirming whether the install script (scripts/install.sh / install-patternfly-cli.sh) is safe to re-run (idempotent) and what state it leaves the system in (e.g., will skip installing Node/npm/GitHub CLI if already present) plus a note that it may prompt for sudo when installing system packages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@README.md`:
- Around line 13-29: Update the "Install script (macOS and Linux)" section to
state shell compatibility and idempotency: add a short sentence after the header
saying the script requires bash (or a POSIX-compatible shell) to run and will
work when piped via curl to bash, and add a second sentence confirming whether
the install script (scripts/install.sh / install-patternfly-cli.sh) is safe to
re-run (idempotent) and what state it leaves the system in (e.g., will skip
installing Node/npm/GitHub CLI if already present) plus a note that it may
prompt for sudo when installing system packages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ad6599a9-571b-40a4-9384-a550e1236f49
📒 Files selected for processing (4)
README.mdsrc/__tests__/doctor.test.tssrc/cli.tssrc/doctor.ts
💤 Files with no reviewable changes (3)
- src/cli.ts
- src/doctor.ts
- src/tests/doctor.test.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/__tests__/read-package-version.test.ts (1)
7-7: Avoidprocess.cwd()in this test; resolve fromimport.meta.urlinstead.This test is brittle when run from a non-repo-root working directory.
Proposed fix
-import path from 'path'; +import { fileURLToPath } from 'node:url'; import fs from 'fs-extra'; import { readPackageVersion } from '../read-package-version.js'; @@ - const pkgPath = path.join(process.cwd(), 'package.json'); + const pkgPath = fileURLToPath(new URL('../../package.json', import.meta.url));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/read-package-version.test.ts` at line 7, The test uses process.cwd() to build pkgPath which is brittle; instead derive the repo root from import.meta.url: replace the process.cwd() usage for pkgPath with a path resolved from import.meta.url (e.g. use fileURLToPath(import.meta.url) and path.dirname or new URL(import.meta.url) to locate the package.json next to the test file), updating the symbol pkgPath so it no longer depends on process.cwd() and import the helper fileURLToPath from 'url' if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli.ts`:
- Around line 79-82: Add the new CLI command "cli-upgrade" to the README's
"Available Commands" section so it's discoverable: update the README command
list to include an entry for the cli-upgrade command (matching the description
from src/cli.ts: "Upgrade patternfly-cli to the latest version (npm global
install)"), show the usage/command name "patternfly-cli cli-upgrade" (or the
repo's CLI invocation) and a one-line description; place it alongside the other
commands and ensure any alphabetical or grouped ordering is preserved.
- Around line 17-18: The module currently calls readPackageVersion at import
time (packageJsonPath and packageVersion) which can throw and crash startup;
change this to a safe, lazy/guarded load: wrap the readPackageVersion call in a
try/catch or move it into a getPackageVersion() function that reads
packageJsonPath on first use, returning a sensible default like "0.0.0" or
"unknown" on error and logging the error instead of letting it propagate; ensure
all references use the new safe getter so startup never throws from version
loading.
In `@src/read-package-version.ts`:
- Around line 3-4: The readPackageVersion function currently casts
fs.readJsonSync(packageJsonPath) to { version: string } and returns .version
directly, which can be undefined or empty; update readPackageVersion to call
fs.readJsonSync(packageJsonPath), extract the version value, validate that it's
a non-empty string, and throw a clear Error (including packageJsonPath) if the
version is missing or invalid so failures are explicit and easier to diagnose;
reference the readPackageVersion function and the fs.readJsonSync call when
applying this guard.
---
Nitpick comments:
In `@src/__tests__/read-package-version.test.ts`:
- Line 7: The test uses process.cwd() to build pkgPath which is brittle; instead
derive the repo root from import.meta.url: replace the process.cwd() usage for
pkgPath with a path resolved from import.meta.url (e.g. use
fileURLToPath(import.meta.url) and path.dirname or new URL(import.meta.url) to
locate the package.json next to the test file), updating the symbol pkgPath so
it no longer depends on process.cwd() and import the helper fileURLToPath from
'url' if needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6a3f3708-3eb2-4168-85de-9baed3872532
📒 Files selected for processing (5)
README.mdscripts/uninstall.shsrc/__tests__/read-package-version.test.tssrc/cli.tssrc/read-package-version.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/__tests__/read-package-version.test.ts (1)
14-19: Make spy cleanup failure-safe to avoid test pollution.If an assertion throws before Line 18,
console.logstays mocked and can affect subsequent tests. Wrap restore infinally(or move toafterEach).♻️ Proposed fix
it("returns 'unknown' when the file does not exist", () => { const log = jest.spyOn(console, 'log').mockImplementation(() => {}); const missing = path.join(process.cwd(), 'nonexistent-package-xyz.json'); - expect(readPackageVersion(missing)).toBe('unknown'); - log.mockRestore(); + try { + expect(readPackageVersion(missing)).toBe('unknown'); + } finally { + log.mockRestore(); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/read-package-version.test.ts` around lines 14 - 19, The test currently spies on console.log with jest.spyOn in the "returns 'unknown' when the file does not exist" case but calls log.mockRestore() only at the end, which can leave the spy active if an assertion throws; update the test to guarantee restoration by moving the mock restore into a finally block around the expect (or move the spy/restore into a shared afterEach), i.e., ensure the jest.spyOn(console, 'log') created in this test is always restored even on failures so console.log is not left mocked for other tests; refer to the spy variable "log" and the function under test "readPackageVersion" when making the change.scripts/install.sh (2)
220-223: Consider checking if the package is already installed.The function always attempts to install
@patternfly/patternfly-cliglobally. While npm handles reinstalls gracefully, checking first withnpm list -g@patternfly/patternfly-cli`` could provide better user feedback and avoid unnecessary network calls.♻️ Optional check before install
install_patternfly_cli() { + if npm list -g `@patternfly/patternfly-cli` >/dev/null 2>&1; then + info "@patternfly/patternfly-cli is already installed globally." + return 0 + fi info "Installing `@patternfly/patternfly-cli` globally from npm." npm install -g `@patternfly/patternfly-cli` || error "npm failed to install `@patternfly/patternfly-cli` globally. Check permissions and your network." }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install.sh` around lines 220 - 223, Update install_patternfly_cli to first check whether `@patternfly/patternfly-cli` is already installed globally (e.g., using npm list -g `@patternfly/patternfly-cli` or npm ls -g --depth=0) and only run npm install -g if it is not present; keep the existing info/error logging but add a log path for "already installed" so the function skips the network install when the package is found and falls back to the install and existing error message when it is not.
7-7: Consider updating to nvm v0.40.4.
v0.40.3is a valid, stable release (released April 2025), butv0.40.4has been available since January 2026. Updating to the more recent version would ensure better compatibility with recent node version management improvements without requiring changes to the install logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install.sh` at line 7, Update the default NVM_VERSION value from "v0.40.3" to "v0.40.4" by changing the NVM_VERSION assignment (NVM_VERSION="${NVM_VERSION:-v0.40.3}") so the script uses the newer stable nvm release; ensure the default token remains overridable via environment variable but the literal version string is updated to v0.40.4.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/install.sh`:
- Line 240: The printf success message currently uses single quotes so $HOME and
$NVM_DIR are not expanded; update the call that prints the export/loader line
(the printf that contains export NVM_DIR="$HOME/.nvm" && [ -s "$NVM_DIR/nvm.sh"
] && . "$NVM_DIR/nvm.sh") to use double quotes for the format string and escape
the inner quotes and dollar signs (or otherwise escape $HOME and $NVM_DIR) so
the actual paths are shown to users instead of the literal $HOME/$NVM_DIR.
---
Nitpick comments:
In `@scripts/install.sh`:
- Around line 220-223: Update install_patternfly_cli to first check whether
`@patternfly/patternfly-cli` is already installed globally (e.g., using npm list
-g `@patternfly/patternfly-cli` or npm ls -g --depth=0) and only run npm install
-g if it is not present; keep the existing info/error logging but add a log path
for "already installed" so the function skips the network install when the
package is found and falls back to the install and existing error message when
it is not.
- Line 7: Update the default NVM_VERSION value from "v0.40.3" to "v0.40.4" by
changing the NVM_VERSION assignment (NVM_VERSION="${NVM_VERSION:-v0.40.3}") so
the script uses the newer stable nvm release; ensure the default token remains
overridable via environment variable but the literal version string is updated
to v0.40.4.
In `@src/__tests__/read-package-version.test.ts`:
- Around line 14-19: The test currently spies on console.log with jest.spyOn in
the "returns 'unknown' when the file does not exist" case but calls
log.mockRestore() only at the end, which can leave the spy active if an
assertion throws; update the test to guarantee restoration by moving the mock
restore into a finally block around the expect (or move the spy/restore into a
shared afterEach), i.e., ensure the jest.spyOn(console, 'log') created in this
test is always restored even on failures so console.log is not left mocked for
other tests; refer to the spy variable "log" and the function under test
"readPackageVersion" when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 49f7a8bf-8ac2-4718-8398-a14240f90f9f
📒 Files selected for processing (4)
README.mdscripts/install.shsrc/__tests__/read-package-version.test.tssrc/read-package-version.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/read-package-version.ts
- README.md
There was a problem hiding this comment.
♻️ Duplicate comments (1)
scripts/install.sh (1)
72-78:⚠️ Potential issue | 🟠 MajorEnsure full Node toolchain before early return.
At Line 73,
ensure_node()only checksnode. Ifnpmorcorepackis missing, the script skips nvm bootstrap and fails later inenable_corepack_step()/ global install. Gate early return on all required commands.Proposed fix
ensure_node() { - if require_cmd node; then - info "Node.js is already installed: $(command -v node) ($(node --version))" + if require_cmd node && require_cmd npm && require_cmd corepack; then + info "Node.js toolchain is already installed: $(command -v node) ($(node --version))" return 0 fi install_node_via_nvm }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install.sh` around lines 72 - 78, The early-return in ensure_node() only checks for node and can skip bootstrapping nvm even when npm or corepack are missing; change ensure_node() to verify all required tools (node, npm, corepack) before returning and only call install_node_via_nvm when any of them are absent. Update the logic in ensure_node() to use require_cmd (or equivalent checks) for node, npm, and corepack together, and ensure subsequent steps like enable_corepack_step() and global installs rely on the now-guaranteed toolchain; keep install_node_via_nvm as the fallback branch when any check fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@scripts/install.sh`:
- Around line 72-78: The early-return in ensure_node() only checks for node and
can skip bootstrapping nvm even when npm or corepack are missing; change
ensure_node() to verify all required tools (node, npm, corepack) before
returning and only call install_node_via_nvm when any of them are absent. Update
the logic in ensure_node() to use require_cmd (or equivalent checks) for node,
npm, and corepack together, and ensure subsequent steps like
enable_corepack_step() and global installs rely on the now-guaranteed toolchain;
keep install_node_via_nvm as the fallback branch when any check fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4b4b9d57-52e1-45dc-83a5-909194ef6216
📒 Files selected for processing (2)
scripts/install.shsrc/__tests__/read-package-version.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/tests/read-package-version.test.ts
|
🎉 This PR is included in version 1.2.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes issue #21 . Currently there is no way to check if corepack is enabled. A better approach is to use an install script and install everything including the cli that way. This is a more predictable and maintainable approach then the previous way of doing an install.
Summary by CodeRabbit
New Features
cli-upgradecommand to update the globally installed CLI.Improvements
Removals
doctorcommand and its tests.Tests