fix(operator): rename skyhook -> nodewright in fmt.Errorf text

[mohityadav8]

Part 2/3 of #403 -- error text surfaced to callers via wrapped errors. Event/condition text landed in PR1, log messages/keys are PR3.

Refs #403

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: c9635a6b-3109-4e17-aa27-53af5c9e6b3f

📥 Commits

Reviewing files that changed from the base of the PR and between 8a8c2d0 and 0f3e8fc.

📒 Files selected for processing (4)

• operator/internal/controller/cluster_state_v2.go
• operator/internal/controller/event_handler.go
• operator/internal/controller/skyhook_controller.go
• operator/internal/dal/dal.go

---

📝 Walkthrough

Walkthrough

Controller and DAL error messages now use NodeWright terminology instead of Skyhook terminology. Legacy workload reconciliation now queries legacy node-metadata ConfigMaps, relabels matching objects, and removes package ConfigMap discovery and reparenting.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Mergeability Score: ⚪ Minimal · up to 0f3e8

This change renames surfaced error text without introducing an actionable merge-blocking risk in the supplied evidence; it is merge-ready after normal checks and review.

Suggested reviewers: lockwobr

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: renaming Skyhook terminology to NodeWright in error text.
Description check	✅ Passed	The description accurately identifies the error-text scope and links it to issue #403 and the related pull requests.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[mohityadav8]

@lockwobr their was a conflict in oine file so i resolved that can u run workflow again

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

operator/internal/controller/skyhook_controller.go (2)

768-780: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Report ConfigMap mutations through workloadsChanged.

Line 777 updates a ConfigMap, but changed remains false. If legacy ConfigMaps are the only objects changed, HandleMigrations leaves updates false despite writing cluster state.

Set changed = true after a successful r.Update.

Proposed fix

 		if mutated {
 			if err := r.Update(ctx, cm); err != nil {
 				return false, false, fmt.Errorf("updating legacy configmap labels [%s]: %w", cm.Name, err)
 			}
+			changed = true
 		}

🤖 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 `@operator/internal/controller/skyhook_controller.go` around lines 768 - 780,
Set changed = true after the successful r.Update call in the ConfigMap mutation
loop, before continuing iteration, so HandleMigrations reports updates when
relabelLegacyMetadataPrefix or addNodeWrightMetaLabel modifies ConfigMaps.

---

692-700: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use the declared label-key constants.

Use legacyNameLabel and legacyNodeMetaLabel in the pod and ConfigMap selectors. This removes duplicated label-key construction and makes the selectors match the documented legacy keys.

As per coding guidelines, extract repeated literals such as label values into named constants near their owner.

Proposed refactor

