Overhaul quickstart docs with self-contained x509pop setup#342
Merged
bupd merged 19 commits intocontainer-registry:mainfrom Feb 21, 2026
Merged
Overhaul quickstart docs with self-contained x509pop setup#342bupd merged 19 commits intocontainer-registry:mainfrom
bupd merged 19 commits intocontainer-registry:mainfrom
Conversation
Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com>
…ages Signed-off-by: bupd <bupdprasanth@gmail.com>
…nds inline Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com>
…ration Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com>
github-actions
bot
added
the
documentation
Contributor
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds docs UI (copy button, details shortcode), rewrites quickstart/installation to use SPIRE x509pop and certificate workflows, changes local registry/Zot defaults from port 8585→5000, and introduces end-to-end demo and helper scripts (master-demo, cleanup, preflight, find-pi). Changes
Sequence Diagram(s)sequenceDiagram
participant Operator as Operator
participant LocalK3s as Local K3s
participant SPIRE as SPIRE
participant GroundControl as Ground Control
participant Harbor as Harbor
participant Edge as Edge Device
Operator->>SPIRE: generate CA, x509pop keys & configs
SPIRE-->>GroundControl: issue SVIDs / enable attestation
Operator->>GroundControl: start services (docker-compose)
GroundControl->>Harbor: create project, robot account, groups
Operator->>Edge: install SPIRE agent, copy certs
Edge->>SPIRE: perform x509pop attestation
Edge->>GroundControl: register satellite, fetch group config
GroundControl->>Edge: push sync commands / artifact list
Edge->>Harbor: pull/mirror images per group
LocalK3s->>Edge: request images -> served from satellite
Operator->>LocalK3s: verify apps and cleanup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
Codacy's Analysis Summary3 new issues (≤ 0 issue) Review Pull Request in Codacy →
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 5 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="website/layouts/_default/baseof.html">
<violation number="1" location="website/layouts/_default/baseof.html:20">
P2: Guard for unsupported Clipboard API and handle writeText failures to avoid runtime errors and silent failures.</violation>
</file>
<file name="website/content/docs/quickstart.md">
<violation number="1" location="website/content/docs/quickstart.md:146">
P2: Private keys are set to world-readable permissions. Use restrictive permissions (e.g., 600) for *.key files to avoid leaking credentials.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Comment on lines
+20
to
+27
| navigator.clipboard.writeText(text).then(function() { | ||
| btn.textContent = 'Copied!'; | ||
| btn.classList.add('copy-btn--copied'); | ||
| setTimeout(function() { | ||
| btn.textContent = 'Copy'; | ||
| btn.classList.remove('copy-btn--copied'); | ||
| }, 2000); | ||
| }); |
Contributor
There was a problem hiding this comment.
P2: Guard for unsupported Clipboard API and handle writeText failures to avoid runtime errors and silent failures.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At website/layouts/_default/baseof.html, line 20:
<comment>Guard for unsupported Clipboard API and handle writeText failures to avoid runtime errors and silent failures.</comment>
<file context>
@@ -9,6 +9,26 @@
+ btn.addEventListener('click', function() {
+ var code = pre.querySelector('code');
+ var text = (code || pre).textContent;
+ navigator.clipboard.writeText(text).then(function() {
+ btn.textContent = 'Copied!';
+ btn.classList.add('copy-btn--copied');
</file context>
Suggested change
| navigator.clipboard.writeText(text).then(function() { | |
| btn.textContent = 'Copied!'; | |
| btn.classList.add('copy-btn--copied'); | |
| setTimeout(function() { | |
| btn.textContent = 'Copy'; | |
| btn.classList.remove('copy-btn--copied'); | |
| }, 2000); | |
| }); | |
| if (!navigator.clipboard) { | |
| return; | |
| } | |
| navigator.clipboard.writeText(text).then(function() { | |
| btn.textContent = 'Copied!'; | |
| btn.classList.add('copy-btn--copied'); | |
| setTimeout(function() { | |
| btn.textContent = 'Copy'; | |
| btn.classList.remove('copy-btn--copied'); | |
| }, 2000); | |
| }).catch(function() { | |
| btn.textContent = 'Copy failed'; | |
| setTimeout(function() { | |
| btn.textContent = 'Copy'; | |
| }, 2000); | |
| }); |
|
|
||
| # Cleanup temp files | ||
| rm -f certs/*.csr certs/*.ext certs/*.srl | ||
| chmod 644 certs/*.key certs/*.crt |
Contributor
There was a problem hiding this comment.
P2: Private keys are set to world-readable permissions. Use restrictive permissions (e.g., 600) for *.key files to avoid leaking credentials.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At website/content/docs/quickstart.md, line 146:
<comment>Private keys are set to world-readable permissions. Use restrictive permissions (e.g., 600) for *.key files to avoid leaking credentials.</comment>
<file context>
@@ -58,62 +60,151 @@ export HARBOR_PASSWORD=MyPassword123
+
+# Cleanup temp files
+rm -f certs/*.csr certs/*.ext certs/*.srl
+chmod 644 certs/*.key certs/*.crt
</file context>
</details>
<a href="https://www.cubic.dev/action/fix/violation/36ff523b-76db-4c7e-986d-c9dc04e82425" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true">
<picture>
<source media="(prefers-color-scheme: dark)" srcset="https://cubic.dev/buttons/fix-with-cubic-dark.svg">
<source media="(prefers-color-scheme: light)" srcset="https://cubic.dev/buttons/fix-with-cubic-light.svg">
<img alt="Fix with Cubic" src="https://cubic.dev/buttons/fix-with-cubic-dark.svg">
</picture>
</a>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
website/assets/css/main.css (1)
710-728: Copy button invisible on touch-only devices.
opacity: 0revealed by:hovernever triggers on touch screens; the copy button is inaccessible without a mouse. A simple fix is to always show the button on small screens.♻️ Proposed fix
.docs-content pre:hover .copy-btn { opacity: 1; } + + `@media` (hover: none) { + .copy-btn { + opacity: 1; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@website/assets/css/main.css` around lines 710 - 728, The copy button is hidden by default via .copy-btn { opacity: 0 } and only revealed by .docs-content pre:hover .copy-btn, which never fires on touch devices; update the stylesheet to make the button visible on touch/small screens by adding a media query targeting touch/low-hover devices (e.g., `@media` (hover: none), `@media` (pointer: coarse) or a max-width breakpoint) that sets .copy-btn to opacity: 1 (or removes the opacity rule) and adjusts layout if needed so the button remains accessible on mobile; keep the existing hover rule for pointer devices so desktop behavior is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@website/content/docs/quickstart.md`:
- Around line 144-146: The chmod command currently makes private keys
world-readable (the line `chmod 644 certs/*.key certs/*.crt`); change it so
private keys are owner-only readable and certificates remain readable by
others—replace with separate commands such as `chmod 600 certs/*.key` and `chmod
644 certs/*.crt` (or a single combined command that applies 600 to `*.key` and
644 to `*.crt`) to fix the permission for private keys.
- Around line 433-437: Add a blank line between the callout text ("Run all
commands in this step on your **edge device**. You will need the following files
from the cloud server (generated in Step 1.2):") and the subsequent bullet list
to satisfy the markdown linter; locate the callout starting with "{{< callout
type="info" >}}" and insert a single empty line before the first list item
(e.g., before "- `certs/ca.crt`") so the list is separated from the paragraph.
In `@website/layouts/_default/baseof.html`:
- Around line 17-27: The click handler for btn uses
navigator.clipboard.writeText without guarding for navigator.clipboard presence
or handling rejections; update the btn.addEventListener('click', ...) logic to
first check if navigator.clipboard and navigator.clipboard.writeText exist and,
if missing, fall back to a safe behavior (e.g., select the code text and use
document.execCommand('copy') or show a user-friendly error), and attach a
.catch() to navigator.clipboard.writeText(text) to handle failures by updating
the button state (e.g., set btn.textContent to an error message, add a failure
class, and revert after timeout) so the UI reflects success or failure;
reference the existing btn, pre, and navigator.clipboard.writeText symbols when
making the changes.
---
Nitpick comments:
In `@website/assets/css/main.css`:
- Around line 710-728: The copy button is hidden by default via .copy-btn {
opacity: 0 } and only revealed by .docs-content pre:hover .copy-btn, which never
fires on touch devices; update the stylesheet to make the button visible on
touch/small screens by adding a media query targeting touch/low-hover devices
(e.g., `@media` (hover: none), `@media` (pointer: coarse) or a max-width breakpoint)
that sets .copy-btn to opacity: 1 (or removes the opacity rule) and adjusts
layout if needed so the button remains accessible on mobile; keep the existing
hover rule for pointer devices so desktop behavior is unchanged.
Comment on lines
+144
to
+146
| # Cleanup temp files | ||
| rm -f certs/*.csr certs/*.ext certs/*.srl | ||
| chmod 644 certs/*.key certs/*.crt |
Contributor
There was a problem hiding this comment.
Private key files set world-readable — fix chmod permissions.
chmod 644 certs/*.key makes private keys readable by all users on the system. Keys should be 600 (owner-read-only).
🔒 Proposed fix
-chmod 644 certs/*.key certs/*.crt
+chmod 600 certs/*.key
+chmod 644 certs/*.crt📝 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
| # Cleanup temp files | |
| rm -f certs/*.csr certs/*.ext certs/*.srl | |
| chmod 644 certs/*.key certs/*.crt | |
| # Cleanup temp files | |
| rm -f certs/*.csr certs/*.ext certs/*.srl | |
| chmod 600 certs/*.key | |
| chmod 644 certs/*.crt |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@website/content/docs/quickstart.md` around lines 144 - 146, The chmod command
currently makes private keys world-readable (the line `chmod 644 certs/*.key
certs/*.crt`); change it so private keys are owner-only readable and
certificates remain readable by others—replace with separate commands such as
`chmod 600 certs/*.key` and `chmod 644 certs/*.crt` (or a single combined
command that applies 600 to `*.key` and 644 to `*.crt`) to fix the permission
for private keys.
Comment on lines
+433
to
+437
| {{< callout type="info" >}} | ||
| Run all commands in this step on your **edge device**. You will need the following files from the cloud server (generated in Step 1.2): | ||
| - `certs/ca.crt` (bootstrap trust bundle) | ||
| - `certs/us-east-1.crt` (satellite agent certificate) | ||
| - `certs/us-east-1.key` (satellite agent private key) |
Contributor
There was a problem hiding this comment.
Add blank line before the list inside the callout (linter).
The markdown linter flags this — the list items should be preceded by a blank line.
✏️ Proposed fix
-Run all commands in this step on your **edge device**. You will need the following files from the cloud server (generated in Step 1.2):
-- `certs/ca.crt` (bootstrap trust bundle)
+Run all commands in this step on your **edge device**. You will need the following files from the cloud server (generated in Step 1.2):
+
+- `certs/ca.crt` (bootstrap trust bundle)📝 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
| {{< callout type="info" >}} | |
| Run all commands in this step on your **edge device**. You will need the following files from the cloud server (generated in Step 1.2): | |
| - `certs/ca.crt` (bootstrap trust bundle) | |
| - `certs/us-east-1.crt` (satellite agent certificate) | |
| - `certs/us-east-1.key` (satellite agent private key) | |
| {{< callout type="info" >}} | |
| Run all commands in this step on your **edge device**. You will need the following files from the cloud server (generated in Step 1.2): | |
| - `certs/ca.crt` (bootstrap trust bundle) | |
| - `certs/us-east-1.crt` (satellite agent certificate) | |
| - `certs/us-east-1.key` (satellite agent private key) |
🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[notice] 435-435: website/content/docs/quickstart.md#L435
Lists should be surrounded by blank lines
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@website/content/docs/quickstart.md` around lines 433 - 437, Add a blank line
between the callout text ("Run all commands in this step on your **edge
device**. You will need the following files from the cloud server (generated in
Step 1.2):") and the subsequent bullet list to satisfy the markdown linter;
locate the callout starting with "{{< callout type="info" >}}" and insert a
single empty line before the first list item (e.g., before "- `certs/ca.crt`")
so the list is separated from the paragraph.
Comment on lines
+17
to
+27
| btn.addEventListener('click', function() { | ||
| var code = pre.querySelector('code'); | ||
| var text = (code || pre).textContent; | ||
| navigator.clipboard.writeText(text).then(function() { | ||
| btn.textContent = 'Copied!'; | ||
| btn.classList.add('copy-btn--copied'); | ||
| setTimeout(function() { | ||
| btn.textContent = 'Copy'; | ||
| btn.classList.remove('copy-btn--copied'); | ||
| }, 2000); | ||
| }); |
Contributor
There was a problem hiding this comment.
Missing navigator.clipboard guard and .catch() handler.
navigator.clipboard is undefined in non-secure HTTP contexts (anything other than https:// or localhost), so clicking the button would throw an uncaught TypeError. Even in secure contexts, clipboard writes can be rejected by permission policy — with no .catch(), the failure is silent and the button appears broken.
🛡️ Proposed fix
- navigator.clipboard.writeText(text).then(function() {
- btn.textContent = 'Copied!';
- btn.classList.add('copy-btn--copied');
- setTimeout(function() {
- btn.textContent = 'Copy';
- btn.classList.remove('copy-btn--copied');
- }, 2000);
- });
+ if (!navigator.clipboard) return;
+ navigator.clipboard.writeText(text).then(function() {
+ btn.textContent = 'Copied!';
+ btn.classList.add('copy-btn--copied');
+ setTimeout(function() {
+ btn.textContent = 'Copy';
+ btn.classList.remove('copy-btn--copied');
+ }, 2000);
+ }).catch(function() {
+ btn.textContent = 'Failed';
+ setTimeout(function() { btn.textContent = 'Copy'; }, 2000);
+ });📝 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
| btn.addEventListener('click', function() { | |
| var code = pre.querySelector('code'); | |
| var text = (code || pre).textContent; | |
| navigator.clipboard.writeText(text).then(function() { | |
| btn.textContent = 'Copied!'; | |
| btn.classList.add('copy-btn--copied'); | |
| setTimeout(function() { | |
| btn.textContent = 'Copy'; | |
| btn.classList.remove('copy-btn--copied'); | |
| }, 2000); | |
| }); | |
| btn.addEventListener('click', function() { | |
| var code = pre.querySelector('code'); | |
| var text = (code || pre).textContent; | |
| if (!navigator.clipboard) return; | |
| navigator.clipboard.writeText(text).then(function() { | |
| btn.textContent = 'Copied!'; | |
| btn.classList.add('copy-btn--copied'); | |
| setTimeout(function() { | |
| btn.textContent = 'Copy'; | |
| btn.classList.remove('copy-btn--copied'); | |
| }, 2000); | |
| }).catch(function() { | |
| btn.textContent = 'Failed'; | |
| setTimeout(function() { btn.textContent = 'Copy'; }, 2000); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@website/layouts/_default/baseof.html` around lines 17 - 27, The click handler
for btn uses navigator.clipboard.writeText without guarding for
navigator.clipboard presence or handling rejections; update the
btn.addEventListener('click', ...) logic to first check if navigator.clipboard
and navigator.clipboard.writeText exist and, if missing, fall back to a safe
behavior (e.g., select the code text and use document.execCommand('copy') or
show a user-friendly error), and attach a .catch() to
navigator.clipboard.writeText(text) to handle failures by updating the button
state (e.g., set btn.textContent to an error message, add a failure class, and
revert after timeout) so the UI reflects success or failure; reference the
existing btn, pre, and navigator.clipboard.writeText symbols when making the
changes.
Signed-off-by: bupd <bupdprasanth@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
pre-demo-var-check.sh (1)
7-19: Hardcoded default IPs are specific to a particular environment.
CLOUD_IPdefaults to10.147.106.55andSAT_IPto10.147.106.144. While these are overridable via environment variables, consider using more obviously-placeholder values (e.g.,192.0.2.xfrom the documentation range RFC 5737) so users don't accidentally target an unrelated network.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pre-demo-var-check.sh` around lines 7 - 19, The script currently sets environment defaults CLOUD_IP and SAT_IP to real-looking addresses (CLOUD_IP and SAT_IP) which can accidentally target production networks; change their default values to non-routable/documentation placeholder addresses (e.g., use addresses from RFC 5737 like 192.0.2.1 and 192.0.2.2) so the defaults are clearly harmless and update the corresponding derived value HARBOR_URL if it relies on CLOUD_IP; ensure all references to CLOUD_IP, SAT_IP (and any dependent HARBOR_URL) still work when overridden by real environment variables.master-demo.sh (1)
11-28: Hardcoded configuration without environment variable overrides.Same issue as
cleanup-demo.sh—pre-demo-var-check.shuses${VAR:-default}but this script hardcodes everything. Use the same pattern for consistency across all three scripts.Proposed fix (excerpt)
-CLOUD_IP="10.147.106.55" -HARBOR_URL="http://${CLOUD_IP}:8080" -HARBOR_USERNAME="admin" -HARBOR_PASSWORD="Harbor12345" -ADMIN_PASSWORD="Harbor12345" +CLOUD_IP="${CLOUD_IP:-10.147.106.55}" +HARBOR_URL="${HARBOR_URL:-http://${CLOUD_IP}:8080}" +HARBOR_USERNAME="${HARBOR_USERNAME:-admin}" +HARBOR_PASSWORD="${HARBOR_PASSWORD:-Harbor12345}" +ADMIN_PASSWORD="${ADMIN_PASSWORD:-Harbor12345}" -SAT_USER="sat-1" -SAT_IP="10.147.106.144" -SAT_PASS="password" -SAT_UID="1000" -SAT_NAME="us-east-1" +SAT_USER="${SAT_USER:-sat-1}" +SAT_IP="${SAT_IP:-10.147.106.144}" +SAT_UID="${SAT_UID:-1000}" +SAT_NAME="${SAT_NAME:-us-east-1}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@master-demo.sh` around lines 11 - 28, master-demo.sh currently hardcodes configuration values; change each assignment (CLOUD_IP, HARBOR_URL, HARBOR_USERNAME, HARBOR_PASSWORD, ADMIN_PASSWORD, SAT_USER, SAT_IP, SAT_PASS, SAT_UID, SAT_NAME, DEMO_IMAGE, DEMO_TAG, GROUP_NAME, GC_HOST_PORT, SPIRE_HOST_PORT) to use POSIX parameter expansion so values can be overridden from the environment (e.g. VAR="${VAR:-default}") matching the pattern used in pre-demo-var-check.sh and cleanup-demo.sh; ensure HARBOR_URL still constructs correctly when CLOUD_IP is overridden and keep default values exactly as in the diff.website/content/docs/quickstart.md (1)
383-386: Volume namegc_spire-server-dataassumes the Compose project is namedgc.Docker Compose derives the project name from the directory. Since the instructions say
cd quickstart/gc, this works — but if a user renames the directory or setsCOMPOSE_PROJECT_NAME, the pre-created volume won't match. A brief inline note (e.g., "thegc_prefix must match your directory name") would reduce confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@website/content/docs/quickstart.md` around lines 383 - 386, The hard-coded volume name "gc_spire-server-data" in the quickstart snippet assumes the Compose project name prefix "gc" — update the docs around the two-line block (the docker volume create + chmod commands) to add a brief inline note that the "gc_" prefix comes from the Compose project name (derived from the directory or COMPOSE_PROJECT_NAME), and instruct readers to either rename the volume to match their project prefix or set COMPOSE_PROJECT_NAME accordingly (mention "gc_spire-server-data" and the quickstart/gc directory as examples so they can locate the snippet).cleanup-demo.sh (1)
7-14: Configuration is hardcoded — unlikepre-demo-var-check.sh, which uses${VAR:-default}.
pre-demo-var-check.shallows environment-variable overrides (e.g.,CLOUD_IP="${CLOUD_IP:-10.147.106.55}"), but this script hardcodes the same values. Use the same pattern for consistency so users don't need to edit the script.Proposed fix
-CLOUD_IP="10.147.106.55" -HARBOR_URL="http://${CLOUD_IP}:8080" -HARBOR_USERNAME="admin" -HARBOR_PASSWORD="Harbor12345" -SAT_USER="sat-1" -SAT_IP="10.147.106.144" -SAT_NAME="us-east-1" -WORK_DIR="$HOME/quickstart" +CLOUD_IP="${CLOUD_IP:-10.147.106.55}" +HARBOR_URL="${HARBOR_URL:-http://${CLOUD_IP}:8080}" +HARBOR_USERNAME="${HARBOR_USERNAME:-admin}" +HARBOR_PASSWORD="${HARBOR_PASSWORD:-Harbor12345}" +SAT_USER="${SAT_USER:-sat-1}" +SAT_IP="${SAT_IP:-10.147.106.144}" +SAT_NAME="${SAT_NAME:-us-east-1}" +WORK_DIR="${WORK_DIR:-$HOME/quickstart}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cleanup-demo.sh` around lines 7 - 14, The script currently hardcodes configuration variables (CLOUD_IP, HARBOR_URL, HARBOR_USERNAME, HARBOR_PASSWORD, SAT_USER, SAT_IP, SAT_NAME, WORK_DIR); change each assignment to use shell parameter expansion with defaults (e.g., CLOUD_IP="${CLOUD_IP:-10.147.106.55}") so callers can override via environment variables, ensure HARBOR_URL is constructed from the resolved CLOUD_IP (e.g., HARBOR_URL="http://${CLOUD_IP}:8080" after CLOUD_IP is set), and make WORK_DIR use a default via ${WORK_DIR:-$HOME/quickstart}; apply this pattern to the variables named CLOUD_IP, HARBOR_URL, HARBOR_USERNAME, HARBOR_PASSWORD, SAT_USER, SAT_IP, SAT_NAME, and WORK_DIR.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cleanup-demo.sh`:
- Around line 84-86: The current ROBOT_ID lookup uses jq's test() which does an
unanchored regex and can match unintended names; change the jq invocation to
pass SAT_NAME as an argument and use exact equality (or an anchored regex)
instead — e.g. call jq with --arg name "$SAT_NAME" and use '.[] | select(.name
== $name) | .id // empty' (refer to the ROBOT_ID assignment, the jq expression
using test(), and the SAT_NAME variable) so only exact robot names are matched.
In `@find-pi.sh`:
- Line 39: Remove the unused variable `found` and any increments of it inside
the pipeline-created subshell; delete the `found=0` declaration and any
references to `found` in the script. If you actually intend to count items from
the `while` loop, convert the pipeline into a process substitution or redirected
input so the loop runs in the main shell (e.g., use `while ...; do ...; done <
<(command)` instead of `command | while ...`) so the counter variable (e.g.,
`found`) will propagate correctly.
In `@master-demo.sh`:
- Around line 460-461: The script is making private keys world-readable by
running "chmod 644 certs/*.key"; update master-demo.sh to set stricter
permissions for private keys by changing the chmod for key files to 600 while
leaving certs/*.crt as 644 (or similarly non-secret files unchanged); locate the
chmod invocation in the script (the line that currently reads chmod 644
certs/*.key certs/*.crt) and split it so you run chmod 600 certs/*.key and chmod
644 certs/*.crt (keep the existing rm -f certs/*.csr certs/*.ext certs/*.srl
unaffected).
- Line 19: Remove the unused SAT_PASS variable declaration to avoid implying
password-based SSH support: locate the SAT_PASS declaration in master-demo.sh,
delete that assignment, and ensure no other code references SAT_PASS (if any,
replace with key-based config or remove references). Also update any comments or
documentation in the script that mention password auth so they consistently
reflect that only key-based SSH is required by the pre-flight check.
---
Duplicate comments:
In `@website/content/docs/quickstart.md`:
- Around line 144-146: Change the file-permission command to ensure private keys
are owner-only readable: replace the current chmod command that sets
"certs/*.key certs/*.crt" to 644 with a two-step chmod so that certs/*.key files
are set to 600 (owner read/write only) and certs/*.crt files remain 644; update
the chmod invocation that currently operates on "certs/*.key certs/*.crt"
accordingly in quickstart.md so private keys (certs/*.key) are not
world-readable.
- Around line 442-447: Insert a blank line between the callout opening tag {{<
callout type="info" >}} and the start of the list so the markdown list is
preceded by an empty line; specifically, modify the block that begins with {{<
callout type="info" >}} and ends with {{< /callout >}} so it reads the info
message line, then a blank line, then the three list items (`- certs/ca.crt`, `-
certs/us-east-1.crt`, `- certs/us-east-1.key`) to satisfy the markdown linter.
---
Nitpick comments:
In `@cleanup-demo.sh`:
- Around line 7-14: The script currently hardcodes configuration variables
(CLOUD_IP, HARBOR_URL, HARBOR_USERNAME, HARBOR_PASSWORD, SAT_USER, SAT_IP,
SAT_NAME, WORK_DIR); change each assignment to use shell parameter expansion
with defaults (e.g., CLOUD_IP="${CLOUD_IP:-10.147.106.55}") so callers can
override via environment variables, ensure HARBOR_URL is constructed from the
resolved CLOUD_IP (e.g., HARBOR_URL="http://${CLOUD_IP}:8080" after CLOUD_IP is
set), and make WORK_DIR use a default via ${WORK_DIR:-$HOME/quickstart}; apply
this pattern to the variables named CLOUD_IP, HARBOR_URL, HARBOR_USERNAME,
HARBOR_PASSWORD, SAT_USER, SAT_IP, SAT_NAME, and WORK_DIR.
In `@master-demo.sh`:
- Around line 11-28: master-demo.sh currently hardcodes configuration values;
change each assignment (CLOUD_IP, HARBOR_URL, HARBOR_USERNAME, HARBOR_PASSWORD,
ADMIN_PASSWORD, SAT_USER, SAT_IP, SAT_PASS, SAT_UID, SAT_NAME, DEMO_IMAGE,
DEMO_TAG, GROUP_NAME, GC_HOST_PORT, SPIRE_HOST_PORT) to use POSIX parameter
expansion so values can be overridden from the environment (e.g.
VAR="${VAR:-default}") matching the pattern used in pre-demo-var-check.sh and
cleanup-demo.sh; ensure HARBOR_URL still constructs correctly when CLOUD_IP is
overridden and keep default values exactly as in the diff.
In `@pre-demo-var-check.sh`:
- Around line 7-19: The script currently sets environment defaults CLOUD_IP and
SAT_IP to real-looking addresses (CLOUD_IP and SAT_IP) which can accidentally
target production networks; change their default values to
non-routable/documentation placeholder addresses (e.g., use addresses from RFC
5737 like 192.0.2.1 and 192.0.2.2) so the defaults are clearly harmless and
update the corresponding derived value HARBOR_URL if it relies on CLOUD_IP;
ensure all references to CLOUD_IP, SAT_IP (and any dependent HARBOR_URL) still
work when overridden by real environment variables.
In `@website/content/docs/quickstart.md`:
- Around line 383-386: The hard-coded volume name "gc_spire-server-data" in the
quickstart snippet assumes the Compose project name prefix "gc" — update the
docs around the two-line block (the docker volume create + chmod commands) to
add a brief inline note that the "gc_" prefix comes from the Compose project
name (derived from the directory or COMPOSE_PROJECT_NAME), and instruct readers
to either rename the volume to match their project prefix or set
COMPOSE_PROJECT_NAME accordingly (mention "gc_spire-server-data" and the
quickstart/gc directory as examples so they can locate the snippet).
Comment on lines
+84
to
+86
| ROBOT_ID=$(curl -sk -u "${HARBOR_USERNAME}:${HARBOR_PASSWORD}" \ | ||
| "${HARBOR_URL}/api/v2.0/robots" 2>/dev/null \ | ||
| | jq -r ".[] | select(.name | test(\"${SAT_NAME}\")) | .id // empty" 2>/dev/null | head -1) |
Contributor
There was a problem hiding this comment.
Robot account lookup uses regex test() — may match unintended accounts.
jq -r ".[] | select(.name | test(\"${SAT_NAME}\"))" performs a regex substring match. If SAT_NAME is us-east-1, it would also match a robot named us-east-10. Use an anchored pattern or exact string match:
Proposed fix (exact substring bounded by word boundaries)
- | jq -r ".[] | select(.name | test(\"${SAT_NAME}\")) | .id // empty" 2>/dev/null | head -1)
+ | jq -r ".[] | select(.name | contains(\"${SAT_NAME}\")) | .id // empty" 2>/dev/null | head -1)Or more precise with an anchored pattern:
- | jq -r ".[] | select(.name | test(\"${SAT_NAME}\")) | .id // empty" 2>/dev/null | head -1)
+ | jq -r ".[] | select(.name | test(\"\\b${SAT_NAME}\\b\")) | .id // empty" 2>/dev/null | head -1)📝 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
| ROBOT_ID=$(curl -sk -u "${HARBOR_USERNAME}:${HARBOR_PASSWORD}" \ | |
| "${HARBOR_URL}/api/v2.0/robots" 2>/dev/null \ | |
| | jq -r ".[] | select(.name | test(\"${SAT_NAME}\")) | .id // empty" 2>/dev/null | head -1) | |
| ROBOT_ID=$(curl -sk -u "${HARBOR_USERNAME}:${HARBOR_PASSWORD}" \ | |
| "${HARBOR_URL}/api/v2.0/robots" 2>/dev/null \ | |
| | jq -r ".[] | select(.name | test(\"\\b${SAT_NAME}\\b\")) | .id // empty" 2>/dev/null | head -1) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cleanup-demo.sh` around lines 84 - 86, The current ROBOT_ID lookup uses jq's
test() which does an unanchored regex and can match unintended names; change the
jq invocation to pass SAT_NAME as an argument and use exact equality (or an
anchored regex) instead — e.g. call jq with --arg name "$SAT_NAME" and use '.[]
| select(.name == $name) | .id // empty' (refer to the ROBOT_ID assignment, the
jq expression using test(), and the SAT_NAME variable) so only exact robot names
are matched.
| wait | ||
|
|
||
| # Collect results | ||
| found=0 |
Contributor
There was a problem hiding this comment.
Remove unused variable found.
found is declared but never read. Additionally, even if it were incremented inside the while loop on Line 45, it wouldn't propagate back because the pipe creates a subshell.
Proposed fix
-found=0🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[warning] 39-39: find-pi.sh#L39
found appears unused. Verify use (or export if used externally).
🪛 Shellcheck (0.11.0)
[warning] 39-39: found appears unused. Verify use (or export if used externally).
(SC2034)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@find-pi.sh` at line 39, Remove the unused variable `found` and any increments
of it inside the pipeline-created subshell; delete the `found=0` declaration and
any references to `found` in the script. If you actually intend to count items
from the `while` loop, convert the pipeline into a process substitution or
redirected input so the loop runs in the main shell (e.g., use `while ...; do
...; done < <(command)` instead of `command | while ...`) so the counter
variable (e.g., `found`) will propagate correctly.
|
|
||
| SAT_USER="sat-1" | ||
| SAT_IP="10.147.106.144" | ||
| SAT_PASS="password" |
Contributor
There was a problem hiding this comment.
SAT_PASS is declared but never used.
Remove it to avoid confusion about whether password-based SSH is supported by this script (the pre-flight check on Line 98 explicitly requires key-based auth).
Proposed fix
-SAT_PASS="password"📝 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
| SAT_PASS="password" |
🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[warning] 19-19: master-demo.sh#L19
SAT_PASS appears unused. Verify use (or export if used externally).
🪛 Shellcheck (0.11.0)
[warning] 19-19: SAT_PASS appears unused. Verify use (or export if used externally).
(SC2034)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@master-demo.sh` at line 19, Remove the unused SAT_PASS variable declaration
to avoid implying password-based SSH support: locate the SAT_PASS declaration in
master-demo.sh, delete that assignment, and ensure no other code references
SAT_PASS (if any, replace with key-based config or remove references). Also
update any comments or documentation in the script that mention password auth so
they consistently reflect that only key-based SSH is required by the pre-flight
check.
Comment on lines
+460
to
+461
| rm -f certs/*.csr certs/*.ext certs/*.srl | ||
| chmod 644 certs/*.key certs/*.crt |
Contributor
There was a problem hiding this comment.
Private keys set world-readable — same issue as in quickstart.md.
chmod 644 certs/*.key makes private keys readable by all users. Use 600 for key files.
Proposed fix
-chmod 644 certs/*.key certs/*.crt
+chmod 600 certs/*.key
+chmod 644 certs/*.crt📝 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
| rm -f certs/*.csr certs/*.ext certs/*.srl | |
| chmod 644 certs/*.key certs/*.crt | |
| rm -f certs/*.csr certs/*.ext certs/*.srl | |
| chmod 600 certs/*.key | |
| chmod 644 certs/*.crt |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@master-demo.sh` around lines 460 - 461, The script is making private keys
world-readable by running "chmod 644 certs/*.key"; update master-demo.sh to set
stricter permissions for private keys by changing the chmod for key files to 600
while leaving certs/*.crt as 644 (or similarly non-secret files unchanged);
locate the chmod invocation in the script (the line that currently reads chmod
644 certs/*.key certs/*.crt) and split it so you run chmod 600 certs/*.key and
chmod 644 certs/*.crt (keep the existing rm -f certs/*.csr certs/*.ext
certs/*.srl unaffected).
Contributor
There was a problem hiding this comment.
4 issues found across 7 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cleanup-demo.sh">
<violation number="1" location="cleanup-demo.sh:25">
P2: Avoid disabling SSH host key checking; it opens the cleanup step to MITM. Use the default behavior or a safer mode like accept-new.</violation>
</file>
<file name="master-demo.sh">
<violation number="1" location="master-demo.sh:461">
P1: Private keys should not be world-readable. Use restrictive permissions for `certs/*.key` so other users cannot read the x509pop keys.</violation>
</file>
<file name="find-pi.sh">
<violation number="1" location="find-pi.sh:45">
P2: The `grep` pipeline will exit the script when there are no matching ARP entries because `set -euo pipefail` treats `grep`’s “no matches” as a failure. This makes the script abort instead of just showing an empty list of devices.</violation>
</file>
<file name="website/content/docs/quickstart.md">
<violation number="1" location="website/content/docs/quickstart.md:385">
P2: Avoid making the SPIRE data volume world-writable. This directory holds the server keys and datastore; 777 permissions let any local process tamper with them. Use least-privilege permissions and ownership for the SPIRE user instead.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| -out certs/${SAT_NAME}.crt -extfile certs/${SAT_NAME}.ext 2>/dev/null | ||
|
|
||
| rm -f certs/*.csr certs/*.ext certs/*.srl | ||
| chmod 644 certs/*.key certs/*.crt |
Contributor
There was a problem hiding this comment.
P1: Private keys should not be world-readable. Use restrictive permissions for certs/*.key so other users cannot read the x509pop keys.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At master-demo.sh, line 461:
<comment>Private keys should not be world-readable. Use restrictive permissions for `certs/*.key` so other users cannot read the x509pop keys.</comment>
<file context>
@@ -0,0 +1,1210 @@
+ -out certs/${SAT_NAME}.crt -extfile certs/${SAT_NAME}.ext 2>/dev/null
+
+rm -f certs/*.csr certs/*.ext certs/*.srl
+chmod 644 certs/*.key certs/*.crt
+
+info "Certificates generated:"
</file context>
| info() { echo -e " ${CYAN}$1${RESET}"; } | ||
|
|
||
| remote() { | ||
| ssh -o StrictHostKeyChecking=no -o ConnectTimeout=10 \ |
Contributor
There was a problem hiding this comment.
P2: Avoid disabling SSH host key checking; it opens the cleanup step to MITM. Use the default behavior or a safer mode like accept-new.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cleanup-demo.sh, line 25:
<comment>Avoid disabling SSH host key checking; it opens the cleanup step to MITM. Use the default behavior or a safer mode like accept-new.</comment>
<file context>
@@ -0,0 +1,181 @@
+info() { echo -e " ${CYAN}$1${RESET}"; }
+
+remote() {
+ ssh -o StrictHostKeyChecking=no -o ConnectTimeout=10 \
+ "${SAT_USER}@${SAT_IP}" "$@"
+}
</file context>
| printf " %-16s %-19s %s\n" "IP" "MAC" "NOTE" | ||
| echo "----------------------------------------------" | ||
|
|
||
| arp -an | grep "$IFACE" | grep -v incomplete | while read -r line; do |
Contributor
There was a problem hiding this comment.
P2: The grep pipeline will exit the script when there are no matching ARP entries because set -euo pipefail treats grep’s “no matches” as a failure. This makes the script abort instead of just showing an empty list of devices.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At find-pi.sh, line 45:
<comment>The `grep` pipeline will exit the script when there are no matching ARP entries because `set -euo pipefail` treats `grep`’s “no matches” as a failure. This makes the script abort instead of just showing an empty list of devices.</comment>
<file context>
@@ -0,0 +1,65 @@
+printf " %-16s %-19s %s\n" "IP" "MAC" "NOTE"
+echo "----------------------------------------------"
+
+arp -an | grep "$IFACE" | grep -v incomplete | while read -r line; do
+ ip=$(echo "$line" | grep -oP '\(\K[0-9.]+')
+ mac=$(echo "$line" | grep -oP '([0-9a-f]{2}:){5}[0-9a-f]{2}')
</file context>
Contributor
There was a problem hiding this comment.
2 issues found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="website/content/docs/quickstart.md">
<violation number="1" location="website/content/docs/quickstart.md:985">
P2: SSH host key verification is disabled (`StrictHostKeyChecking=no`), which allows MITM attacks. Use `accept-new` or rely on known_hosts instead of disabling verification.</violation>
<violation number="2" location="website/content/docs/quickstart.md:1398">
P1: Private keys are made world-readable (`chmod 644 certs/*.key`), which exposes the x509pop agent keys to any local user. Use 600 for keys and 644 for public certs.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| -out certs/${SAT_NAME}.crt -extfile certs/${SAT_NAME}.ext 2>/dev/null | ||
|
|
||
| rm -f certs/*.csr certs/*.ext certs/*.srl | ||
| chmod 644 certs/*.key certs/*.crt |
Contributor
There was a problem hiding this comment.
P1: Private keys are made world-readable (chmod 644 certs/*.key), which exposes the x509pop agent keys to any local user. Use 600 for keys and 644 for public certs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At website/content/docs/quickstart.md, line 1398:
<comment>Private keys are made world-readable (`chmod 644 certs/*.key`), which exposes the x509pop agent keys to any local user. Use 600 for keys and 644 for public certs.</comment>
<file context>
@@ -750,6 +750,1664 @@ docker network rm harbor-satellite 2>/dev/null || true
+ -out certs/${SAT_NAME}.crt -extfile certs/${SAT_NAME}.ext 2>/dev/null
+
+rm -f certs/*.csr certs/*.ext certs/*.srl
+chmod 644 certs/*.key certs/*.crt
+
+info "Certificates generated:"
</file context>
Suggested change
| chmod 644 certs/*.key certs/*.crt | |
| chmod 600 certs/*.key | |
| chmod 644 certs/*.crt |
|
|
||
| # SSH wrapper (uses key-based auth — run ssh-copy-id first) | ||
| remote() { | ||
| ssh -o StrictHostKeyChecking=no -o ConnectTimeout=10 \ |
Contributor
There was a problem hiding this comment.
P2: SSH host key verification is disabled (StrictHostKeyChecking=no), which allows MITM attacks. Use accept-new or rely on known_hosts instead of disabling verification.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At website/content/docs/quickstart.md, line 985:
<comment>SSH host key verification is disabled (`StrictHostKeyChecking=no`), which allows MITM attacks. Use `accept-new` or rely on known_hosts instead of disabling verification.</comment>
<file context>
@@ -750,6 +750,1664 @@ docker network rm harbor-satellite 2>/dev/null || true
+
+# SSH wrapper (uses key-based auth — run ssh-copy-id first)
+remote() {
+ ssh -o StrictHostKeyChecking=no -o ConnectTimeout=10 \
+ "${SAT_USER}@${SAT_IP}" "$@"
+}
</file context>
Suggested change
| ssh -o StrictHostKeyChecking=no -o ConnectTimeout=10 \ | |
| ssh -o StrictHostKeyChecking=accept-new -o ConnectTimeout=10 \ | |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
website/content/docs/quickstart.md (1)
599-617:⚠️ Potential issue | 🟠 MajorIncorrect registry URL — satellite cannot resolve
harboras a hostname.The note claims Ground Control resolves
harborvia the Compose network, but Harbor is installed separately (online installer) and is not a service in theharbor-satellitenetwork. More critically, the satellite runs as a native binary on the edge device — it cannot resolveharbor:8080at all.
master-demo.shcorrectly uses the actual host IP (${HARBOR_URL}=http://<CLOUD_IP>:8080). The quickstart example should match:🐛 Proposed fix
-Note: The `registry` field uses the Docker-internal service name (`http://harbor:8080`), not your host-facing `HARBOR_URL`. Ground Control runs inside Docker and resolves `harbor` via the Compose network. +Note: The `registry` field must be the URL that the **satellite** can reach Harbor at. Use the actual host IP or `HARBOR_URL`. curl -sk -X POST https://localhost:9080/api/groups/sync \ -H "Content-Type: application/json" \ -H "Authorization: Bearer ${AUTH_TOKEN}" \ -d '{ "group": "edge-images", - "registry": "http://harbor:8080", + "registry": "'"${HARBOR_URL:-http://localhost:8080}"'",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@website/content/docs/quickstart.md` around lines 599 - 617, Update the quickstart docs so the curl example uses the actual host-facing Harbor URL variable instead of the Docker-internal hostname: replace the hardcoded "http://harbor:8080" registry value with the host variable (e.g., "${HARBOR_URL}") and update the adjacent note to state that the satellite runs as a native binary on the edge device and cannot resolve Docker Compose service names; reference the example request payload's "registry" field and the curl invocation (and align with master-demo.sh's use of ${HARBOR_URL}) when making the change.
🧹 Nitpick comments (1)
website/content/docs/quickstart.md (1)
956-958:SAT_PASSis assigned but never used — remove it.All SSH connections in this script use key-based auth (
BatchMode=yes). The variable is dead code and embedding a plaintext password in public documentation is unnecessary.🧹 Proposed fix
SAT_USER="sat-1" SAT_IP="10.147.106.144" -SAT_PASS="password" SAT_UID="1000"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@website/content/docs/quickstart.md` around lines 956 - 958, Remove the unused SAT_PASS="password" assignment from the quickstart doc and any references to SAT_PASS in the surrounding script snippets (leave SAT_UID="1000" and SAT_NAME="us-east-1" intact); since SSH is key-based (BatchMode=yes) there is no password usage, so delete the dead variable and any example that embeds a plaintext password to avoid leaking credentials in the documentation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@website/content/docs/quickstart.md`:
- Around line 2243-2249: The script checks HTTP_CODE for "200" when deleting a
Harbor robot but Harbor returns 204 on success; update the deletion-success
check in the block that sets HTTP_CODE (the curl call and subsequent if) to
treat 204 as success (e.g., accept "204" or any 2xx) and keep the existing info
logging that references ROBOT_ID and uses the info function so successful
deletions no longer report "Failed to delete robot account."
- Around line 666-670: Update the satellite run example that invokes
./harbor-satellite to include the missing --harbor-registry-url and
--use-unsecure flags; specifically, add --harbor-registry-url <HARBOR_URL> (or
${HARBOR_URL} when using the automation script) and --use-unsecure so the
example matches the automation script and allows the local Zot registry (HTTP on
port 5000) to be used without TLS errors.
---
Outside diff comments:
In `@website/content/docs/quickstart.md`:
- Around line 599-617: Update the quickstart docs so the curl example uses the
actual host-facing Harbor URL variable instead of the Docker-internal hostname:
replace the hardcoded "http://harbor:8080" registry value with the host variable
(e.g., "${HARBOR_URL}") and update the adjacent note to state that the satellite
runs as a native binary on the edge device and cannot resolve Docker Compose
service names; reference the example request payload's "registry" field and the
curl invocation (and align with master-demo.sh's use of ${HARBOR_URL}) when
making the change.
---
Duplicate comments:
In `@website/content/docs/quickstart.md`:
- Around line 144-147: The cleanup step currently sets private keys
world-readable; change the permissions so private keys are restricted and certs
remain public: replace the single chmod line that targets both files with two
commands—set certs/*.key to mode 600 (owner read/write only) and certs/*.crt to
mode 644 (owner read/write, others read) so private keys are not world-readable;
update the lines around the "rm -f certs/*.csr certs/*.ext certs/*.srl" and the
chmod invocation in quickstart.md accordingly.
- Around line 442-447: Inside the callout block that starts with "{{< callout
type="info" >}}" and ends with "{{< /callout >}}", add one blank line between
the sentence that ends with ":" and the bulleted list (the lines listing
`certs/ca.crt`, `certs/us-east-1.crt`, `certs/us-east-1.key`) so the list is
separated by an empty line from the preceding paragraph; do not alter other text
or list content.
- Around line 1394-1401: In master-demo.sh fix the insecure file-permissions
change: do not make private key files world-readable; instead set private key
files (certs/*.key) to owner-only readable/writable and set certificate files
(certs/*.crt) to world-readable as needed. Locate the chmod invocation that
currently sets "chmod 644 certs/*.key certs/*.crt" and replace it with two
separate permission changes so keys are 600 (owner-only) and certs are 644.
---
Nitpick comments:
In `@website/content/docs/quickstart.md`:
- Around line 956-958: Remove the unused SAT_PASS="password" assignment from the
quickstart doc and any references to SAT_PASS in the surrounding script snippets
(leave SAT_UID="1000" and SAT_NAME="us-east-1" intact); since SSH is key-based
(BatchMode=yes) there is no password usage, so delete the dead variable and any
example that embeds a plaintext password to avoid leaking credentials in the
documentation.
Comment on lines
+2243
to
+2249
| HTTP_CODE=$(curl -sk -o /dev/null -w '%{http_code}' -X DELETE \ | ||
| -u "${HARBOR_USERNAME}:${HARBOR_PASSWORD}" \ | ||
| "${HARBOR_URL}/api/v2.0/robots/${ROBOT_ID}") | ||
| if [ "$HTTP_CODE" = "200" ]; then | ||
| info "Deleted robot account (ID: ${ROBOT_ID})" | ||
| else | ||
| info "Failed to delete robot account (HTTP $HTTP_CODE)" |
Contributor
There was a problem hiding this comment.
Harbor DELETE returns 204, not 200 — cleanup reports false failure.
The Harbor API returns HTTP 204 No Content on successful robot account deletion. Checking for 200 means the script prints "Failed to delete" even on success.
🐛 Proposed fix
- if [ "$HTTP_CODE" = "200" ]; then
+ if [ "$HTTP_CODE" = "200" ] || [ "$HTTP_CODE" = "204" ]; then📝 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
| HTTP_CODE=$(curl -sk -o /dev/null -w '%{http_code}' -X DELETE \ | |
| -u "${HARBOR_USERNAME}:${HARBOR_PASSWORD}" \ | |
| "${HARBOR_URL}/api/v2.0/robots/${ROBOT_ID}") | |
| if [ "$HTTP_CODE" = "200" ]; then | |
| info "Deleted robot account (ID: ${ROBOT_ID})" | |
| else | |
| info "Failed to delete robot account (HTTP $HTTP_CODE)" | |
| HTTP_CODE=$(curl -sk -o /dev/null -w '%{http_code}' -X DELETE \ | |
| -u "${HARBOR_USERNAME}:${HARBOR_PASSWORD}" \ | |
| "${HARBOR_URL}/api/v2.0/robots/${ROBOT_ID}") | |
| if [ "$HTTP_CODE" = "200" ] || [ "$HTTP_CODE" = "204" ]; then | |
| info "Deleted robot account (ID: ${ROBOT_ID})" | |
| else | |
| info "Failed to delete robot account (HTTP $HTTP_CODE)" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@website/content/docs/quickstart.md` around lines 2243 - 2249, The script
checks HTTP_CODE for "200" when deleting a Harbor robot but Harbor returns 204
on success; update the deletion-success check in the block that sets HTTP_CODE
(the curl call and subsequent if) to treat 204 as success (e.g., accept "204" or
any 2xx) and keep the existing info logging that references ROBOT_ID and uses
the info function so successful deletions no longer report "Failed to delete
robot account."
Port 5000 is the industry standard for container registries. Updated the default Zot config and Ground Control default satellite config from 8585 to 5000. Added end-to-end demo automation scripts for the Harbor Satellite quickstart. Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
master-demo.sh (1)
52-57:remote_bg()is defined but never called — remove the dead code.
ssh -fis used for all background remote processes (lines 835–837 and 976–978).remote_bgis never invoked anywhere in the script.♻️ Proposed fix
-remote_bg() { - # Run a command on the remote host in the background. - # Redirect stdin, stdout, stderr and disown so SSH can exit cleanly. - ssh -o StrictHostKeyChecking=no -o ConnectTimeout=10 \ - "${SAT_USER}@${SAT_IP}" "nohup $1 </dev/null > $2 2>&1 & disown; sleep 1" -} -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@master-demo.sh` around lines 52 - 57, Remove the unused dead function remote_bg: delete the entire remote_bg() function definition (the SSH nohup wrapper) since SSH -f is used elsewhere and no callers reference remote_bg; after removal, run a search for the symbol remote_bg to confirm there are no remaining references and update any comments if they mention this helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@find-pi.sh`:
- Around line 19-20: The script calculates GATEWAY and SUBNET_PREFIX from IFACE
but doesn't guard for an empty GATEWAY, which makes subsequent ping/ARP steps
behave incorrectly; add a validation immediately after computing GATEWAY and
SUBNET_PREFIX (check if GATEWAY is non-empty and a valid IPv4 address and that
SUBNET_PREFIX is non-empty) and if the check fails, print a clear error message
and exit with a non-zero status (or optionally fall back to auto-detecting a
default route via `ip route show default` without an interface). Update the
logic around the variables GATEWAY and SUBNET_PREFIX so any loops or grep/arp
commands are skipped when the guard fails to avoid matching everything.
In `@pre-demo-var-check.sh`:
- Around line 138-153: The SSH calls that set REMOTE_UID and REMOTE_ARCH can
hang waiting for password prompts; update the ssh invocations used when
assigning REMOTE_UID and REMOTE_ARCH (the two lines that run ssh -o
ConnectTimeout=5 "${SAT_USER}@${SAT_IP}" ...) to include non-interactive auth by
adding -o BatchMode=yes so SSH fails immediately instead of prompting; keep the
existing ConnectTimeout and error fallback logic intact so the variables still
resolve to "unknown" on failure.
In `@website/content/docs/quickstart.md`:
- Around line 385-386: The docs currently suggest running "chmod 777" on the
SPIRE data volume which exposes key material; update the instruction in
website/content/docs/quickstart.md to use a minimum-privilege alternative such
as "chmod 700" on the mounted volume or chown the volume to the SPIRE server
container UID (use 1000 as the example or replace 1000 with the actual container
UID if different) so the CA private key and SVID data are not
world-readable/writable.
---
Duplicate comments:
In `@find-pi.sh`:
- Line 39: The variable 'found' is declared but never used in find-pi.sh; either
remove the unused declaration ("found=0") or wire it into the intended logic
(e.g., set and check 'found' where primes are detected). Locate the 'found'
variable in the script and either delete that line or update the surrounding
loop/condition that should toggle/read 'found' so the variable is actually used.
In `@master-demo.sh`:
- Line 19: The variable SAT_PASS is declared but never used; either remove the
unused SAT_PASS declaration or make it actually used/exported where intended
(e.g., export SAT_PASS and reference it in the script’s authentication logic).
Locate the SAT_PASS assignment in the script and either delete that line or wire
the variable into the code that requires the password (or read it from an
environment variable) so the declaration is not unused.
- Around line 460-461: The script currently makes private key files
world-readable with the line "chmod 644 certs/*.key certs/*.crt"; change this so
private keys are not world-readable (e.g., use "chmod 600 certs/*.key" or "chmod
640 certs/*.key" depending on required group access) while keeping public certs
world-readable (e.g., "chmod 644 certs/*.crt"); update the chmod invocation in
the block containing "rm -f certs/*.csr certs/*.ext certs/*.srl" and "chmod 644
certs/*.key certs/*.crt" to apply the stricter permissions to certs/*.key and
leave certs/*.crt as 644.
In `@website/content/docs/quickstart.md`:
- Around line 666-670: The manual example invocation for ./harbor-satellite is
missing the flags --harbor-registry-url and --use-unsecure; update the command
shown by adding --harbor-registry-url http://0.0.0.0:5000 and --use-unsecure so
the ./harbor-satellite run example matches the automation script and the "Local
Zot registry" note; locate the example invocation of ./harbor-satellite in the
quickstart document and add those two flags to the command line.
- Line 146: The shell command in the Quickstart markdown makes private key files
world-readable by using "chmod 644 certs/*.key certs/*.crt"; update the
instructions so private keys use a restrictive mode (e.g., chmod 600 on
certs/*.key) while keeping public certs readable (e.g., chmod 644 on
certs/*.crt), and ensure the docs explicitly run two separate chmods (one for
*.key, one for *.crt) so private keys are not world-readable.
- Around line 442-447: The markdown callout block starting with {{< callout
type="info" >}} contains a list that needs a blank line before it to satisfy the
linter; edit the callout (the block including {{< callout type="info" >}} and
the lines listing `certs/ca.crt`, `certs/us-east-1.crt`, and
`certs/us-east-1.key`) and insert a single empty line between the descriptive
sentence "You will need the following files from the cloud server (generated in
Step 1.2):" and the subsequent bullet list so the list is properly separated and
the linter warning is resolved.
---
Nitpick comments:
In `@master-demo.sh`:
- Around line 52-57: Remove the unused dead function remote_bg: delete the
entire remote_bg() function definition (the SSH nohup wrapper) since SSH -f is
used elsewhere and no callers reference remote_bg; after removal, run a search
for the symbol remote_bg to confirm there are no remaining references and update
any comments if they mention this helper.
Comment on lines
+19
to
+20
| GATEWAY=$(ip -4 route show default dev "$IFACE" | awk '{print $3}' | head -1) | ||
| SUBNET_PREFIX="${GATEWAY%.*}" |
Contributor
There was a problem hiding this comment.
No guard for an empty GATEWAY when IFACE is passed as an argument.
If the supplied interface has no default route, GATEWAY and SUBNET_PREFIX are empty, causing ping ".1" … ".254" (all silently discarded), and arp -an | grep "" matches every entry — making the gateway/local-IP filters ineffective.
🛡️ Proposed fix
GATEWAY=$(ip -4 route show default dev "$IFACE" | awk '{print $3}' | head -1)
SUBNET_PREFIX="${GATEWAY%.*}"
+
+if [ -z "$GATEWAY" ]; then
+ echo "Error: could not determine gateway for interface $IFACE. Does it have a default route?"
+ exit 1
+fi📝 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
| GATEWAY=$(ip -4 route show default dev "$IFACE" | awk '{print $3}' | head -1) | |
| SUBNET_PREFIX="${GATEWAY%.*}" | |
| GATEWAY=$(ip -4 route show default dev "$IFACE" | awk '{print $3}' | head -1) | |
| SUBNET_PREFIX="${GATEWAY%.*}" | |
| if [ -z "$GATEWAY" ]; then | |
| echo "Error: could not determine gateway for interface $IFACE. Does it have a default route?" | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@find-pi.sh` around lines 19 - 20, The script calculates GATEWAY and
SUBNET_PREFIX from IFACE but doesn't guard for an empty GATEWAY, which makes
subsequent ping/ARP steps behave incorrectly; add a validation immediately after
computing GATEWAY and SUBNET_PREFIX (check if GATEWAY is non-empty and a valid
IPv4 address and that SUBNET_PREFIX is non-empty) and if the check fails, print
a clear error message and exit with a non-zero status (or optionally fall back
to auto-detecting a default route via `ip route show default` without an
interface). Update the logic around the variables GATEWAY and SUBNET_PREFIX so
any loops or grep/arp commands are skipped when the guard fails to avoid
matching everything.
Comment on lines
+138
to
+153
| REMOTE_UID=$(ssh -o ConnectTimeout=5 "${SAT_USER}@${SAT_IP}" "id -u" 2>/dev/null || echo "unknown") | ||
| if [ "$REMOTE_UID" = "$SAT_UID" ]; then | ||
| pass "Remote UID matches ($SAT_UID)" | ||
| elif [ "$REMOTE_UID" = "unknown" ]; then | ||
| warn "Could not verify remote UID" | ||
| else | ||
| warn "Remote UID is $REMOTE_UID, expected $SAT_UID" | ||
| fi | ||
|
|
||
| # Check edge architecture | ||
| REMOTE_ARCH=$(ssh -o ConnectTimeout=5 "${SAT_USER}@${SAT_IP}" "uname -m" 2>/dev/null || echo "unknown") | ||
| if [ "$REMOTE_ARCH" != "unknown" ]; then | ||
| pass "Edge architecture: $REMOTE_ARCH" | ||
| else | ||
| warn "Could not detect edge architecture" | ||
| fi |
Contributor
There was a problem hiding this comment.
SSH calls on lines 138 and 148 can hang if key-based auth isn't available.
ConnectTimeout=5 only limits TCP connection time; SSH still reads a password from /dev/tty during authentication (unaffected by 2>/dev/null or || echo "unknown"). Adding -o BatchMode=yes makes SSH fail immediately instead of prompting.
🛡️ Proposed fix
-REMOTE_UID=$(ssh -o ConnectTimeout=5 "${SAT_USER}@${SAT_IP}" "id -u" 2>/dev/null || echo "unknown")
+REMOTE_UID=$(ssh -o BatchMode=yes -o ConnectTimeout=5 "${SAT_USER}@${SAT_IP}" "id -u" 2>/dev/null || echo "unknown")
...
-REMOTE_ARCH=$(ssh -o ConnectTimeout=5 "${SAT_USER}@${SAT_IP}" "uname -m" 2>/dev/null || echo "unknown")
+REMOTE_ARCH=$(ssh -o BatchMode=yes -o ConnectTimeout=5 "${SAT_USER}@${SAT_IP}" "uname -m" 2>/dev/null || echo "unknown")📝 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
| REMOTE_UID=$(ssh -o ConnectTimeout=5 "${SAT_USER}@${SAT_IP}" "id -u" 2>/dev/null || echo "unknown") | |
| if [ "$REMOTE_UID" = "$SAT_UID" ]; then | |
| pass "Remote UID matches ($SAT_UID)" | |
| elif [ "$REMOTE_UID" = "unknown" ]; then | |
| warn "Could not verify remote UID" | |
| else | |
| warn "Remote UID is $REMOTE_UID, expected $SAT_UID" | |
| fi | |
| # Check edge architecture | |
| REMOTE_ARCH=$(ssh -o ConnectTimeout=5 "${SAT_USER}@${SAT_IP}" "uname -m" 2>/dev/null || echo "unknown") | |
| if [ "$REMOTE_ARCH" != "unknown" ]; then | |
| pass "Edge architecture: $REMOTE_ARCH" | |
| else | |
| warn "Could not detect edge architecture" | |
| fi | |
| REMOTE_UID=$(ssh -o BatchMode=yes -o ConnectTimeout=5 "${SAT_USER}@${SAT_IP}" "id -u" 2>/dev/null || echo "unknown") | |
| if [ "$REMOTE_UID" = "$SAT_UID" ]; then | |
| pass "Remote UID matches ($SAT_UID)" | |
| elif [ "$REMOTE_UID" = "unknown" ]; then | |
| warn "Could not verify remote UID" | |
| else | |
| warn "Remote UID is $REMOTE_UID, expected $SAT_UID" | |
| fi | |
| # Check edge architecture | |
| REMOTE_ARCH=$(ssh -o BatchMode=yes -o ConnectTimeout=5 "${SAT_USER}@${SAT_IP}" "uname -m" 2>/dev/null || echo "unknown") | |
| if [ "$REMOTE_ARCH" != "unknown" ]; then | |
| pass "Edge architecture: $REMOTE_ARCH" | |
| else | |
| warn "Could not detect edge architecture" | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pre-demo-var-check.sh` around lines 138 - 153, The SSH calls that set
REMOTE_UID and REMOTE_ARCH can hang waiting for password prompts; update the ssh
invocations used when assigning REMOTE_UID and REMOTE_ARCH (the two lines that
run ssh -o ConnectTimeout=5 "${SAT_USER}@${SAT_IP}" ...) to include
non-interactive auth by adding -o BatchMode=yes so SSH fails immediately instead
of prompting; keep the existing ConnectTimeout and error fallback logic intact
so the variables still resolve to "unknown" on failure.
Comment on lines
+385
to
+386
| docker run --rm -v gc_spire-server-data:/data alpine chmod 777 /data | ||
| ``` |
Contributor
There was a problem hiding this comment.
chmod 777 on the SPIRE data volume exposes key material to all host processes.
SPIRE stores its CA private key, agent keys, and SVID data under this directory. Using chmod 777 makes those files readable/writable by any host UID. chmod 700 (or matching the UID of the SPIRE container user) is the minimum-privilege alternative.
🛡️ Proposed fix
-docker run --rm -v gc_spire-server-data:/data alpine chmod 777 /data
+docker run --rm -v gc_spire-server-data:/data alpine sh -c 'chmod 700 /data && chown 1000:1000 /data'Replace 1000 with the actual UID of the SPIRE server container's process if it differs.
📝 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
| docker run --rm -v gc_spire-server-data:/data alpine chmod 777 /data | |
| ``` | |
| docker run --rm -v gc_spire-server-data:/data alpine sh -c 'chmod 700 /data && chown 1000:1000 /data' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@website/content/docs/quickstart.md` around lines 385 - 386, The docs
currently suggest running "chmod 777" on the SPIRE data volume which exposes key
material; update the instruction in website/content/docs/quickstart.md to use a
minimum-privilege alternative such as "chmod 700" on the mounted volume or chown
the volume to the SPIRE server container UID (use 1000 as the example or replace
1000 with the actual container UID if different) so the CA private key and SVID
data are not world-readable/writable.
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.
Description
to clone the repo
{{< details >}}shortcode to the docs siteAdditional context
Summary by cubic
Overhauled the quickstart to be fully self-contained with inline configs and Docker Compose, and switched SPIRE attestation from join-token to x509pop with the edge agent attesting before satellite registration. Standardized the satellite’s local Zot registry on port 5000, added end-to-end demo scripts, and improved docs UX.
New Features
Migration
Written for commit dd835dd. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation
Changes