Self service categories - GitOps support - #46671
Conversation
- Authz: GitOps role can now read+write software_category (global and team-scoped). Drops the TODO from the previous sub-task. - Spec parsing: software.self_service_categories parses into the existing Software struct; GitOps tracks SelfServiceCategoriesPresent so the omitted-vs-empty distinction is preserved. Names are trimmed, length- checked (≤255 runes), case-insensitive deduped, and legacy plain defaults are translated to their canonical emoji form. Every category referenced by a package/app/FMA is cross-validated against the declared set when present. - Apply: new reconcileSelfServiceCategories runs in afterTeamApply (before software upload) and drives the fleet's categories to the YAML list via the four CRUD endpoints. Skipped when the key is omitted or when the software exception is active and `software:` is absent. - Generate: fleetctl generate-gitops emits self_service_categories under each team's software: block when the fleet has any categories.
- pkg/spec/gitops_test.go: parser unit tests for presence detection
(omitted vs explicit-empty vs populated), per-name validation (empty,
whitespace, length, dupes — including dupes via legacy translation),
and cross-validation that package categories must reference a name in
self_service_categories when present.
- cmd/fleetctl/fleetctl/gitops_test.go: TestGitOpsSelfServiceCategoriesReconcile
exercises four scenarios end-to-end through `fleetctl gitops`:
1. Explicit list — creates new, deletes server-only.
2. Empty list — deletes everything.
3. Absent key — no datastore mutation.
4. Software exception + software absent — no datastore mutation.
- cmd/fleetctl/fleetctl/generate_gitops_test.go: a MockClientWithCategories
variant proves generateSoftware emits `self_service_categories:` when
the fleet has categories and omits the key when it doesn't.
- Self-service categories in pkg/spec now use optjson.Slice[string] on both Software (YAML parse target) and GitOpsSoftware (result), so the "key omitted vs explicit empty" distinction is carried by the field itself. Dropped the separate SelfServiceCategoriesPresent bool on GitOps and the map-reparse trick in parseSoftware. - Cross-validation now gates on result.Software.SelfServiceCategories.Set and uses slices.ContainsFunc + strings.EqualFold instead of an intermediate set map. - reconcileSelfServiceCategories drops the softwareExcepted parameter (redundant — exceptions.Software && SoftwarePresent is rejected upstream; the !Set guard handles the excepted+absent case) and the defensive nil-TeamID check (caller is the afterTeamApply hook which sets TeamID immediately before). Inner diff/apply now uses two closures over slices.ContainsFunc instead of two pre-built maps.
Drop the parser-side helper, cross-validation, and dedup logic in pkg/spec — these were all caught server-side anyway. The parser keeps the translation step (because the server's GetSoftwareCategoryIDs translates lookups but NewSoftwareCategory doesn't, so storing canonical form here avoids the asymmetry) plus a pure name-validity check via the new SoftwareCategory.Validate method, so empty / over-length names surface at parse time again (covers dry-run). - server/fleet/software.go: SoftwareCategory.Validate checks the name exactly as set (no mutation, no trimming) and returns NewInvalidArgumentError. Callers normalize first. - ee/server/service/categories.go: NewSoftwareCategory and UpdateSoftwareCategory now TrimSpace then call Validate, replacing inline trim+empty+length blocks. - pkg/spec/gitops.go: parseSoftware translates legacy names, trims each in place, and calls Validate on each; on the way out the slice goes through optjson.SetSlice into Software.SelfServiceCategories. - server/service/client_software.go: drop dead UpdateSelfServiceCategory. - pkg/spec/gitops_test.go: drop TestGitOpsSelfServiceCategoriesValidation and TestGitOpsPackageCategoryReferencesMustExist (asserted parser behavior that's now server-side). Presence test still covers the optjson + translation round-trip.
…on lookup
Legacy plain category names ("Productivity") are now only translated when
resolving a reference against the team's rows — not when storing a gitops
declaration or seeding new categories. This lets an admin manage a plain
"Productivity" row alongside the emoji-prefixed defaults.
- SoftwareCategoryReferenceMatches helper carries the one-way rule (a→b
if equal, or if translate(a) equals b)
- pkg/spec/gitops parser stores self_service_categories verbatim; dedup is
literal-only so plain and emoji forms are distinct
- GetSoftwareCategoryNameToIDMap fetches the team's rows ordered by name
ASC under utf8mb4_unicode_ci so a literal match wins over a translation
match when both rows coexist
- removeDuplicateOrMissingCategories helper in ee/server/service/categories
filters references through GetSoftwareCategoryNameToIDMap, silently
dropping names that don't resolve (instead of erroring at the batch /
installer / vpp call sites)
- regression test covers empty input, literal/emoji forms, translation
fallback, literal-wins-over-translation, team scoping, and missing
names
…spec - Drop setupGitOpsCategoriesMocks/gitopsCategoriesTestState/writeGitOpsCategoriesYAML helpers; each subtest in TestGitOpsSelfServiceCategoriesReconcile now wires its own minimal mocks via a closure inside the test function, so the setup is visible at the call site. - Move parser-only cases (duplicate name in payload, package referencing undeclared category) from cmd/fleetctl down to TestGitOpsSelfServiceCategoriesPresence in pkg/spec. The parser layer is the right place for them: no full server setup, no real software installer URL, faster and more focused.
- Parser dedup of self_service_categories switches from a ToLower-keyed map to a slice + slices.ContainsFunc/EqualFold, matching the case-insensitive style used elsewhere. Added a few blank lines between validate/dedup/accept steps for readability. - Restore (and update) the GetSoftwareCategoryNameToIDMap doc comment so the contract is visible at the function header. - Collapse the 46370 changefile to a single user-facing line.
…ore test - Wrap fleet.SoftwareCategory.Validate errors in New/UpdateSoftwareCategory and the six removeDuplicateOrMissingCategories call sites with ctxerr.Wrap. - Add parser-level cross-validation tests for app_store_apps and fleet_maintained_apps in pkg/spec to mirror the existing packages coverage. - Replace MockClientWithCategories with a Categories field on MockClient, matching the IsFree/WithoutMDM pattern already in the file. - Trim GetSoftwareCategoryNameToIDMap's restored doc comment to two lines. - Consolidate testGetSoftwareCategoryNameToIDMap from 8 t.Run subtests into one table of lookups + two sequential state-changing assertions.
Two CI failures from the previous push: 1. integration_mdm_test.go TestSoftwareCategories: an empty Categories batch payload should clear existing categories. The removeDuplicate- OrMissingCategories helper was returning nil for empty input; downstream datastore writes gate on `if CategoryIDs != nil` so nil was preserving instead of clearing. Return non-nil empty slices for the short-circuit path so the "clear" semantics survive. 2. TestGitOpsBasicGlobalAndTeam asserted GetSoftwareCategoryIDsFuncInvoked after dry-run. After the refactor (a) the helper calls GetSoftwareCategoryNameToIDMap instead, and (b) it short-circuits for empty input so the mock isn't invoked when the test YAML has no categories. The assertion was a leftover from the old code's unconditional call; the VPP-token assertion in the same block still covers "VPP code path was exercised". Drop the obsolete check. Also updates 13 stale test mock setups across the repo that still wired GetSoftwareCategoryIDsFunc; they now wire GetSoftwareCategoryNameToIDMapFunc so callers that pass non-empty categories don't nil-panic.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat/39018-self-service-categories #46671 +/- ##
=====================================================================
Coverage ? 66.83%
=====================================================================
Files ? 2807
Lines ? 223959
Branches ? 11346
=====================================================================
Hits ? 149673
Misses ? 60689
Partials ? 13597
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:
|
![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.
|
@claude review once |
| } | ||
| payloads := config.Software.SelfServiceCategories.Value | ||
|
|
||
| var toInsert []string | ||
| for _, name := range payloads { | ||
| if !slices.ContainsFunc(existing, func(c fleet.SoftwareCategory) bool { return strings.EqualFold(c.Name, name) }) { | ||
| toInsert = append(toInsert, name) | ||
| } | ||
| } | ||
| var toDelete []fleet.SoftwareCategory | ||
| for _, cat := range existing { | ||
| if !slices.ContainsFunc(payloads, func(p string) bool { return strings.EqualFold(p, cat.Name) }) { | ||
| toDelete = append(toDelete, cat) |
There was a problem hiding this comment.
🔴 doSelfServiceCategories reconciles toInsert/toDelete with strings.EqualFold, while the rest of this PR (parse-time validateCategoryReferences in pkg/spec/gitops.go and GetSoftwareCategoryNameToIDMap in server/datastore/mysql/software.go) routes name comparisons through fleet.SoftwareCategoryReferenceMatches, which handles the legacy plain↔emoji mapping ("Productivity" ↔ "💻 Productivity"). Applying self_service_categories: ["Productivity"] against a team seeded with the new default "💻 Productivity" deletes the emoji row (cascading via ON DELETE CASCADE through software_installer_software_categories, vpp_app_team_categories, and in_house_app_software_categories — wiping every UI-managed installer→category association) and creates a brand-new "Productivity" row. The fix is to swap strings.EqualFold(...) for fleet.SoftwareCategoryReferenceMatches(...) in both slices.ContainsFunc closures, matching the rest of the PR.
Extended reasoning...
The asymmetry
This PR routes every other category-name comparison through fleet.SoftwareCategoryReferenceMatches (server/fleet/software.go), which returns true for legacy plain↔emoji equivalents per LegacySoftwareCategoryNames (e.g. "Productivity" ↔ "💻 Productivity"):
pkg/spec/gitops.govalidateCategoryReferences(parse-time package/app/FMA validation) usesSoftwareCategoryReferenceMatches.server/datastore/mysql/software.goGetSoftwareCategoryNameToIDMap(rewritten in this PR) usesSoftwareCategoryReferenceMatches.
The new reconciliation step in server/service/client.go doSelfServiceCategories at lines 2923 and 2929 uses bare strings.EqualFold instead, so the same name pair that resolves as equivalent everywhere else resolves as distinct here.
Step-by-step proof
Assume a team that was created on a Fleet version after the renaming migration. Per fleet.DefaultSelfServiceCategoryNames (server/fleet/software.go:879-886), it is seeded with the emoji-prefixed defaults: ["🌎 Browsers", "👬 Communication", "🧰 Developer tools", "💻 Productivity", "🔐 Security", "🛟 Support"]. The DB row for "💻 Productivity" has some id, call it 101.
An admin applies a GitOps YAML that uses the legacy plain form (this PR's description explicitly endorses this scenario):
software:
self_service_categories:
- "Productivity"parseSoftwareaccepts the YAML.validateCategoryReferencesfor any package/app/FMA category named"Productivity"or"💻 Productivity"passes via the legacy mapping. Dry run reports nothing wrong.doSelfServiceCategoriesreadsexisting = [{ID:101, Name:"💻 Productivity"}, ...]from the API andpayloads = ["Productivity"].- Compute
toInsert: forname="Productivity",slices.ContainsFunc(existing, EqualFold("💻 Productivity", "Productivity"))→false→ enqueue"Productivity". - Compute
toDelete: forcat={ID:101, Name:"💻 Productivity"},slices.ContainsFunc(payloads, EqualFold("Productivity", "💻 Productivity"))→false→ enqueue the row. DeleteSelfServiceCategory(101)fires. The migrations20250501162727_AddSoftwareCategories.goand20251110172137_InHouseAppCategories.godeclareON DELETE CASCADEonsoftware_installer_software_categories.software_category_id,vpp_app_team_categories.software_category_id, andin_house_app_software_categories.software_category_id. Every row tying an installer/VPP/in-house app to category 101 is silently destroyed.AddSelfServiceCategory(teamID, "Productivity")creates a fresh row with a new id. ThendoGitOpsruns the rest of the apply: any YAML-declared installer that references"Productivity"resolves to the new row via the legacy-permissiveGetSoftwareCategoryNameToIDMap, so YAML-declared associations are restored. But UI-managed installer associations, VPP/in-house app associations not declared in this YAML run, and anything excepted from this GitOps apply, lose their category permanently — there is no "if the row existed before, preserve it" path.
The second apply of the same YAML is idempotent because the new "Productivity" row now exists in existing. The first apply's data loss is permanent.
Why the PR description treats this as in-scope
"It takes into account what exists in the database now so if an admin wants to add for example 'Productivity' without the emoji as a category it won't get overwritten."
The current doSelfServiceCategories violates that contract — it does overwrite, by deleting the emoji default in favor of the plain legacy name. The reverse case (YAML uses the emoji name against an older Fleet that still has the plain legacy row) has the same destructive symptom.
Fix
Replace strings.EqualFold with fleet.SoftwareCategoryReferenceMatches in both closures. SoftwareCategoryReferenceMatches is asymmetric (it consults LegacySoftwareCategoryNames[reference]), so use the parameter order documented by the rest of the PR: at the toInsert site SoftwareCategoryReferenceMatches(name /* payload */, c.Name /* existing */), and at the toDelete site SoftwareCategoryReferenceMatches(p /* payload */, cat.Name /* existing */). This mirrors GetSoftwareCategoryNameToIDMap (server/datastore/mysql/software.go:6885), which calls it as (n /* requested */, r.Name /* DB row */).
The existing "plain and emoji names treated as distinct" test in cmd/fleetctl/fleetctl/gitops_test.go only exercises the empty-existing case, so it does not catch this regression. A new subtest with existing = [{Name:"💻 Productivity"}] and payloads = ["Productivity"] should expect added empty and deleted empty.
There was a problem hiding this comment.
Will not be addressing this. We want the self_service_categories to be declarative.
If an admin sets just "Productivity" without the emoji, that's the category that should be set. We could have some logic to update it rather then delete and insert, but I think that would be overcomplicating things.
| if dryRun { | ||
| return nil | ||
| } | ||
| for _, cat := range toDelete { | ||
| if err := c.DeleteSelfServiceCategory(cat.ID); err != nil { | ||
| return fmt.Errorf("deleting self-service category %q: %w", cat.Name, err) | ||
| } | ||
| } | ||
| for _, name := range toInsert { | ||
| if _, err := c.AddSelfServiceCategory(teamID, name); err != nil { | ||
| return fmt.Errorf("adding self-service category %q: %w", name, err) | ||
| } |
There was a problem hiding this comment.
🔴 doSelfServiceCategories at server/service/client.go:2937-2945 issues all DELETE calls first and only then issues the POST calls, with no transaction wrapping the loop. Each DELETE cascades to software_installer_software_categories, vpp_app_team_software_categories, and in_house_app_software_categories (all ON DELETE CASCADE), so any mid-loop failure on AddSelfServiceCategory — including the utf8mb4_unicode_ci collation collision that the PR itself warns about at pkg/spec/gitops.go:1925, or any transient server 5xx — silently destroys every UI-managed installer / VPP / in-house association the deleted categories had. Easiest fix: reverse the loop order so inserts run first and a failed insert leaves stale rows rather than missing rows; a proper fix would mirror doGitOpsLabels at client.go:2991, which delegates the entire swap to a single atomic ApplyLabels server call.
Extended reasoning...
What the bug is
doSelfServiceCategories reconciles the YAML-declared self-service category list against the server state by first looping over toDelete to issue DELETE /api/latest/fleet/software/self_service_categories/{id}, and then looping over toInsert to issue POST. Each iteration is an independent REST call. There is no transaction wrapping the two loops and no rollback on partial failure.
// server/service/client.go:2937-2945
for _, cat := range toDelete {
if err := c.DeleteSelfServiceCategory(cat.ID); err != nil {
return fmt.Errorf("deleting self-service category %q: %w", cat.Name, err)
}
}
for _, name := range toInsert {
if _, err := c.AddSelfServiceCategory(teamID, name); err != nil {
return fmt.Errorf("adding self-service category %q: %w", name, err)
}
}Why the blast radius is destructive
On the server side, software_categories.id is referenced with ON DELETE CASCADE from three association tables: software_installer_software_categories, vpp_app_team_software_categories, and in_house_app_software_categories. When the DELETE loop succeeds for category X, every association row pinning category X to UI-managed installers / VPP apps / in-house apps is cascaded away. None of those associations exist in the YAML — they are state created exclusively through the Fleet UI — so re-running gitops cannot rebuild them. Operators must manually re-attach each affected installer / VPP / in-house app from the UI.
A non-hypothetical trigger lives in this same PR
The dry-run dedup check in pkg/spec/gitops.go carries this explicit caveat:
// Doesn't catch utf8mb4_unicode_ci collation collisions (e.g. "🔐 Security" vs "🛡 Security") in dry runs.
So a YAML with two emoji-prefixed names that fold to the same trailing word under utf8mb4_unicode_ci passes parser dedup, passes --dry-run, and then hits a unique-key violation on the second or third AddSelfServiceCategory POST — after the entire DELETE loop has committed. Any transient 5xx, network blip, or auth token expiry mid-insert produces the same outcome.
Step-by-step proof
Suppose an operator has a fleet with three UI-managed categories — Old A, Old B, Old C — each linked from several installers / VPP apps. They push this YAML:
software:
self_service_categories:
- "🔐 Security"
- "🛡 Security" # folds to the same value under utf8mb4_unicode_ciparseSoftwarerunsslices.ContainsFunc(... strings.EqualFold ...)—🔐 Securityand🛡 Securityare not equal-fold, so dry-run passes.doSelfServiceCategorieslists existing →[Old A, Old B, Old C]. It computestoDelete = [Old A, Old B, Old C]andtoInsert = ["🔐 Security", "🛡 Security"].- The DELETE loop runs. Three commits hit the database. Three CASCADE deletes wipe every installer / VPP / in-house app association to
Old A,Old B,Old C. - The POST for
🔐 Securitysucceeds. - The POST for
🛡 Securityerrors out with a unique-key violation, because MySQL'sutf8mb4_unicode_cicollation treats the trailing word the same. doSelfServiceCategoriesreturns the error. GitOps apply fails.- End state: the three old categories are gone, every UI-managed association to them is gone, and only
🔐 Security(with zero associations) exists. Operator fixes the YAML and re-applies, but the cascaded rows in the three join tables are not in YAML and cannot be reconstructed.
Why the fix is trivial
The labels reconciliation in the same file at client.go:2991 delegates to c.ApplyLabels(...), a single transactional server endpoint. The category code diverges from that precedent without rationale. The minimal fix is a one-line reorder — insert first, then delete — so that any insert failure returns with the old categories (and their associations) still intact. The proper fix is a BatchApplySelfServiceCategories endpoint mirroring ApplyLabels so the whole swap is atomic server-side.
Addressing the "nit" framing
One verifier argued this should be nit because the subsequent batch upload of installers / VPP apps in ApplyGroup would rebuild the (installer ↔ category) and (vpp_app ↔ category) rows from the YAML-declared categories. That argument doesn't hold: only categories declared in YAML (under software.packages[].categories, software.app_store_apps[].categories, etc.) get rebuilt. UI-managed associations — installers and apps that an admin attached to categories from the web UI without YAML representation — have no source of truth outside the database. The cascaded rows for those are gone for good, and the operator's only recourse is manual re-attachment.
There was a problem hiding this comment.
Addressed.
claude:
Finding 2**: Reversed the loop to INSERT-first, so a failed AddSelfServiceCategory leaves stale rows rather than running a prior DELETE whose cascade would wipe link-table associations.
| } | ||
| if err := c.doSelfServiceCategories(incoming, dryRun); err != nil { | ||
| return err |
There was a problem hiding this comment.
🔴 doSelfServiceCategories is never invoked when applying a no-team / unassigned.yml config because the only call site is inside the if incoming.TeamName != nil && !incoming.IsNoTeam() block at server/service/client.go:2485. Categories declared in software.self_service_categories for no-team are accepted and validated by the parser, but the DB reconciliation never runs (no INSERTs, no DELETEs), and at install time removeDuplicateOrMissingCategories silently drops package category references that don't resolve. generate-gitops also emits this key for the unassigned fleet (cmd/fleetctl/fleetctl/generate_gitops.go:2270), so unassigned.yml does not round-trip. Either reject self_service_categories on no-team in parseSoftware, or invoke doSelfServiceCategories for the no-team flow with teamID=0.
Extended reasoning...
What the bug is\n\nIn server/service/client.go the closure that calls doSelfServiceCategories is installed only when the incoming spec is a real team:\n\ngo\n// client.go:2484-2501\nvar afterTeamApply func(teamIDsByName map[string]uint) error\nif incoming.TeamName != nil && !incoming.IsNoTeam() {\n afterTeamApply = func(teamIDsByName map[string]uint) error {\n ...\n if err := c.doSelfServiceCategories(incoming, dryRun); err != nil {\n return err\n }\n ...\n }\n}\n\n\nFor no-team.yml / unassigned.yml, IsNoTeam() returns true (unassignedTeamName is coerced to No team in parseName), so afterTeamApply is nil and ApplyGroup never calls it. The no-team branch falls through to doGitOpsNoTeamSetupAndSoftware at line 2561, which I read in full — it handles installers, VPP apps, FMAs, setup script, and webhook settings, but never invokes doSelfServiceCategories. Grep confirms doSelfServiceCategories has exactly one call site in the codebase.\n\nWhy existing code doesn't prevent it\n\nparseSoftware in pkg/spec/gitops.go rejects the software: key only when result.global() is true (i.e. the org-settings file). For no-team.yml the team name is set (to "No team"), so global() returns false and the software block — including self_service_categories — is parsed normally. validateCategoryReferences then enforces that package/app categories must be declared in this same in-YAML list, which actively pushes users toward declaring no-team categories. Categories for team_id=0 are also legal in the DB: policy.rego allows global admin/maintainer/gitops to write software_category objects without a team_id != 0 constraint, and NewSoftwareCategory in ee/server/service/categories.go explicitly skips the TeamExists check when *teamID == 0. So every layer except the reconciliation step thinks no-team categories are a supported configuration.\n\nImpact\n\nDeclared no-team categories never reach the DB (no INSERT for new names) and removed names are never deleted (no DELETE for stale rows). At install time, removeDuplicateOrMissingCategories in ee/server/service/categories.go calls GetSoftwareCategoryNameToIDMap(ctx, 0, names), finds no row, and silently drops the names from the payload — the integration test at integration_mdm_test.go:20821 already documents this "silently dropped" behavior for non-existent categories. So a no-team package that references a category passes parse-time validation but installs with an empty Categories list. The round-trip claim in the PR description ("takes into account what exists in the database now") also breaks for the unassigned fleet: generate_gitops.go:2270 calls ListSelfServiceCategories(teamID) for every team including the unassigned virtual team (ID=0, see the teams iteration starting around line 462), and unconditionally emits the names into unassigned.yml. The mock in generate_gitops_test.go:349-354 only returns rows for teamID==1, which masks the bug in tests but not in real deployments.\n\nStep-by-step proof\n\n1. Author writes unassigned.yml containing:\nyaml\nname: Unassigned\nsoftware:\n self_service_categories:\n - "MyCustom"\n packages:\n - url: https://example.com/installer.pkg\n hash_sha256: "..."\n categories: ["MyCustom"]\n\n2. GitOpsFromFile parses successfully: parseSoftware accepts self_service_categories because result.global() returns false (team name "No team"), and validateCategoryReferences passes because "MyCustom" is in the declared list.\n3. DoGitOps is called. IsNoTeam() is true, so the guard at line 2485 evaluates to false and afterTeamApply stays nil.\n4. ApplyGroup runs and the no-team branch at line 2561 calls doGitOpsNoTeamSetupAndSoftware — which handles installers, VPP apps, FMAs, setup script. No category reconciliation. "MyCustom" is never INSERTed into software_categories.\n5. The installer batch reaches softwareBatchUpload, which calls removeDuplicateOrMissingCategories(ctx, 0, ["MyCustom"]). GetSoftwareCategoryNameToIDMap returns an empty map. The function returns ([], []) and the package is installed with Categories=[].\n6. End-user sees zero self-service categories on hosts in the unassigned fleet, despite the YAML succeeding without error.\n\nThe symmetric failure also matters: self_service_categories: [] on unassigned.yml should delete all team_id=0 category rows but is a no-op, so stale rows accumulate.\n\nHow to fix\n\nTwo options, both small:\n\n- Reject the key in the parser: in parseSoftware (pkg/spec/gitops.go), if result.IsNoTeam() and software.SelfServiceCategories.Set, append a multiError. This mirrors how reports and agent_options are gated for no-team and keeps users from declaring something that won't be honored.\n- Run the reconciliation: extend the guard at client.go:2485 so the no-team path also calls doSelfServiceCategories (with teamID=0). The auth path is already permitted (policy.rego allows gitops to write software_category and NewSoftwareCategory skips TeamExists for team 0), and doSelfServiceCategories already dereferences *config.TeamID, so the no-team path just needs to set incoming.TeamID = ptr.Uint(0) before invoking it. The latter is consistent with generate-gitops emitting the key for the unassigned fleet and preserves round-trip.
There was a problem hiding this comment.
Addressed.
claude:
Finding 3: doSelfServiceCategories is now invoked from doGitOpsNoTeamSetupAndSoftware.
- Reconcile is now INSERT-first so a failed AddSelfServiceCategory leaves stale rows rather than letting the prior DELETE wipe link-table rows via ON DELETE CASCADE (review Finding 2). - doSelfServiceCategories is now invoked from the no-team apply path so Unassigned configs reconcile under team_id = 0 (review Finding 3). The parser coerces "Unassigned" to "No team" at parse time, so the team-flow afterTeamApply hook never fires for these configs. - TestGitOpsSelfServiceCategoriesReconcile consolidated from 7 t.Run subtests into one sequential function: shared ds + mocks at the top, per-case reset(), labeled blocks for each case. Added a no-team case asserting reconcile happens against team_id = 0, plus an INSERT-first ordering assertion that catches Finding 2 regression. - TestGitOpsSelfServiceCategoriesPresence gained three parser-level cases: validation (empty/whitespace/over-length), trim + case-insensitive dedup, and a positive cross-validation case for plain->emoji translation.
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
|
Failing test doesn't seem related, not sure what I can do about it. |
a3338d0
into
feat/39018-self-service-categories
2 participants
Related issue: Resolves #46392
A few things in this PR:
Didn't add logs like "[+] applied X self service categories" since it wasn't mentioned in the docs, but wouldn't be too hard to add.
Checklist for submitter
Testing
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually
New Fleet configuration settings
fleetctl generate-gitops