docs: rename Skyhook to NodeWright in remaining docs + chart README - #370
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughUpdated Helm chart and project documentation to replace Skyhook terminology with NodeWright. CLI guidance now describes NodeWright annotations, processing controls, state updates, deployment-policy resets, and recovery procedures. Webhook, Kyverno, metrics, release-process, versioning, resource-management, runtime, and uninstall documents were likewise updated. Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@chart/README.md`:
- Line 36: Update the runtimeRequiredTaint description in the chart
documentation to use the grammatically correct terms “assumed” and “once,”
leaving the surrounding meaning and configuration value unchanged.
- Around line 8-9: Correct the grammar in the Autoscaling benefit bullet in
README.md: change “NodeWright as feature,” “you nodes,” and “before then enter”
to clear, grammatically correct wording while preserving the intended meaning
about preparing added nodes before they join the cluster.
In `@docs/cli.md`:
- Around line 80-81: Update the documentation for the CLI pause, resume,
disable, and enable commands to match the preflight behavior: state that older
v0.7.x operators fail fast when only skyhook.nvidia.com is served and require an
upgrade, or direct legacy users to edit the Skyhook resource rather than a
NodeWright spec. Use “recognise” consistently with the surrounding
documentation.
In `@docs/README.md`:
- Around line 11-12: Update the prose description associated with “Providing
Secrets to Packages” to say “NodeWright-managed packages” instead of
“Skyhook-managed packages.” Leave the ordering_of_skyhooks.md filename, Skyhook
namespaces, and agent image references unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 1e4c3d94-2640-40c2-9ae5-3db9137314f2
📒 Files selected for processing (8)
chart/README.mddocs/README.mddocs/cli.mddocs/designs/webhook-bootstrap-lease.mddocs/kyverno/README.mddocs/metrics/README.mddocs/release-process.mddocs/versioning.md
a74414b to
c514a1d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/cli.md`:
- Line 64: Update the historical v0.7.x section in docs/cli.md to refer to the
legacy resource as “Skyhook” rather than “NodeWright,” while preserving the
existing explanation that pausing and disabling were controlled through spec
fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: ba5d1036-717b-4d92-9ee2-44c45ba9d559
📒 Files selected for processing (8)
chart/README.mddocs/README.mddocs/cli.mddocs/designs/webhook-bootstrap-lease.mddocs/kyverno/README.mddocs/metrics/README.mddocs/release-process.mddocs/versioning.md
c514a1d to
a6889af
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/cli.md (1)
64-64: 🎯 Functional Correctness | 🟠 MajorKeep the v0.7.x resource name as
Skyhook.Operator v0.7.x and earlier used the legacy
Skyhookkind and spec fields. Calling that historical resource aNodeWrightconflicts with the compatibility guidance and can send upgrade users to edit the wrong resource. RestoreSkyhookfor this legacy section.Based on learnings, v0.7.x guidance must refer to the legacy Skyhook kind/spec rather than NodeWright.
🤖 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/cli.md` at line 64, Update the legacy v0.7.x-and-earlier section in docs/cli.md to refer to the resource as Skyhook, including its kind and spec fields, instead of NodeWright; leave current-version NodeWright guidance unchanged.Source: Learnings
🤖 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 `@chart/README.md`:
- Line 36: Update the controllerManager.manager.env.runtimeRequiredTaint
documentation to state that pods tolerate the per-node taint, while the
controller removes it independently for each node only after every
runtimeRequired: true NodeWright targeting that node has completed there;
completion on other nodes must not delay removal. Align the wording with
docs/runtime_required.md.
---
Duplicate comments:
In `@docs/cli.md`:
- Line 64: Update the legacy v0.7.x-and-earlier section in docs/cli.md to refer
to the resource as Skyhook, including its kind and spec fields, instead of
NodeWright; leave current-version NodeWright guidance unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 160c35a1-8ad3-4eb8-ad23-f8a3bc7d55b0
📒 Files selected for processing (11)
chart/README.mddocs/README.mddocs/cli.mddocs/designs/webhook-bootstrap-lease.mddocs/kyverno/README.mddocs/metrics/README.mddocs/release-process.mddocs/resource_management.mddocs/runtime_required.mddocs/uninstall.mddocs/versioning.md
Prose-only Skyhook->NodeWright rename across the rest of docs/ and the chart README, following the API rename. The swap runs outside code fences and inline-code spans, so every frozen literal is preserved: metric names (skyhook_status, skyhook_name, skyhook_node_*_count), the skyhook namespace (incl. the CLI default "skyhook"), the runtime-required taint, host paths (/var/lib/skyhook, /var/log/skyhook), the ghcr.io/nvidia/skyhook/agent image, skyhook.nvidia.com annotations/group, the createSkyhook*Role values keys, and dashboard/image/policy filenames. Files: docs/metrics/README.md, docs/cli.md, docs/README.md, docs/kyverno/README.md, docs/versioning.md, docs/release-process.md, docs/designs/webhook-bootstrap-lease.md, chart/README.md. Also drops the now-nonsensical "SCR" acronym to plain "CR" in these files. Folds in review fixes on the docs merged via #369: - runtime_required.md: finish the prose rename (per-NodeWright, NodeWrights, "NodeWright automatically tolerates"). - resource_management.md: clarify the namespace LimitRange is consumed when present (chart installs one by default, can be disabled), not guaranteed. - uninstall.md: document shared cordon ownership — a node is uncordoned only once no cordon_* ownership annotations remain. - cli.md: v0.7.x legacy path edits the Skyhook spec (not NodeWright); chart README grammar fixes.
a6889af to
8490d8f
Compare
What
Continues the Skyhook→NodeWright prose rename into the remaining
docs/files +chart/README.md(8 files, 78/78 word swaps). Follows #369.How it stays safe
Same prose-only swap as #369: it never touches text inside code fences or inline-code spans, so every frozen literal is preserved by construction. Verified post-swap that none of these were altered:
skyhook_status,skyhook_name,skyhook_node_status_count, etc. (only their prose descriptions changed).skyhooknamespace, including the CLI's--namespacedefault"skyhook"(matchescontext.DefaultNamespacein the CLI source).runtime-requiredtaint, host paths/var/lib/skyhook&/var/log/skyhook, theghcr.io/nvidia/skyhook/agentimage, and thecreateSkyhook*Rolevalues keys.skyhook.nvidia.com/*.skyhook-dashboard.json,skyhook-overview.png,skyhook-viewer-binding.yaml,ordering_of_skyhooks.mdlinks.SCR → CR
SCR("Skyhook Custom Resource") no longer expands sensibly, so these files now use plainCR(noCleanupSCRMetadata-style identifiers exist in this set).Notes
docs/nodewright-migration.md(intentionally bilingual) and all frozen surfaces (operator Go source + comments, agent package, chart templates,operator/configCRD/RBAC, metric names, examples) remain untouched — those move with the actual code/CRD migration.README.md/ROADMAP/CONTRIBUTING/SUPPORTrebrand is deferred pending a product-identity decision.