feat: improve structure of setup guides, add disk format script#4
Open
feat: improve structure of setup guides, add disk format script#4
Conversation
📝 WalkthroughWalkthroughReorganizes Kubernetes docs into a three-step guide (node setup, storage replication, chart installation), removes legacy node/installation files, adds detailed new guides, updates README with a Guides and Quick Install section, renames a prepare script reference, and adds a drive-formatting script for node preparation. Changes
Sequence Diagram(s)(Skipped — changes do not introduce new multi-component sequential control flow requiring visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 8
🧹 Nitpick comments (2)
kubernetes/3_install_flame_hub_chart.md (1)
57-57: Use a valid Helm placeholder format for release name.Line 57 uses
<release name>with a space;<release-name>is less error-prone for copy/paste.Suggested fix
-helm install <release name> -f <my-custom-values-file> . +helm install <release-name> -f <my-custom-values-file> .🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kubernetes/3_install_flame_hub_chart.md` at line 57, The Helm command example uses an invalid placeholder " <release name> " (with a space); update the example in the helm install line (the snippet "helm install <release name> -f <my-custom-values-file> .") to use a valid placeholder such as "<release-name>" (or a conventional uppercase token like "<RELEASE_NAME>") so it can be copy/pasted without error.kubernetes/1_setup_kubernetes_nodes.md (1)
42-42: Clarify how PARTUUID output should be used in Mayastor config.Line 42 references Part 2, but adding one concrete example here would reduce setup ambiguity.
scripts/0_format_drives.sh(Lines 96-101) emits/dev/disk/by-partuuid/...; showing where that exact value goes in the Mayastor disk-pool config would help.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kubernetes/1_setup_kubernetes_nodes.md` at line 42, Add a brief concrete example showing that the PartUUID emitted by scripts/0_format_drives.sh (the /dev/disk/by-partuuid/<UUID> path printed in lines ~96-101) should be used as the device/path value in the Mayastor disk-pool configuration referenced in Part 2 (2_setup_storage_replication.md); update the sentence at line 42 to include an inline example like “use /dev/disk/by-partuuid/<PARTUUID> as the disk-pool device/path in the Mayastor disk-pool manifest” so readers know to paste the exact PartUUID string into the disk-pool device/path field.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@kubernetes/1_setup_kubernetes_nodes.md`:
- Around line 32-35: The fenced code blocks in the markdown are missing language
identifiers (e.g., the triple-backtick fences around the shell commands such as
the block containing "bash scripts/0_format_drives.sh <device>" and the other
similar command snippets); update each code fence to include an appropriate
language tag (for shells use ```bash, for YAML use ```yaml, etc.) so all
command/code blocks include a language identifier and satisfy MD040 linting.
- Line 7: The TOC anchor for "Prepare Drives" is broken because the heading is
actually "Prepare Drives (optional)" which GitHub converts to
"#prepare-drives-optional"; update the TOC entry that currently links to
"#prepare-drives" to instead link to "#prepare-drives-optional" or alternatively
rename the heading "Prepare Drives (optional)" to exactly "Prepare Drives" so
the existing "#prepare-drives" anchor works—look for the TOC entry and the
heading text "Prepare Drives (optional)" to apply the change.
In `@kubernetes/2_setup_storage_replication.md`:
- Line 65: The docs currently reference charts/flame-hub/values.yaml as a repo
path that isn't directly navigable; update the sentence to use a resolvable URL
pointing to the values.yaml file in the repository (e.g., the raw or blob link
on the hosting platform) so readers can open it immediately, keeping the mention
of the mayastor-replicated storage class and the hint to "search for mayastor"
intact.
In `@kubernetes/3_install_flame_hub_chart.md`:
- Line 16: Fix the typos and make the OpenEBS reference a clickable URL in the
sentence that currently starts "The chart will use whichever storage class is
the default in your cluster...": correct "persistant" to "persistent" and
"accross" to "across", and replace the path-like reference
"/charts/third-party/openebs" with a Markdown link to the official OpenEBS
documentation or repo (e.g., https://openebs.io or the OpenEBS GitHub) so the
sentence reads clearly and includes a direct clickable reference.
In `@scripts/0_format_drives.sh`:
- Around line 38-43: Before running destructive commands, add an explicit
confirmation gate: prompt the user to type the exact device path stored in the
DISK variable (or a literal "YES" plus the device) and verify the input matches
DISK; if it does not match, abort with a clear message. Apply this check
immediately before the sfdisk invocation that creates the GPT label and
partitions (reference DISK and the sfdisk here) and also before the later
formatting/repartitioning commands (the second destructive block around
mkfs/sfdisk), so the script requires an explicit, exact confirmation to proceed.
- Line 2: The usage comment at the top of the script references the wrong
filename; replace the stale reference "01_format_drives.sh" with the correct
script name "0_format_drives.sh" in the comment line (the line containing '# may
need to be run with "01_format_drives.sh"') so the usage message matches the
actual file name.
- Line 84: The script unconditionally appends "PARTUUID=${PARTUUID}
${MOUNTPOINT} ext4 defaults,nofail 0 2" which creates duplicate /etc/fstab
entries on reruns; modify the logic around the echo/tee call so it first checks
for an existing entry (grep -q for the PARTUUID or MOUNTPOINT) and only appends
if not present, or updates the existing line (sed replacement) when the PARTUUID
or MOUNTPOINT matches; target the block that writes to /etc/fstab (the echo ...
| sudo tee -a /etc/fstab line) and use a guard/replace approach to prevent
duplicates.
- Around line 22-25: The script builds partition paths as "${DISK}${i}" which
breaks for device names that end with a digit (e.g., /dev/nvme0n1,
/dev/mmcblk0); add a small helper (e.g., partition_for or make_partition) that
takes disk and index and returns "${DISK}p${i}" when DISK ends with a digit and
"${DISK}${i}" otherwise (use a shell test like [[ "$DISK" =~ [0-9]$ ]] to detect
this). Replace all direct constructions of PART="${DISK}${i}" (and any other
concatenations at the sites referenced) with calls to this helper so partition
paths are correct for both NVMe/MMC-style and traditional devices.
---
Nitpick comments:
In `@kubernetes/1_setup_kubernetes_nodes.md`:
- Line 42: Add a brief concrete example showing that the PartUUID emitted by
scripts/0_format_drives.sh (the /dev/disk/by-partuuid/<UUID> path printed in
lines ~96-101) should be used as the device/path value in the Mayastor disk-pool
configuration referenced in Part 2 (2_setup_storage_replication.md); update the
sentence at line 42 to include an inline example like “use
/dev/disk/by-partuuid/<PARTUUID> as the disk-pool device/path in the Mayastor
disk-pool manifest” so readers know to paste the exact PartUUID string into the
disk-pool device/path field.
In `@kubernetes/3_install_flame_hub_chart.md`:
- Line 57: The Helm command example uses an invalid placeholder " <release name>
" (with a space); update the example in the helm install line (the snippet "helm
install <release name> -f <my-custom-values-file> .") to use a valid placeholder
such as "<release-name>" (or a conventional uppercase token like
"<RELEASE_NAME>") so it can be copy/pasted without error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 936dfc19-53e0-4bcd-8eca-e28ba700c5de
📒 Files selected for processing (9)
README.mdkubernetes/1_setup_kubernetes_nodes.mdkubernetes/2_setup_storage_replication.mdkubernetes/3_install_flame_hub_chart.mdkubernetes/kubernetes-instructions.mdkubernetes/node-setup.mdscripts/0_format_drives.shscripts/1_microk8s_setup.shscripts/2_prepare_for_mayastor.sh
💤 Files with no reviewable changes (2)
- kubernetes/kubernetes-instructions.md
- kubernetes/node-setup.md
Comment on lines
+32
to
+35
| ``` | ||
| bash scripts/0_format_drives.sh <device> | ||
| # e.g. bash scripts/0_format_drives.sh vdb | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced code blocks.
These command blocks are missing fenced languages (MD040). This can fail markdown linting and hurts readability.
Suggested pattern (apply similarly to all shell command fences)
-```
+```bash
bash scripts/0_format_drives.sh <device>
# e.g. bash scripts/0_format_drives.sh vdb</details>
Also applies to: 52-54, 72-77, 85-88, 92-94, 98-100, 104-106, 110-112, 114-116, 120-122, 124-126, 136-138, 146-148, 160-162, 164-168
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.21.0)</summary>
[warning] 32-32: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @kubernetes/1_setup_kubernetes_nodes.md around lines 32 - 35, The fenced code
blocks in the markdown are missing language identifiers (e.g., the
triple-backtick fences around the shell commands such as the block containing
"bash scripts/0_format_drives.sh " and the other similar command
snippets); update each code fence to include an appropriate language tag (for
shells use bash, for YAML use yaml, etc.) so all command/code blocks
include a language identifier and satisfy MD040 linting.
</details>
<!-- fingerprinting:phantom:medusa:hawk -->
<!-- This is an auto-generated comment by CodeRabbit -->
| ``` | ||
| 8. Edit your FLAME-Hub values-file to use the new StorageClass. A reinstall of the Hub will probably be required. No newline at end of file | ||
| 4. Follow the installation instructions of the [chart's README](https://github.com/PrivateAIM/helm/tree/master/charts/third-party/openebs#readme) | ||
| 5. For the following installation of the FLAME Hub Helm chart, remember to configure the `mayastor-replicated` storage class for stateful services. See example comments in `charts/flame-hub/values.yaml` (search for mayastor) No newline at end of file |
There was a problem hiding this comment.
Use a resolvable link for the values.yaml reference.
Line 65 points to charts/flame-hub/values.yaml, which is not a directly navigable path in this repo. Use a direct URL so readers can open it immediately.
Suggested fix
-5. For the following installation of the FLAME Hub Helm chart, remember to configure the `mayastor-replicated` storage class for stateful services. See example comments in `charts/flame-hub/values.yaml` (search for mayastor)
+5. For the following installation of the FLAME Hub Helm chart, remember to configure the `mayastor-replicated` storage class for stateful services. See example comments in [`values.yaml`](https://github.com/PrivateAIM/helm/blob/master/charts/flame-hub/values.yaml) (search for mayastor).📝 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
| 5. For the following installation of the FLAME Hub Helm chart, remember to configure the `mayastor-replicated` storage class for stateful services. See example comments in `charts/flame-hub/values.yaml` (search for mayastor) | |
| 5. For the following installation of the FLAME Hub Helm chart, remember to configure the `mayastor-replicated` storage class for stateful services. See example comments in [`values.yaml`](https://github.com/PrivateAIM/helm/blob/master/charts/flame-hub/values.yaml) (search for mayastor). |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@kubernetes/2_setup_storage_replication.md` at line 65, The docs currently
reference charts/flame-hub/values.yaml as a repo path that isn't directly
navigable; update the sentence to use a resolvable URL pointing to the
values.yaml file in the repository (e.g., the raw or blob link on the hosting
platform) so readers can open it immediately, keeping the mention of the
mayastor-replicated storage class and the hint to "search for mayastor" intact.
| * Two domain names (one for the Hub, one for Harbor). If you run the Hub on your local machine, you can choose them freely and set them in your hosts file (`/etc/hosts`) | ||
|
|
||
| ## Storage | ||
| The chart will use whichever storage class is the default in your cluster, unless you specify otherwise in the values. See `/charts/third-party/openebs` for instructions how to setup Mayastor Storage Replication. This requires 3+ nodes in your cluster and will replicate persistant volumes accross them. |
There was a problem hiding this comment.
Fix storage sentence typos and make the OpenEBS reference clickable.
Line 16 has spelling errors and a path-like reference that is hard to follow. Use a direct URL and clean wording.
Suggested fix
-The chart will use whichever storage class is the default in your cluster, unless you specify otherwise in the values. See `/charts/third-party/openebs` for instructions how to setup Mayastor Storage Replication. This requires 3+ nodes in your cluster and will replicate persistant volumes accross them.
+The chart uses the cluster’s default storage class unless you override it in values. See the [OpenEBS wrapper chart instructions](https://github.com/PrivateAIM/helm/tree/master/charts/third-party/openebs) for setting up Mayastor storage replication. This requires 3+ nodes and replicates persistent volumes across them.📝 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
| The chart will use whichever storage class is the default in your cluster, unless you specify otherwise in the values. See `/charts/third-party/openebs` for instructions how to setup Mayastor Storage Replication. This requires 3+ nodes in your cluster and will replicate persistant volumes accross them. | |
| The chart uses the cluster's default storage class unless you override it in values. See the [OpenEBS wrapper chart instructions](https://github.com/PrivateAIM/helm/tree/master/charts/third-party/openebs) for setting up Mayastor storage replication. This requires 3+ nodes and replicates persistent volumes across them. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@kubernetes/3_install_flame_hub_chart.md` at line 16, Fix the typos and make
the OpenEBS reference a clickable URL in the sentence that currently starts "The
chart will use whichever storage class is the default in your cluster...":
correct "persistant" to "persistent" and "accross" to "across", and replace the
path-like reference "/charts/third-party/openebs" with a Markdown link to the
official OpenEBS documentation or repo (e.g., https://openebs.io or the OpenEBS
GitHub) so the sentence reads clearly and includes a direct clickable reference.
Comment on lines
+22
to
+25
| if [ -b "${DISK}1" ] && [ -b "${DISK}2" ]; then | ||
| echo "Existing partitions found on $DISK." | ||
| for i in 1 2; do | ||
| PART="${DISK}${i}" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's check the file exists and its line count
wc -l scripts/0_format_drives.sh
# Run the suggested verification command from the review
rg -n '\$\{DISK\}\$\{i\}|\$\{DISK\}1|\$\{DISK\}2|nvme|part_path' scripts/0_format_drives.shRepository: PrivateAIM/hub-deployment
Length of output: 232
🏁 Script executed:
#!/bin/bash
# Read the specific lines mentioned (22-25, 47, 98)
echo "=== Lines 20-30 (context for 22-25) ==="
sed -n '20,30p' scripts/0_format_drives.sh | cat -n
echo ""
echo "=== Lines 45-50 (context for 47) ==="
sed -n '45,50p' scripts/0_format_drives.sh | cat -n
echo ""
echo "=== Lines 96-100 (context for 98) ==="
sed -n '96,100p' scripts/0_format_drives.sh | cat -nRepository: PrivateAIM/hub-deployment
Length of output: 1115
🏁 Script executed:
#!/bin/bash
# Check if DISK variable is used in partition paths in other ways
rg -n 'PART.*DISK|DISK.*PART' scripts/0_format_drives.sh | head -20Repository: PrivateAIM/hub-deployment
Length of output: 150
🏁 Script executed:
#!/bin/bash
# Check DISK variable assignment and script context
head -25 scripts/0_format_drives.sh | cat -nRepository: PrivateAIM/hub-deployment
Length of output: 708
🏁 Script executed:
#!/bin/bash
# Look for DISK variable initialization and any comments about device types
rg -n 'DISK=' scripts/0_format_drives.sh | head -10Repository: PrivateAIM/hub-deployment
Length of output: 93
🏁 Script executed:
#!/bin/bash
# Check shebang and initial comments/documentation
head -5 scripts/0_format_drives.shRepository: PrivateAIM/hub-deployment
Length of output: 153
Partition path construction fails for NVMe/MMC-style device names.
Lines 22, 25, 47, and 98 assume partitions are ${DISK}1/${DISK}2. For disks ending in digits (e.g., /dev/nvme0n1, /dev/mmcblk0), partition paths require a p separator, producing /dev/nvme0n1p1 and /dev/mmcblk0p1 instead of /dev/nvme0n11 and /dev/mmcblk01.
A helper function to conditionally add the p separator based on whether the disk name ends with a digit is needed:
Suggested fix
+part_path() {
+ local disk="$1"
+ local idx="$2"
+ if [[ "$disk" =~ [0-9]$ ]]; then
+ echo "${disk}p${idx}"
+ else
+ echo "${disk}${idx}"
+ fi
+}
-if [ -b "${DISK}1" ] && [ -b "${DISK}2" ]; then
+if [ -b "$(part_path "$DISK" 1)" ] && [ -b "$(part_path "$DISK" 2)" ]; then
echo "Existing partitions found on $DISK."
for i in 1 2; do
- PART="${DISK}${i}"
+ PART="$(part_path "$DISK" "$i")"
FSTYPE="$(sudo blkid -s TYPE -o value "$PART" 2>/dev/null || true)"
@@
for i in 1 2; do
- PART="${DISK}${i}"
+ PART="$(part_path "$DISK" "$i")"
@@
for i in 1 2; do
- PART="${DISK}${i}"
+ PART="$(part_path "$DISK" "$i")"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/0_format_drives.sh` around lines 22 - 25, The script builds partition
paths as "${DISK}${i}" which breaks for device names that end with a digit
(e.g., /dev/nvme0n1, /dev/mmcblk0); add a small helper (e.g., partition_for or
make_partition) that takes disk and index and returns "${DISK}p${i}" when DISK
ends with a digit and "${DISK}${i}" otherwise (use a shell test like [[ "$DISK"
=~ [0-9]$ ]] to detect this). Replace all direct constructions of
PART="${DISK}${i}" (and any other concatenations at the sites referenced) with
calls to this helper so partition paths are correct for both NVMe/MMC-style and
traditional devices.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
kubernetes/1_setup_kubernetes_nodes.md (1)
52-54:⚠️ Potential issue | 🟡 MinorAdd language identifiers to remaining fenced command blocks.
This is still failing MD040. Please add
bash(orshell) to each unlabeled command fence (e.g., Line [52], Line [72], Line [85], etc.).Suggested pattern
-``` +```bash ./scripts/1_microk8s_setup.shApply the same pattern to each unlabeled shell-command block listed above. </details> Also applies to: 72-77, 85-88, 92-94, 98-100, 104-106, 110-112, 114-116, 120-122, 124-126, 136-138, 146-148, 160-162, 164-168 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@kubernetes/1_setup_kubernetes_nodes.mdaround lines 52 - 54, Update the
unlabeled fenced command blocks in kubernetes/1_setup_kubernetes_nodes.md by
adding a shell language identifier (e.g.,bash orshell) to each opening
fence; specifically change blocks containing commands like
"./scripts/1_microk8s_setup.sh", "microk8s status --wait-ready", "microk8s
enable dns registry helm3", and other plain triple-fence command blocks to usepattern to every unlabeled shell-command block listed in the review.scripts/0_format_drives.sh (2)
22-25:⚠️ Potential issue | 🔴 CriticalFix partition path construction for NVMe/MMC devices.
Line [22], Line [25], Line [52], and Line [112] assume
${DISK}${i}. That fails for devices ending in digits (/dev/nvme0n1,/dev/mmcblk0) and can trigger incorrect repartition behavior.Proposed fix
+part_path() { + local disk="$1" + local idx="$2" + if [[ "$disk" =~ [0-9]$ ]]; then + echo "${disk}p${idx}" + else + echo "${disk}${idx}" + fi +} + -if [ -b "${DISK}1" ] && [ -b "${DISK}2" ]; then +if [ -b "$(part_path "$DISK" 1)" ] && [ -b "$(part_path "$DISK" 2)" ]; then echo "Existing partitions found on $DISK." for i in 1 2; do - PART="${DISK}${i}" + PART="$(part_path "$DISK" "$i")" FSTYPE="$(sudo blkid -s TYPE -o value "$PART" 2>/dev/null || true)" @@ for i in 1 2; do - PART="${DISK}${i}" + PART="$(part_path "$DISK" "$i")" @@ for i in 1 2; do - PART="${DISK}${i}" + PART="$(part_path "$DISK" "$i")" PARTUUID="$(sudo blkid -s PARTUUID -o value "$PART")"Also applies to: 51-53, 111-113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/0_format_drives.sh` around lines 22 - 25, The partition path construction wrongly uses "${DISK}${i}" which breaks for device names that already end in a digit (e.g., /dev/nvme0n1, /dev/mmcblk0); update all places that build partition paths and block-device checks (where PART is formed and tests like [ -b "${DISK}1" ]) to choose the suffix dynamically: if DISK matches nvme or mmcblk patterns append "p${i}", otherwise append "${i}". Implement this logic once (a small helper using a case/regex on the DISK variable) and reuse it for the PART variable construction and all block-device existence checks referenced by the script (the places currently using "${DISK}${i}").
93-97:⚠️ Potential issue | 🟠 MajorGuard against duplicate
/etc/fstabmountpoint entries on reruns.Line [93] only checks PARTUUID. Repartition/reformat can change PARTUUID and still leave stale entries for the same mountpoint.
Proposed fix
- if grep -q "PARTUUID=${PARTUUID}" /etc/fstab; then - echo "fstab entry for PARTUUID=${PARTUUID} already exists, skipping." + if grep -q "PARTUUID=${PARTUUID}" /etc/fstab || grep -qE "[[:space:]]${MOUNTPOINT}[[:space:]]" /etc/fstab; then + echo "fstab entry for PARTUUID=${PARTUUID} or mountpoint ${MOUNTPOINT} already exists, skipping." else echo "PARTUUID=${PARTUUID} ${MOUNTPOINT} ext4 defaults,nofail 0 2" | sudo tee -a /etc/fstab fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/0_format_drives.sh` around lines 93 - 97, The script currently only checks for an existing PARTUUID in /etc/fstab which can miss stale entries for the same mountpoint; update the check before writing to /etc/fstab to detect an existing entry by either PARTUUID or the target MOUNTPOINT (variable MOUNTPOINT) and skip adding if either is present (or optionally remove the stale line for that mountpoint before appending). In short: replace the single grep for "PARTUUID=${PARTUUID}" with a check that looks for "PARTUUID=${PARTUUID}" OR any line containing the exact mountpoint (the ${MOUNTPOINT} token) in /etc/fstab, and only append the new fstab line when neither is found; reference variables PARTUUID and MOUNTPOINT and the /etc/fstab file in your fix.
🤖 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/0_format_drives.sh`:
- Around line 68-76: When KEEP[$((i-1))] is true you still hardcode "ext4" into
/etc/fstab; instead detect and use the actual filesystem type for $PART. Modify
the partition-handling logic (the block that reads KEEP, the mkfs call, and the
fstab write) to set a FSTYPE variable: if KEEP is true, obtain the type via
blkid -s TYPE -o value "$PART" or lsblk -no FSTYPE "$PART" (falling back
gracefully if detection fails); if not keeping, set FSTYPE="ext4" (matching the
mkfs.ext4). Use that FSTYPE when writing the /etc/fstab entry so preserved
partitions keep their real filesystem type rather than being forced to ext4.
---
Duplicate comments:
In `@kubernetes/1_setup_kubernetes_nodes.md`:
- Around line 52-54: Update the unlabeled fenced command blocks in
kubernetes/1_setup_kubernetes_nodes.md by adding a shell language identifier
(e.g., ```bash or ```shell) to each opening fence; specifically change blocks
containing commands like "./scripts/1_microk8s_setup.sh", "microk8s status
--wait-ready", "microk8s enable dns registry helm3", and other plain
triple-fence command blocks to use ```bash so they pass MD040; leave the command
contents unchanged and apply this pattern to every unlabeled shell-command block
listed in the review.
In `@scripts/0_format_drives.sh`:
- Around line 22-25: The partition path construction wrongly uses "${DISK}${i}"
which breaks for device names that already end in a digit (e.g., /dev/nvme0n1,
/dev/mmcblk0); update all places that build partition paths and block-device
checks (where PART is formed and tests like [ -b "${DISK}1" ]) to choose the
suffix dynamically: if DISK matches nvme or mmcblk patterns append "p${i}",
otherwise append "${i}". Implement this logic once (a small helper using a
case/regex on the DISK variable) and reuse it for the PART variable construction
and all block-device existence checks referenced by the script (the places
currently using "${DISK}${i}").
- Around line 93-97: The script currently only checks for an existing PARTUUID
in /etc/fstab which can miss stale entries for the same mountpoint; update the
check before writing to /etc/fstab to detect an existing entry by either
PARTUUID or the target MOUNTPOINT (variable MOUNTPOINT) and skip adding if
either is present (or optionally remove the stale line for that mountpoint
before appending). In short: replace the single grep for "PARTUUID=${PARTUUID}"
with a check that looks for "PARTUUID=${PARTUUID}" OR any line containing the
exact mountpoint (the ${MOUNTPOINT} token) in /etc/fstab, and only append the
new fstab line when neither is found; reference variables PARTUUID and
MOUNTPOINT and the /etc/fstab file in your fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef82529f-7e52-4538-9aeb-048021a82a9d
📒 Files selected for processing (2)
kubernetes/1_setup_kubernetes_nodes.mdscripts/0_format_drives.sh
Comment on lines
+68
to
+76
| if [ "${KEEP[$((i-1))]}" = "true" ]; then | ||
| echo "Keeping existing filesystem on $PART." | ||
| else | ||
| read -r -p "Format $PART as ext4 with label $LABEL? This will destroy data on this partition. [y/N]: " REPLY | ||
| if [[ ! "${REPLY:-N}" =~ ^[Yy]$ ]]; then | ||
| echo "Skipping format of $PART." | ||
| continue | ||
| fi | ||
| sudo mkfs.ext4 -F -L "$LABEL" "$PART" |
There was a problem hiding this comment.
Don’t hardcode ext4 in /etc/fstab when preserving existing filesystems.
If the user keeps an existing filesystem (Line [68]), Line [96] still writes ext4. That can create invalid fstab entries for non-ext4 partitions.
Proposed fix
if [[ "$REPLY" =~ ^[Yy]$ ]]; then
sudo mkdir -p "$MOUNTPOINT"
PARTUUID="$(sudo blkid -s PARTUUID -o value "$PART")"
+ FSTYPE="$(sudo blkid -s TYPE -o value "$PART")"
+ if [ -z "$FSTYPE" ]; then
+ echo "Error: could not detect filesystem type for $PART"
+ exit 1
+ fi
if grep -q "PARTUUID=${PARTUUID}" /etc/fstab; then
echo "fstab entry for PARTUUID=${PARTUUID} already exists, skipping."
else
- echo "PARTUUID=${PARTUUID} ${MOUNTPOINT} ext4 defaults,nofail 0 2" | sudo tee -a /etc/fstab
+ echo "PARTUUID=${PARTUUID} ${MOUNTPOINT} ${FSTYPE} defaults,nofail 0 2" | sudo tee -a /etc/fstab
fiAlso applies to: 92-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/0_format_drives.sh` around lines 68 - 76, When KEEP[$((i-1))] is true
you still hardcode "ext4" into /etc/fstab; instead detect and use the actual
filesystem type for $PART. Modify the partition-handling logic (the block that
reads KEEP, the mkfs call, and the fstab write) to set a FSTYPE variable: if
KEEP is true, obtain the type via blkid -s TYPE -o value "$PART" or lsblk -no
FSTYPE "$PART" (falling back gracefully if detection fails); if not keeping, set
FSTYPE="ext4" (matching the mkfs.ext4). Use that FSTYPE when writing the
/etc/fstab entry so preserved partitions keep their real filesystem type rather
than being forced to ext4.
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.
Summary by CodeRabbit
Documentation
New Features