feat: add macOS support to quickstart scripts#220
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughBoth setup and cleanup scripts now support a configurable container name argument (defaulting to ChangesCross-platform container configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
agullon
force-pushed
the
macos-quickstart
branch
4 times, most recently
from
750c382 to
e1d0c51
Compare
|
@agullon , can we please also address the following issues in this PR?
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/quickstart.sh (2)
9-9: 💤 Low valueHeads up:
CONTAINER_NAMEconsumes$1underset -u.With
set -u, the${1:-microshift-okd}default is safe — but note any latershift-based arg parsing would now be off by one. Currently no other positional args, so it's fine as-is. Just leaving a breadcrumb for future args.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/quickstart.sh` at line 9, CONTAINER_NAME is assigned from positional $1 while the script uses set -u which will treat unset positional parameters as errors and any later use of shift would misalign args; to fix, either avoid consuming $1 directly by using a named option parsing or preserve positional parameters (e.g., parse args with getopts or copy $@ to another array) and update any shift logic accordingly, or explicitly document and guard CONTAINER_NAME assignment so future shift-based parsing isn’t off-by-one; refer to the CONTAINER_NAME assignment and any subsequent shift or positional parsing logic to implement one of these approaches.
63-85: 💤 Low valueDarwin reuse path doesn't early-return like the Linux branch.
On Linux (lines 87-91), when
lvm_diskexists you zero it andreturn 0immediately. On Darwin, after zeroing you fall through to thevgscheck and may re-runlosetup/vgcreate. Sinceddof 100MB likely wipes the LVM header,vgsreturns non-zero and a freshvgcreateruns — which is arguably the correct behavior for a "clear and reuse" cycle. Worth deciding whether Linux should match macOS (also recreate VG after zeroing) or vice versa, otherwise reuse semantics diverge subtly between platforms.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/quickstart.sh` around lines 63 - 85, The Darwin branch's heredoc handling of lvm_disk differs from Linux: after zeroing the existing lvm_disk with dd the Linux branch returns immediately (return 0) but the Darwin branch continues to run vgs/losetup/vgcreate, causing divergent reuse semantics; pick one behavior and make both branches consistent—either (A) match Linux by causing the Darwin heredoc to exit early after the dd (so the remote ssh block stops and the script returns 0) or (B) match Darwin by removing the immediate return in the Linux branch so both platforms proceed to vgs/losetup/vgcreate after zeroing; update the code around lvm_disk handling (the dd/truncate logic) and the flow control (return/exit) so lvm_disk and vg_name reuse is identical across platforms.src/quickclean.sh (1)
10-13: ⚡ Quick winRoot enforcement is inconsistent with
quickstart.shon macOS.
quickstart.sh(lines 169-195) lets Darwin run without root and only requires root on Linux. Here, root is required for every platform, so a user who quickstarted on macOS withoutsudocan't quickclean withoutsudo. Suggest mirroring the same Darwin/Linux split:♻️ Suggested mirror of quickstart's platform check
-# Check if the script is running as root -if [ "$(id -u)" -ne 0 ]; then - echo "ERROR: This script must be run as root (use sudo)" - exit 1 -fi +# Linux requires root; macOS uses the user's podman machine. +if [[ "$(uname -s)" != "Darwin" ]] && [ "$(id -u)" -ne 0 ]; then + echo "ERROR: This script must be run as root (use sudo)" + exit 1 +fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/quickclean.sh` around lines 10 - 13, The root-enforcement in quickclean.sh currently uses the id -u check unconditionally and should match quickstart.sh's platform-specific logic; change the check to only require root on non-Darwin platforms by detecting the OS via uname (e.g., test "$(uname -s)" = "Darwin") and skip the id -u root check when on Darwin, otherwise keep the existing id -u check and error message unchanged; update the conditional around the id -u test in quickclean.sh accordingly so macOS users who ran quickstart without sudo can run quickclean without sudo while Linux still requires root.README.md (1)
32-33: ⚡ Quick winMinimal macOS mention — consider a brief setup snippet.
The "including macOS" tag-on satisfies the literal ask, but new macOS users hitting this README still won't know they need a rootful Podman machine. Optional: add a short note (or link to a doc) with the four prerequisite commands from the PR description (
brew install podman,podman machine init --memory 4096,podman machine set --rootful,podman machine start) so the Bootc quickstart actually works on macOS out of the box.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 32 - 33, The README currently only mentions "including macOS" but lacks setup steps; add a short macOS prerequisite snippet near the sentence that mentions macOS (the paragraph starting "MicroShift Bootc container images can be run on `x86_64` and `aarch64`... including macOS") that shows the four required Podman commands (brew install podman, podman machine init --memory 4096, podman machine set --rootful, podman machine start) or a link to a macOS-specific doc, so macOS users know they must create a rootful Podman machine before following the quickstart.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/quickstart.sh`:
- Around line 70-85: The unquoted variable expansion ${machine_ssh} in
quickstart.sh triggers shellcheck SC2086 because it intentionally relies on
word-splitting; fix it either by adding a shellcheck suppression comment
immediately before the expansion (# shellcheck disable=SC2086) or by changing
machine_ssh to an array and invoking it as "${machine_ssh[@]}" where used;
locate the use of machine_ssh in the SSH command and apply one of these two
fixes and ensure any code that constructs machine_ssh is adjusted to produce an
array if you choose the array approach.
---
Nitpick comments:
In `@README.md`:
- Around line 32-33: The README currently only mentions "including macOS" but
lacks setup steps; add a short macOS prerequisite snippet near the sentence that
mentions macOS (the paragraph starting "MicroShift Bootc container images can be
run on `x86_64` and `aarch64`... including macOS") that shows the four required
Podman commands (brew install podman, podman machine init --memory 4096, podman
machine set --rootful, podman machine start) or a link to a macOS-specific doc,
so macOS users know they must create a rootful Podman machine before following
the quickstart.
In `@src/quickclean.sh`:
- Around line 10-13: The root-enforcement in quickclean.sh currently uses the id
-u check unconditionally and should match quickstart.sh's platform-specific
logic; change the check to only require root on non-Darwin platforms by
detecting the OS via uname (e.g., test "$(uname -s)" = "Darwin") and skip the id
-u root check when on Darwin, otherwise keep the existing id -u check and error
message unchanged; update the conditional around the id -u test in quickclean.sh
accordingly so macOS users who ran quickstart without sudo can run quickclean
without sudo while Linux still requires root.
In `@src/quickstart.sh`:
- Line 9: CONTAINER_NAME is assigned from positional $1 while the script uses
set -u which will treat unset positional parameters as errors and any later use
of shift would misalign args; to fix, either avoid consuming $1 directly by
using a named option parsing or preserve positional parameters (e.g., parse args
with getopts or copy $@ to another array) and update any shift logic
accordingly, or explicitly document and guard CONTAINER_NAME assignment so
future shift-based parsing isn’t off-by-one; refer to the CONTAINER_NAME
assignment and any subsequent shift or positional parsing logic to implement one
of these approaches.
- Around line 63-85: The Darwin branch's heredoc handling of lvm_disk differs
from Linux: after zeroing the existing lvm_disk with dd the Linux branch returns
immediately (return 0) but the Darwin branch continues to run
vgs/losetup/vgcreate, causing divergent reuse semantics; pick one behavior and
make both branches consistent—either (A) match Linux by causing the Darwin
heredoc to exit early after the dd (so the remote ssh block stops and the script
returns 0) or (B) match Darwin by removing the immediate return in the Linux
branch so both platforms proceed to vgs/losetup/vgcreate after zeroing; update
the code around lvm_disk handling (the dd/truncate logic) and the flow control
(return/exit) so lvm_disk and vg_name reuse is identical across platforms.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b39a70c8-cdad-477a-aa7f-c54ffcd8d98b
📒 Files selected for processing (3)
README.mdsrc/quickclean.shsrc/quickstart.sh
quickstart.sh and quickclean.sh now detect macOS and handle podman machine validation (rootful mode required) and LVM setup via `podman machine ssh`. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
Usage: ./src/quickstart.sh [name] ./src/quickclean.sh [name] Defaults to 'microshift-okd' when no argument is provided. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> pre-commit.check-secrets: ENABLED
|
|
||
| # Platform-specific initialization | ||
| if [[ "$(uname -s)" == "Darwin" ]]; then | ||
| if ! sudo -u "${SUDO_USER}" podman info &>/dev/null </dev/null; then |
| exit 1 | ||
| fi | ||
|
|
||
| local_rootful="$(sudo -u "${SUDO_USER}" podman machine inspect --format '{{.Rootful}}' 2>/dev/null || echo "false")" |
| IMAGE=${IMAGE:-"ghcr.io/${OWNER}/${REPO}"} | ||
| TAG=${TAG:-latest} | ||
|
|
||
| CONTAINER_NAME="${1:-microshift-okd}" |
There was a problem hiding this comment.
| CONTAINER_NAME="${1:-microshift-okd}" | |
| CONTAINER_NAME="${CONTAINER_NAME:-microshift-okd}" |
| #!/bin/bash | ||
| set -euo pipefail | ||
|
|
||
| CONTAINER_NAME="${1:-microshift-okd}" |
There was a problem hiding this comment.
| CONTAINER_NAME="${1:-microshift-okd}" | |
| CONTAINER_NAME="${CONTAINER_NAME:-microshift-okd}" |
| echo | ||
| echo "To uninstall MicroShift, run the following command:" | ||
| echo " - curl -s https://${OWNER}.github.io/${REPO}/quickclean.sh | sudo bash" | ||
| echo " - curl -s https://${OWNER}.github.io/${REPO}/quickclean.sh | sudo bash -s ${CONTAINER_NAME}" |
There was a problem hiding this comment.
| echo " - curl -s https://${OWNER}.github.io/${REPO}/quickclean.sh | sudo bash -s ${CONTAINER_NAME}" | |
| echo " - curl -s https://${OWNER}.github.io/${REPO}/quickclean.sh | sudo CONTAINER_NAME=${CONTAINER_NAME} bash" |
2 participants
Adds macOS support to
quickstart.shandquickclean.sh. The same scripts now work on both Linux and macOS.On macOS, the scripts require a rootful podman machine:
Changes:
quickstart.sh: detect macOS, validate rootful podman machine, run LVM setup inside the VM viapodman machine sshquickclean.sh: detect macOS, clean up LVM inside the VM viapodman machine ssh./src/quickstart.sh [name]🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation