-
Notifications
You must be signed in to change notification settings - Fork 14
Ait 65906 #125
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughAdds a new documentation guide for cleaning the platform's built-in image registry (MinIO) and bumps the dev dependency Changes
Sequence Diagram(s)(omitted — changes are documentation and a devDependency bump) Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@docs/en/solutions/ait/how-to-cleanup-platform-imageregistry.md`:
- Line 32: Update the "**Backup Custom Images**" line to clarify whether Part 1
(the MinIO directory backup) already includes custom/non-platform images or
provide explicit steps to selectively back up them; specifically, under the
"**Backup Custom Images**" heading either (a) state that the Part 1 MinIO backup
covers custom images and no further action is needed, or (b) add short selective
backup instructions (e.g., how to locate and export custom images from MinIO or
the registry UI/CLI) and a reference to Part 1 for full backups so readers know
exactly how to preserve non-platform images before cleanup.
- Around line 175-189: Update the docs to explain the two upload modes used with
res/upload.sh ("all" vs "necessary") by describing what each mode uploads (e.g.,
complete core image set vs minimal required images), when to choose each (full
install vs reduced bandwidth/airgapped installs), and whether both must be run
in sequence; also replace the plaintext credential example (REGISTRY, USERNAME,
PASSWORD) with a security note advising use of environment variables or secure
secret managers, demonstrating secure input methods (e.g., read -s or CI secret
injection), warning about shell history, and instructing to unset/clear the
USERNAME and PASSWORD variables after use, while referencing the symbols
REGISTRY, USERNAME, PASSWORD and the script/res/upload.sh command so readers can
locate and update the exact lines.
- Around line 224-243: The loop that uploads plugins (uses plugin_dir, violet
push and Platform_URL/Platform_USER/Platform_PASSWORD) lacks error handling and
stores credentials in plain variables; update the script to check the exit
status of each violet push, log success or failure for each plugin, and
optionally stop or collect failures for a summary if a push fails, and replace
plain-text credentials with secure practices (read credentials from environment
variables or a secrets store, or prompt/read -s) and ensure any sensitive values
aren’t echoed or written to disk.
- Around line 254-263: Update the test commands that use placeholders
(${REGISTRY}/acp/plugin-name:version and ${REGISTRY}/operator-name:version) by
instructing readers how to find real image names: reference the recorded
operators and plugins from Part 1, explain how to extract image names and tags
from the ARTIFACT or MODULE fields (e.g., parse the image URL or repository:tag
from those fields), and replace the placeholders with a concrete example (use an
actual plugin/operator name and tag from the example in Part 1) so the nerdctl
pull and verify steps are reproducible.
- Around line 53-68: Clarify where ${ip}-minio.tar is written and how to collect
it: state that the tar command writes the backup to the current working
directory unless a full path is given and recommend writing to a dedicated
backup mount (e.g., /var/backups/minio or an NFS/S3-backed mount) instead of
CWD; add instructions to verify free space before running the tar (e.g., check
df -h on the node hosting /cpaas/minio) and to copy each node's ${ip}-minio.tar
to a central backup host or object store using scp/rsync or an S3 client,
including a short note to run the tar/verify steps on all three control plane
nodes and confirm successful collection of all three archives to the safe
storage location.
- Around line 154-162: The doc uses placeholders and assumptions without
verification; update the section around the kubectl exec example to show how to
discover the actual <registry-namespace> and <registry-minio-pod> (add a brief
note to list pods in common namespaces and mention common naming patterns like
"registry" or "minio"), add a pre-check for the referenced file used by "source
/etc/config/minio.env" (verify the env file exists and fail with a clear message
if not), and add a quick verification step after "mc --insecure alias set minio"
to ensure the alias and credentials worked and that the registry bucket
(minio/registry) exists before running "mc --insecure rm --recursive --force
minio/registry/".
- Around line 201-203: Replace the brittle test line that pulls "nerdctl pull
${REGISTRY}/acp/core:latest" with a reference to a specific, known image tag
produced by the upload step (for example use the exact core image variable
emitted by the upload script such as the core image tag or name), and add a
short note telling users how to discover available images in the registry (e.g.,
via a registry image-listing tool or the upload script’s output) so the doc
always points to an image that exists after the upload.
🧹 Nitpick comments (2)
docs/en/solutions/ait/how-to-cleanup-platform-imageregistry.md (2)
74-76: Consider improving command readability.The kubectl command is quite long and could be more maintainable if split across multiple lines using bash line continuation (
\). This would make it easier to read and modify.♻️ Proposed refactor for readability
-kubectl get operatorviews -A -o custom-columns='CLUSTER:.metadata.namespace,NAME:.metadata.name,PHASE:.status.operatorStatus.phase,ARTIFACT:.status.operatorStatus.installation.artifactName,INSTALLED_CSV:.status.operatorStatus.installation.subscription.installedCSV,DISPLAY_NAME:.status.packageManifestStatus.channels[0].currentCSVDesc.displayName' | awk 'NR==1 || ($3 == "Installed")' +kubectl get operatorviews -A -o custom-columns=\ +'CLUSTER:.metadata.namespace,'\ +'NAME:.metadata.name,'\ +'PHASE:.status.operatorStatus.phase,'\ +'ARTIFACT:.status.operatorStatus.installation.artifactName,'\ +'INSTALLED_CSV:.status.operatorStatus.installation.subscription.installedCSV,'\ +'DISPLAY_NAME:.status.packageManifestStatus.channels[0].currentCSVDesc.displayName' \ +| awk 'NR==1 || ($3 == "Installed")'
99-124: Consider documenting the go-template logic.The go-template is complex and handles lifecycle filtering and version aggregation. Consider adding inline comments in the command or a separate explanation of what the template does (filters for agnostic/aligned types, aggregates installed versions per cluster). This would make future maintenance easier.
| # Test pulling a core platform image | ||
| nerdctl pull ${REGISTRY}/acp/core:latest | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use a specific, known image for verification.
The test command pulls acp/core:latest, but this tag might not exist after the upload. Consider:
- Specifying an actual core image that's guaranteed to exist after running the upload script
- Providing guidance on how to find available images to test with
- Using an image name/tag that matches what was actually uploaded
🤖 Prompt for AI Agents
In `@docs/en/solutions/ait/how-to-cleanup-platform-imageregistry.md` around lines
201 - 203, Replace the brittle test line that pulls "nerdctl pull
${REGISTRY}/acp/core:latest" with a reference to a specific, known image tag
produced by the upload step (for example use the exact core image variable
emitted by the upload script such as the core image tag or name), and add a
short note telling users how to discover available images in the registry (e.g.,
via a registry image-listing tool or the upload script’s output) so the doc
always points to an image that exists after the upload.
jiazhiguang
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
|
/lgtm |
Summary by CodeRabbit
Documentation
Chores