- client.MatchingLabels{fmt.Sprintf("%s/name", legacyMetadataPrefix): skyhookName}); err != nil {
+ client.MatchingLabels{legacyNameLabel: skyhookName}); err != nil {
...
- client.MatchingLabels{fmt.Sprintf("%s/skyhook-node-meta", legacyMetadataPrefix): skyhookName}); err != nil {
+ client.MatchingLabels{legacyNodeMetaLabel: skyhookName}); err != nil {

Also applies to: 741-743, 760-763

🤖 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 `@operator/internal/controller/skyhook_controller.go` around lines 692 - 700,
Replace duplicated legacy label-key construction in the pod and ConfigMap
selectors with the declared constants legacyNameLabel and legacyNodeMetaLabel.
Update the selector logic near the affected ranges while preserving the existing
resource-kind behavior, and extract any repeated label values into named
constants near their owner.

Source: Coding guidelines

🤖 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.

Outside diff comments:
In `@operator/internal/controller/skyhook_controller.go`:
- Around line 768-780: Set changed = true after the successful r.Update call in
the ConfigMap mutation loop, before continuing iteration, so HandleMigrations
reports updates when relabelLegacyMetadataPrefix or addNodeWrightMetaLabel
modifies ConfigMaps.
- Around line 692-700: Replace duplicated legacy label-key construction in the
pod and ConfigMap selectors with the declared constants legacyNameLabel and
legacyNodeMetaLabel. Update the selector logic near the affected ranges while
preserving the existing resource-kind behavior, and extract any repeated label
values into named constants near their owner.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: f4a34bf1-4ff6-4415-b99d-a1dfa4c7f204

📥 Commits

Reviewing files that changed from the base of the PR and between f88c407 and f49c695.

📒 Files selected for processing (1)

• operator/internal/controller/skyhook_controller.go

[ayuskauskas]

The -70 in operator/internal/controller/skyhook_controller.go isn't part of the rename — it reverts code that's already on main.

Where it came from

f49c695a ("Merge branch 'main' into fix/403-errorf-text-rename") merged main at 84df6448 = #380 (repair three Skyhook to NodeWright migration defects), which rewrote reconcileLegacyLabeledWorkloads. That conflicted with the rename touching the same function, and the resolution kept the branch's pre-#380 side. git diff 84df6448 f49c695a shows the same +35 -70 on this file, so the merge itself dropped the code — no later commit did.

What's lost

1. The two-key legacy ConfigMap sweep. main lists legacy ConfigMaps by both <legacy>/skyhook-node-meta and <legacy>/name (two Lists, since MatchingLabels ANDs its terms, plus a dedupe set). This branch lists only skyhook-node-meta, so package ConfigMaps — which carry <legacy>/name — never converge. UpsertConfigmaps lists by the new <nodewright>/name label, doesn't find them, falls through to Create, and fails AlreadyExists on every reconcile. That is the wedged-upgrade defect #380 fixed; the comment explaining it is deleted along with the code.
2. The reparentToNodeWright call on those ConfigMaps. Legacy skyhook.nvidia.com ownerReferences stay in place, so the migration guide's "delete the old Skyhook CR" step cascade-deletes the package and per-node metadata ConfigMaps out from under the live NodeWright.
3. changed = true after a successful ConfigMap Update. The ConfigMap is updated but changed is never set, so workloadsChanged comes back false and HandleMigrations doesn't record the update.

Side effects: the pod List reverts to inline fmt.Sprintf("%s/name", legacyMetadataPrefix) instead of the legacyNameLabel constant, and at head legacyNodeMetaLabel, legacyNameLabel, and the entire reparentToNodeWright helper are dead — declared, never referenced. It still compiles (unused consts are legal in Go), which is why CI is green.

Suggested fix

Take main's version of the file and re-apply only the string renames on top:

git checkout origin/main -- operator/internal/controller/skyhook_controller.go
# re-apply the ~10 fmt.Errorf skyhook->nodewright edits, then:
git diff origin/main -- operator/internal/controller/skyhook_controller.go   # should be text-only

Worth re-checking the other three files the same way — they're small and look clean, but they went through the same merges.

[ayuskauskas]

This block is a revert of #380, not a rename. On main this sweeps legacy ConfigMaps by both <legacy>/skyhook-node-meta and <legacy>/name (two Lists + dedupe, because MatchingLabels ANDs), and calls reparentToNodeWright on each. Here only skyhook-node-meta is listed, so package ConfigMaps labeled <legacy>/name never converge and UpsertConfigmaps wedges on AlreadyExists; legacy ownerReferences also survive, making these ConfigMaps cascade-delete bait when the old Skyhook CR is removed.

The changed = true after a successful r.Update(ctx, cm) is also gone, so workloadsChanged returns false even when ConfigMaps were relabeled.

It came in via the f49c695a merge of main@84df6448 — restoring main's version of this function and re-applying just the fmt.Errorf renames should resolve it.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[lockwobr]

@mohityadav8 Can you please fix the merge issue that @ayuskauskas found above, want to merge this.

[mohityadav8]

@mohityadav8 Can you please fix the merge issue that @ayuskauskas found above, want to merge this.

yes today i will fix this and and complete the remaining third part of the issue