docs: restructure documentation with updated command references#423
Open
istein1 wants to merge 3 commits into
Open
docs: restructure documentation with updated command references#423istein1 wants to merge 3 commits into
istein1 wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
✅ Files skipped from review due to trivial changes (15)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds comprehensive documentation for Crane, a Kubernetes migration tool, including user guides (installation, CLI commands), conceptual documentation (plugins, resource compatibility, multi-stage pipeline), and developer resources (architecture, setup, testing, plugin development, contribution guidelines). All changes are documentation-only. ChangesCrane Documentation Suite
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Test Coverage ReportTotal: 46.9% Per-package coverage
Full function-level detailsPosted by CI |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/development/plugin-development.md (1)
181-188: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winBest practices contradict the code examples.
Best practice
#2states "Defensive: Check if fields exist before removing them," but this principle should also apply to add operations. The Go and Bash examples earlier in the document add labels without checking if/metadata/labelsexists, which violates defensive programming and will cause patches to fail.Consider adding:
- "Check if parent objects exist before adding fields" as an explicit best practice
- Or revise
#2to: "Defensive: Check if fields and parent objects exist before modifying them"♻️ Proposed expanded best practices
## Best Practices 1. **Idempotent**: Running the plugin multiple times should produce the same result -2. **Defensive**: Check if fields exist before removing them +2. **Defensive**: Check if fields and parent objects exist before modifying them 3. **Focused**: Each plugin should handle one concern 4. **Documented**: Include usage instructions and examples 5. **Tested**: Cover edge cases (missing fields, different resource types)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/development/plugin-development.md` around lines 181 - 188, Update the Best Practices section to ensure consistency with the examples: change item `#2` (Defensive) to explicitly mention both fields and parent objects (e.g., "Defensive: Check that fields and parent objects exist before modifying them") and/or add a new bullet saying "Check if parent objects exist before adding fields"; also reference the problematic path used in examples (/metadata/labels) and mention the Go and Bash examples so readers know where to apply the defensive check.
🧹 Nitpick comments (1)
docs/development/plugin-development.md (1)
92-98: ⚡ Quick winAdd nil check for defensive programming.
While the key existence check at line 93 prevents adding a remove patch for non-existent annotations, if
GetAnnotations()returnsnil, the map access could panic. Consider adding a nil check.♻️ Proposed defensive nil check
// Example: remove a specific annotation annotations := resource.GetAnnotations() - if _, ok := annotations["source-cluster-only"]; ok { + if annotations != nil { + if _, ok := annotations["source-cluster-only"]; ok { + patches = append(patches, PatchOp{ + Op: "remove", + Path: "/metadata/annotations/source-cluster-only", + }) + } + } +} +``` Or more concisely: ```diff // Example: remove a specific annotation annotations := resource.GetAnnotations() - if _, ok := annotations["source-cluster-only"]; ok { + if len(annotations) > 0 { + if _, ok := annotations["source-cluster-only"]; ok { + patches = append(patches, PatchOp{ + Op: "remove", + Path: "/metadata/annotations/source-cluster-only", + }) + } - } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/development/plugin-development.md` around lines 92 - 98, The code reads annotations := resource.GetAnnotations() and then does a key existence check which could panic if annotations is nil; add a defensive nil check before accessing the map (e.g., ensure annotations != nil) and only append the PatchOp when annotations is non-nil and the "source-cluster-only" key exists; update the block around resource.GetAnnotations(), the annotations variable and the patches = append(patches, PatchOp{...}) call to perform this conditional check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/commands/transfer-pvc.md`:
- Around line 36-37: The table currently marks `--ingress-class` and
`--subdomain` as optional but the Endpoint Options text later says they are
required for nginx-ingress, causing ambiguity; update the flag table entries for
`--ingress-class` and `--subdomain` to read "Required when endpoint is
nginx-ingress" (or similar) and/or add a clarifying sentence in the "Endpoint
Options" section that explicitly states these flags are conditionally required
when the endpoint is set to `nginx-ingress` and optional otherwise so both
places match `--ingress-class` and `--subdomain`.
In `@docs/development/plugin-development.md`:
- Around line 115-124: The example patch fails when /metadata/labels doesn't
exist; update the bash plugin to read the incoming resource from stdin, inspect
whether .metadata.labels exists (e.g., using jq), and emit a JSONPatch that
first ensures labels exists (an "add" to "/metadata/labels" with {} if missing)
before adding "/metadata/labels/environment" — alternatively emit two operations
in order (add "/metadata/labels" -> {} then add "/metadata/labels/environment"
-> "production") when labels is absent; reference the script that produces the
static JSONPatch and the JSON Pointer "/metadata/labels/environment" when making
this conditional change.
- Around line 85-89: The JSONPatch add at PatchOp with Path
"/metadata/labels/migrated-by" will fail when the parent "/metadata/labels" is
missing; update the patch sequence around the PatchOp to first ensure the labels
map exists (either prepend a test+add pattern that tests for "/metadata/labels"
and adds an empty map if absent, or add an explicit add for "/metadata/labels"
before adding "/metadata/labels/migrated-by"), or switch the operation to
"replace" only if you guarantee labels always exist; modify the PatchOp list to
implement one of these defensive approaches so the add to
"/metadata/labels/migrated-by" cannot error on resources without labels.
In `@docs/multistage-pipeline.md`:
- Line 166: Resolve the contradiction about stage-specific applies: determine
whether the CLI supports the --stage flag for the "apply" command; if the flag
is implemented, update the statement that "The apply command does not support
stage-specific flags" to reflect that apply can target a single stage (mention
the --stage flag) and document its behavior and limitations in the "apply"
section; if the flag is not implemented, remove the suggestion to run `crane
apply --stage <stage>` and replace the troubleshooting note with an alternative
(for example, suggest isolating the stage by running the stage-specific command
that does exist, or using `crane run --stage <stage>` if that is the supported
command) and ensure both places consistently state the correct behavior of
"apply" and the correct command to isolate a failing stage.
---
Outside diff comments:
In `@docs/development/plugin-development.md`:
- Around line 181-188: Update the Best Practices section to ensure consistency
with the examples: change item `#2` (Defensive) to explicitly mention both fields
and parent objects (e.g., "Defensive: Check that fields and parent objects exist
before modifying them") and/or add a new bullet saying "Check if parent objects
exist before adding fields"; also reference the problematic path used in
examples (/metadata/labels) and mention the Go and Bash examples so readers know
where to apply the defensive check.
---
Nitpick comments:
In `@docs/development/plugin-development.md`:
- Around line 92-98: The code reads annotations := resource.GetAnnotations() and
then does a key existence check which could panic if annotations is nil; add a
defensive nil check before accessing the map (e.g., ensure annotations != nil)
and only append the PatchOp when annotations is non-nil and the
"source-cluster-only" key exists; update the block around
resource.GetAnnotations(), the annotations variable and the patches =
append(patches, PatchOp{...}) call to perform this conditional check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d1a7bd49-ea9b-46bb-800e-1da548d8b32d
📒 Files selected for processing (17)
CONTRIBUTING.mdREADME.mddocs/README.mddocs/commands/apply.mddocs/commands/export.mddocs/commands/transfer-pvc.mddocs/commands/transform.mddocs/commands/validate.mddocs/development/README.mddocs/development/architecture.mddocs/development/plugin-development.mddocs/development/setup.mddocs/development/testing.mddocs/installation.mddocs/multistage-pipeline.mddocs/plugins.mddocs/resource-compatibility.md
aufi
approved these changes
Jun 1, 2026
aufi
left a comment
aufi
left a comment
Contributor
There was a problem hiding this comment.
There are some things that could be updated (now or later), I left few comments, but overall looks good to me.
|
|
||
| # Create a new pass-through stage for manual editing | ||
| # Stage name NOT ending with "Plugin" creates empty pass-through | ||
| crane transform --stage 40_CustomManualEdits |
Contributor
There was a problem hiding this comment.
There are some updates of this in open PRs #395, but I will send an update to docs when the PR gets merged.
| @@ -0,0 +1,523 @@ | |||
| # Multi-Stage Kustomize Transform Pipeline | |||
Contributor
There was a problem hiding this comment.
This looks quite close to transform command doc, maybe belongs partially there and partially to some example migration scenario, but could be updated later, not blocking this PR.
|
|
||
| The [konveyor/crane-plugins](https://github.com/konveyor/crane-plugins) repository contains community-contributed plugins based on experience from real-world Kubernetes migrations. | ||
|
|
||
| ### OpenShift Plugin |
Contributor
There was a problem hiding this comment.
Thinking if we want mention that those community plugins (including openshift) are not fully maintained at this moment.
| - `feat:` for new features | ||
| - `refactor:` for code restructuring | ||
| - `test:` for test additions | ||
| - `docs:` for documentation |
Contributor
There was a problem hiding this comment.
This is -1 from me (mentioned on previous PR #310 (review)), but not a blocker if thers are OK with this.
Reorganize documentation into docs/ with commands/ and development/ subdirectories. Add CONTRIBUTING.md for contributor onboarding. New structure: - docs/commands/ — reference for all 5 commands (export, transform, apply, validate, transfer-pvc) - docs/development/ — architecture, setup, testing, plugin development - docs/multistage-pipeline.md — multi-stage Kustomize pipeline guide - docs/plugins.md — plugin overview - docs/resource-compatibility.md — resource type compatibility matrix - docs/installation.md — installation and prerequisites Content updated to reflect current codebase: - crane apply uses embedded kustomize (no kubectl dependency) - crane apply supports --skip-cluster-scoped and --kustomize-args flags - crane validate supports offline mode via --api-resources - crane export documents CRD collection and _cluster/ subdirectory Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Update all doc files to reflect 35 commits merged to main since the original PR: --stage flag replaced with positional arguments, --overwrite added to export/apply/validate, transform default behavior changed to create stages for all plugins, --skip-plugins and --instructions-file flags added, --api-resources mutual exclusion expanded, and --plugin-priorities/--stage-name/--plugin-name flags removed. Address reviewer feedback: use verb-first commit/PR titles instead of conventional commit prefixes (aufi), add community plugin maintenance note (aufi), fix conditional Required status for transfer-pvc nginx flags (CodeRabbit), add defensive JSONPatch parent-object checks in plugin examples (CodeRabbit), add nil check for GetAnnotations (CodeRabbit), and resolve apply stage-specific flag contradiction (CodeRabbit). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
istein1
force-pushed
the
docs/restructure-v2
branch
from
5b76ad6 to
a677530
Compare
…igration - Fix stale ApplyFinalStage() reference in multistage-pipeline.md (only ApplySingleStage and ApplyMultiStage exist) - Fix misleading "Single Stage Mode" heading (default creates stages for all plugins) - Fix transform.md title to "crane transform" for consistency - Add capture-api-surface.sh script to validate.md offline validation section (from PR migtools#321 suggestion) - Document admin vs non-admin migration in export.md (graceful Forbidden handling, CRD skip behavior) - Enhance --skip-cluster-scoped docs in apply.md with non-admin pipeline example Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Member
Author
|
Rebased on latest main and pushed two follow-up commits addressing all review feedback: First commit covers the bulk of the review comments — replaced all stale --stage flag references with positional args, added missing flags (--overwrite, Second commit fixes remaining issues found during re-review — removed the non-existent ApplyFinalStage() from the API reference, fixed the misleading "Single Stage All docs are now current with the 35 commits that landed on main since the original PR. More details on the 2 commits: Commit 1: a677530 — Update docs for latest CLI changes and review feedback Files (9): CONTRIBUTING.md, docs/commands/apply.md, docs/commands/export.md, docs/commands/transfer-pvc.md, docs/commands/transform.md, docs/commands/validate.md,
Commit 2: 521edcb — Fix review issues, add API surface script, document non-admin migration Files (5): docs/commands/apply.md, docs/commands/export.md, docs/commands/transform.md, docs/commands/validate.md, docs/multistage-pipeline.md
|
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
2 participants
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
commands/,development/, and flat user-facing docs underdocs/CONTRIBUTING.mdat root for GitHub integration and contributor onboardingREADME.mdwith documentation links sectionNew structure
Content updates reflecting current codebase
crane applyuses embedded kustomize via krusty API (nokubectldependency)crane applydocuments--skip-cluster-scopedand--kustomize-argsflagscrane validatedocuments offline validation mode via--api-resourcescrane exportdocuments CRD collection behavior and_cluster/subdirectoryTest plan
kubectl kustomizein descriptions of internal behaviorgo build ./...passes (docs-only change, no code impact)🤖 Generated with Claude Code
Summary by CodeRabbit
export,transform,apply,validate, andtransfer-pvc, including detailed examples and flag descriptions.crane applybehavior as running embedded kustomize.