Fix fleetctl apply ignoring spec.fleet - #44894
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughGroupFromBytes now prefers the nested "fleet" key when parsing resources with kind set to fleet or team, falling back to "team" if "fleet" is absent. The selected nested payload is passed through rewriteNewToOldKeys and appended to specs.Teams. Unit tests were added/adjusted to assert correct parsing for both kind: team and kind: fleet and to tighten alias-conflict error fields. Test YAML fixtures in fleetctl apply tests were updated to use spec.fleet for kind: fleet scenarios. Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/spec/spec.go (1)
237-243:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a nil guard for
teamRawafter the map lookup.If the YAML spec body contains neither a
team:nor afleet:key that matcheskind(e.g.kind: fleetwith aspec.team:body),rawTeam[kind]returns a niljson.RawMessage. That nil is passed torewriteNewToOldKeysand then appended tospecs.Teams, which causes a confusing unmarshal failure downstream rather than an actionable error here.🛡️ Proposed fix
teamRaw := rawTeam[kind] +if teamRaw == nil { + return nil, fmt.Errorf("spec.%s is missing or empty for kind %q", kind, s.Kind) +} var err error🤖 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 `@pkg/spec/spec.go` around lines 237 - 243, After fetching teamRaw := rawTeam[kind] add a nil-check and return a clear error if teamRaw == nil instead of passing it to rewriteNewToOldKeys or appending it to specs.Teams; specifically, in the block that uses rawTeam[kind], validate teamRaw is non-nil (use kind in the error message), then call rewriteNewToOldKeys(teamRaw, fleet.TeamSpec{}) and only append the returned teamRaw to specs.Teams when non-nil — this prevents passing nil json.RawMessage into rewriteNewToOldKeys and avoids appending a nil entry to specs.Teams.
🧹 Nitpick comments (1)
pkg/spec/spec_test.go (1)
464-505: 💤 Low valueConsider adding a mismatch test case to
TestGroupFromBytesTeamKinds.The new test covers
kind: team / spec.teamandkind: fleet / spec.fleet(the happy paths), but not the cross-key mismatch (kind: fleetwith aspec.teambody, or vice versa). That path currently produces a nil entry inspecs.Teamswithout an error. Adding a table entry for the mismatch would both document the expected behaviour and serve as a regression guard if the nil-guard fix above is applied.➕ Suggested additional test case
{ "kind: fleet with fleet: key", []byte(` apiVersion: v1 kind: fleet spec: fleet: name: macOS `), }, + // Mismatch: kind says "fleet" but spec key is "team". + // With a proper nil guard this should return an error. + // Uncomment / adjust expected behaviour once the nil guard is in place. }🤖 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 `@pkg/spec/spec_test.go` around lines 464 - 505, Add a negative table case to TestGroupFromBytesTeamKinds that exercises the cross-key mismatch (e.g. kind: fleet but spec: team) by adding a test input byte block with "kind: fleet" and a "spec: team: name: macOS" body and then in the t.Run assert the current observed behavior: GroupFromBytes returns no error, g.Teams has length 1 and g.Teams[0] is nil (use require.NoError(t, err); require.Len(t, g.Teams, 1); require.Nil(t, g.Teams[0])). This documents the mismatch behavior and will catch regressions in GroupFromBytes or specs.Teams handling.
🤖 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 `@pkg/spec/spec.go`:
- Around line 237-243: After fetching teamRaw := rawTeam[kind] add a nil-check
and return a clear error if teamRaw == nil instead of passing it to
rewriteNewToOldKeys or appending it to specs.Teams; specifically, in the block
that uses rawTeam[kind], validate teamRaw is non-nil (use kind in the error
message), then call rewriteNewToOldKeys(teamRaw, fleet.TeamSpec{}) and only
append the returned teamRaw to specs.Teams when non-nil — this prevents passing
nil json.RawMessage into rewriteNewToOldKeys and avoids appending a nil entry to
specs.Teams.
---
Nitpick comments:
In `@pkg/spec/spec_test.go`:
- Around line 464-505: Add a negative table case to TestGroupFromBytesTeamKinds
that exercises the cross-key mismatch (e.g. kind: fleet but spec: team) by
adding a test input byte block with "kind: fleet" and a "spec: team: name:
macOS" body and then in the t.Run assert the current observed behavior:
GroupFromBytes returns no error, g.Teams has length 1 and g.Teams[0] is nil (use
require.NoError(t, err); require.Len(t, g.Teams, 1); require.Nil(t,
g.Teams[0])). This documents the mismatch behavior and will catch regressions in
GroupFromBytes or specs.Teams handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 82188624-74e2-4b0b-a0bc-634f188ccf80
📒 Files selected for processing (2)
pkg/spec/spec.gopkg/spec/spec_test.go
There was a problem hiding this comment.
Pull request overview
This PR fixes fleetctl apply failing to apply team/fleet specs when YAML uses the newer spec.fleet wrapper (as produced by fleetctl get ... --yaml --remove-deprecated-keys), bringing get and apply back into compatibility.
Changes:
- Update
GroupFromBytesto select the team/fleet spec wrapper key dynamically based onkind(sokind: fleetreadsspec.fleet). - Add a regression test covering both
kind: team+spec.teamandkind: fleet+spec.fleet.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/spec/spec.go | Fixes wrapper key selection for team/fleet specs during YAML parsing in GroupFromBytes. |
| pkg/spec/spec_test.go | Adds regression coverage ensuring both deprecated and current team/fleet wrapper formats parse into g.Teams. |
Comments suppressed due to low confidence (1)
pkg/spec/spec.go:243
GroupFromBytesstill silently appends anullteam/fleet spec if the expected wrapper key is missing (e.g.kind: fleetbutspeclacks afleet:key). SinceteamRaw := rawTeam[kind]can be nil, this can recreate the original failure mode (sending{specs:[null]}) without an error. Consider validating thatrawTeam[kind]is present (and possibly that only one ofteam/fleetwrappers is set) and returning a clear error when it isn’t.
rawTeam := make(map[string]json.RawMessage)
if err := yaml.Unmarshal(s.Spec, &rawTeam); err != nil {
return nil, fmt.Errorf("unmarshaling %s spec: %w", kind, err)
}
teamRaw := rawTeam[kind]
var err error
teamRaw, deprecatedKeysMap, err = rewriteNewToOldKeys(teamRaw, fleet.TeamSpec{})
if err != nil {
return nil, fmt.Errorf("in %s spec: %w", kind, err)
}
specs.Teams = append(specs.Teams, teamRaw)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #44894 +/- ##
==========================================
+ Coverage 66.68% 66.74% +0.06%
==========================================
Files 2664 2686 +22
Lines 214697 216884 +2187
Branches 9841 9841
==========================================
+ Hits 143160 144761 +1601
- Misses 58509 58871 +362
- Partials 13028 13252 +224
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
spalmesano0
changed the title
fleetctl apply ignoring spec.fleetfleetctl apply ignoring spec.fleet
There was a problem hiding this comment.
These are supposed to mainly test the preferred config; we have apply_deprecated_test.go for testing the deprecated config.
|
@spalmesano0 Converting to draft while @sgress454 is out. Feel free to update, we'll take out of draft next week. |
|
Thanks for the update! No additional changes from my end, I was just trying to get the ball rolling on this one. |
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
LGTM. Tested that the fix correctly uses |
Related issue: Resolves #44892
Claude also added tests, since this wasn't covered before, but I've kept them in a separate commit in case they're not needed.
Checklist for submitter
Testing
Summary by CodeRabbit
Bug Fixes
Tests