Deprecate "team" and "query" API params #39873
Deprecate "team" and "query" API params #39873sgress454 wants to merge 5 commits intosgress454/39344-rename-tags-onlyfrom
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis pull request implements field name migration across Fleet's API and configuration, renaming terminology from team-based to fleet-based and query-based to report-based. It introduces deprecation-handling infrastructure in the endpoint utilities package for backward compatibility, including JSON key rewriting and aliasing mechanisms for streaming and in-memory JSON processing. The changes update fleetctl code generation and spec parsing to apply these transformations automatically, and update activity logging and webhook payload serialization to include both old and new field names. Comprehensive test updates reflect the new field names throughout the codebase. Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/spec/spec.go (1)
144-152:⚠️ Potential issue | 🟠 Major
TeamKindcase hardcodesrawTeam["team"]— will miss input using the new"fleet"key.For all other spec kinds,
rewriteNewToOldKeysrewrites new key names back to old names before unmarshaling. TheTeamKindcase bypasses this by unmarshaling into a raw map and directly accessingrawTeam["team"]. If a user providesfleet:(the new name) in their YAML, line 152 will produce anilvalue since the map won't contain the"team"key.Consider either:
- Applying
rewriteNewToOldKeystos.Specbefore the raw-map unmarshal (using a representative struct that hasjson:"team" renameto:"fleet"), or- Checking for both
rawTeam["team"]andrawTeam["fleet"]with appropriate precedence.Option 2: check both keys
case fleet.TeamKind: 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) } - specs.Teams = append(specs.Teams, rawTeam["team"]) + teamData, ok := rawTeam["team"] + if !ok { + teamData = rawTeam["fleet"] + } + specs.Teams = append(specs.Teams, teamData)server/service/hosts.go (1)
252-258:⚠️ Potential issue | 🔴 CriticalBug:
DuplicateJSONKeysis called before checking the marshal error.Line 253 processes
datafrommarshalJsonbefore line 254 checks whethermarshalJsonreturned an error. WhileDuplicateJSONKeyshandles empty/nil data gracefully, the intent is clearly to skip processing on error. Move the call after the error check.🐛 Proposed fix
data, err := marshalJson(hostResp) - data = endpointer.DuplicateJSONKeys(data, aliasRules) if err != nil { fmt.Fprint(w, `],`) fmt.Fprint(w, marshalError(fmt.Sprintf("marshaling host response: %s", err.Error()))) fmt.Fprint(w, `}`) return } + data = endpointer.DuplicateJSONKeys(data, aliasRules) if !firstHost {
🤖 Fix all issues with AI agents
In `@cmd/fleetctl/fleetctl/generate_gitops.go`:
- Around line 507-516: The nil-pointer occurs because the error path logs
team.Name when team can be nil; update the error handling in the block that
calls cmd.generateQueries(teamToProcess.ID) to avoid dereferencing team.Name —
use a safe name variable (e.g., derive name := "global" or use
teamToProcess.Name if non-nil) or include teamToProcess.ID in the error message
instead, and write that safe value when reporting the error and when assigning
cmd.FilesToWrite[fileName].(map[string]any)["reports"]; apply the same defensive
change to the analogous policies block (the call to cmd.generatePolicies) so
neither error path dereferences team.Name when team is nil.
In
`@cmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_deprecated_test.go`:
- Line 715: Replace the typo in the inline comment "Check script existance" with
the correct spelling "Check script existence" in the
gitops_enterprise_integration_deprecated_test.go test file (search for the exact
comment string to locate it).
- Around line 1599-1600: Update the truncated single-line doc comment above
TestGitOpsSoftwareDisplayName so it is either a complete descriptive sentence or
remove it; do the same for the three other nearby test-function comments that
were cut off (they currently end mid-sentence). Locate the comment tokens
starting with "//" immediately above the test function declarations (e.g.,
TestGitOpsSoftwareDisplayName) and replace each truncated comment with a full,
concise description of what the test verifies or delete the comment entirely to
avoid incomplete/copy-paste artifacts.
- Around line 296-329: The test currently uses globalFile.WriteString(...) to
append YAML blocks (the calls using globalFile.WriteString with dirPath,
digiCertServer.URL and scepServer.URL) which leaves the file handle in append
mode and concatenates documents; replace these append-style writes with a full
overwrite using os.WriteFile(globalFile.Name(), []byte(yamlContent), 0o600) (or
appropriate perm) so the file is truncated and contains only the intended YAML;
do this for both occurrences where globalFile.WriteString is used in this test
(the two multi-line YAML writes) and ensure you check and require.NoError on the
os.WriteFile call.
- Around line 1892-1898: The SQL query that fetches icon filenames uses a
hardcoded team_id = 0 which is wrong for titles created for a specific team;
update the query invocation inside the anonymous function (the call using
sqlx.In with "SELECT filename FROM software_title_icons WHERE team_id = ? AND
software_title_id IN (?)", 0, teamTitleIDs) to pass team.ID instead of 0 so it
filters by the actual team (use team.ID, keeping teamTitleIDs as before) and
ensure the surrounding call to mysql.ExecAdhocSQL/sqlx.SelectContext remains
unchanged.
In `@pkg/spec/gitops_deprecations.go`:
- Around line 21-33: Deprecated nested-array mappings in
DeprecatedGitOpsKeyMappings fail because migrateKeyPathRecursive expects array
markers as separate parts (oldParts[1] == "[]") but key parts like
"apple_business_manager[]" keep the "[]" suffix; update migrateKeyPathRecursive
to detect and handle a trailing "[]" on a part: when splitting path parts, if a
part endsWith "[]", strip the "[]" to get baseKey, use data[baseKey] to access
the slice (and iterate over its elements recursively), and ensure
newParts/newKey construction preserves or replaces the array marker
consistently; reference the function migrateKeyPathRecursive, variables oldParts
and oldKey, and the DeprecatedGitOpsKeyMappings entries (e.g.
"org_settings.mdm.apple_business_manager[].macos_team") when making this change.
In `@pkg/spec/gitops_test.go`:
- Around line 776-784: Update the stale test comment to accurately describe the
test: change the comment text that currently reads "Invalid policies in a
separate file" to something like "Invalid reports in a separate file" so it
matches the test block that writes "[2]" to a temp file and asserts an error
from gitOpsFromString; locate the comment above the tmpFile creation in the test
that uses getConfig, gitOpsFromString, and assert.ErrorContains and adjust the
wording accordingly.
In `@server/platform/endpointer/endpoint_utils.go`:
- Around line 310-331: The current query-parameter handling in the block around
queryVal/queryTagValue and fp.Sf.Tag.Lookup("renameto") ignores the new name
when both deprecated and new query params are present; update the logic to
detect when both the deprecated name (queryTagValue) and the parsed renameto
name (use ParseTag(renameTo) to get the actual newName) are present in
r.URL.Query(), and if so return the same AliasConflictError used by
JSONKeyRewriteReader (construct and return AliasConflictError with both names);
otherwise preserve existing behavior (warn when only old present, use new when
only new present). Ensure you reference queryVal, queryTagValue,
fp.Sf.Tag.Lookup("renameto"), ParseTag(renameTo), and r.URL.Query().Get(newName)
when implementing the check.
In `@server/service/integration_core_test.go`:
- Around line 14122-14126: The test mutates the global AI feature flag via
s.Do("PATCH", "/api/latest/fleet/config", ...) and leaves it disabled for later
tests; capture the original server_settings.ai_features_disabled value before
patching, and register a t.Cleanup that restores it by issuing a PATCH (using
the same s.Do helper and json.RawMessage payload) to set ai_features_disabled
back to the original boolean; use t.Cleanup to ensure restoration even if the
test fails.
In `@server/service/modules/activities/activities.go`:
- Around line 55-59: DuplicateJSONKeys currently re-encodes JSON with pretty
indentation causing inconsistent formatting; change DuplicateJSONKeys
(json_key_duplicator.go) to produce compact JSON by removing
jsontext.WithIndent(" ") or by adding a parameter/option (e.g., compact bool or
indent string) so callers can request compact output, then update the caller in
activities.go where DuplicateJSONKeys is invoked (after
endpointer.ExtractAliasRules) to call it with the compact option (or rely on the
default compact behavior). Ensure the function still applies alias rules and
returns bytes in compact form to keep stored activity details consistent with
json.Marshal output.
🧹 Nitpick comments (10)
pkg/spec/spec.go (1)
48-61: Silent error swallowing inrewriteNewToOldKeysis acceptable but worth a log.The fallback to original data on error (line 58) is a safe default, but errors during key rewriting could silently mask issues (e.g., malformed JSON that happens to unmarshal differently). Consider adding a debug-level log when the rewrite fails so issues are diagnosable.
cmd/fleetctl/fleetctl/generate_gitops.go (1)
116-168: ThealiasRulesmap contains broad key names like"team"and"queries"that are applied recursively.
replaceAliasKeysrenames every occurrence of these keys at any nesting depth. This works for the current data model but is fragile — any future struct field with ajson:"team"tag that is unrelated to this rename would be silently affected. Also, if a map already contains both the old and new key (e.g.,"team_id"and"fleet_id"), the rename silently overwrites the existing value.Consider either:
- Adding a conflict check in
replaceAliasKeys(log a warning or return an error if the target key already exists), or- Documenting the assumption that these keys are globally unique and won't conflict.
Proposed conflict check
var renames []rename for k := range val { if newKey, ok := rules[k]; ok { + if _, exists := val[newKey]; exists { + // Both old and new key exist — skip rename to avoid data loss. + continue + } renames = append(renames, rename{k, newKey}) } }pkg/spec/gitops_deprecations.go (1)
56-118:migrateKeyPathRecursiveassumesoldParts/newPartsare always the same length — no validation.If a mapping has paths of different depths (e.g.,
"a.b.c"→"x.y"), the recursion could index out of bounds or silently mismatch keys at each level. Consider adding an upfront check or at least a comment that equal-depth paths are required.Add a validation in `migrateKeyPath`
func migrateKeyPath(data map[string]any, oldPath, newPath string, logFn Logf) error { oldParts := strings.Split(oldPath, ".") newParts := strings.Split(newPath, ".") + if len(oldParts) != len(newParts) { + return fmt.Errorf("deprecated key mapping path depth mismatch: %q (%d) vs %q (%d)", oldPath, len(oldParts), newPath, len(newParts)) + } return migrateKeyPathRecursive(data, oldParts, newParts, oldPath, newPath, logFn) }server/platform/endpointer/json_key_rewriter.go (1)
96-100: Stale comment onClose()— references "pipe" and "transform goroutine" that no longer exist.The implementation reads everything eagerly into a
bytes.Buffer(no goroutine or pipe). The doc comment should be updated.Suggested doc fix
-// Close closes the reader end of the pipe to unblock the transform goroutine -// if the consumer stops reading early. +// Close is a no-op; the reader buffers all rewritten JSON eagerly in the constructor. func (r *JSONKeyRewriteReader) Close() error { return nil }cmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.go (1)
2805-2821: Consider renaming the template parameterQueriestoReportsfor consistency.The YAML key has been renamed to
reports:but the Go template struct field is stillQueries(line 2808, used at line 2813). This mismatch could confuse maintainers.Proposed rename
type tmplParams struct { Name string - Queries string + Reports string Labels string } teamCfgTmpl, err := template.New("t1").Parse(` controls: software: -reports:{{ .Queries }} +reports:{{ .Reports }} policies:And update usages at lines 2834 and 2877:
- Queries: fmt.Sprintf("\n - name: query-%d\n query: SELECT 1", i), + Reports: fmt.Sprintf("\n - name: query-%d\n query: SELECT 1", i),server/platform/endpointer/json_key_duplicator.go (2)
36-36: Output is always pretty-printed due toWithIndent(" ").The encoder always indents the output. If callers expect compact JSON (e.g., for DB storage of activity details), this inflates payload size. Consider accepting an option or removing the indent if compact output is the common case.
Suggestion: use compact encoding by default
- enc := jsontext.NewEncoder(&buf, jsontext.WithIndent(" "), jsontext.AllowDuplicateNames(true)) + enc := jsontext.NewEncoder(&buf, jsontext.AllowDuplicateNames(true))
125-137: RecursiveDuplicateJSONKeysfor nested values re-creates decoder/encoder per nesting level.
dec.ReadValue()reads the entire nested value as raw JSON, then the recursive call re-parses it from scratch. For deeply nested or large payloads, this creates O(depth) full re-parses of inner content. This is acceptable if payloads are small (typical for API responses), but worth being aware of for large payloads.server/platform/endpointer/endpoint_utils.go (3)
148-204:extractAliasRulesFromTypeis a thin wrapper that only allocates avisitedmap.Consider inlining the
visitedmap allocation intoExtractAliasRulesdirectly and droppingextractAliasRulesFromTypeto reduce indirection. Not blocking.
695-706: Body-decoder deprecation warning omits the new field names, unlike the non-body path.Lines 610-625 compute and log the replacement names (
newNames), but the body-decoder path here (lines 698-705) only says "use the updated field names instead" without listing them. Consider reusing the same deprecation-logging logic.Suggested fix
if rewriter != nil { if deprecated := rewriter.UsedDeprecatedKeys(); len(deprecated) > 0 { + newNames := make([]string, len(deprecated)) + for i, old := range deprecated { + for _, rule := range aliasRules { + if rule.OldKey == old { + newNames[i] = rule.NewKey + break + } + } + } logging.WithLevel(ctx, level.Warn) logging.WithExtras(ctx, "deprecated_fields", fmt.Sprintf("%v", deprecated), - "deprecation_warning", "use the updated field names instead", + "deprecation_warning", fmt.Sprintf("use the updated field names (%s) instead", newNames), ) } }
299-299: Movecontext.Contextto first parameter per Go convention.
context.Contextshould be the first parameter in Go functions per Go idioms. This refactoring requires updating both the function signature at line 299 and the call site at line 672.Required changes
Function signature:
-func DecodeQueryTagValue(r *http.Request, fp fieldPair, customDecoder DomainQueryFieldDecoder, ctx context.Context) error { +func DecodeQueryTagValue(ctx context.Context, r *http.Request, fp fieldPair, customDecoder DomainQueryFieldDecoder) error {Call site at line 672:
- err = DecodeQueryTagValue(r, fp, customQueryDecoder, ctx) + err = DecodeQueryTagValue(ctx, r, fp, customQueryDecoder)
| if team == nil || team.ID != 0 { | ||
| // Generate queries (except for on No Team). | ||
| queries, err := cmd.generateQueries(teamToProcess.ID) | ||
| // Generate reports (except for on No Team). | ||
| reports, err := cmd.generateQueries(teamToProcess.ID) | ||
| if err != nil { | ||
| fmt.Fprintf(cmd.CLI.App.ErrWriter, "Error generating queries for team %s: %s\n", team.Name, err) | ||
| fmt.Fprintf(cmd.CLI.App.ErrWriter, "Error generating reports for team %s: %s\n", team.Name, err) | ||
| return ErrGeneric | ||
| } | ||
| cmd.FilesToWrite[fileName].(map[string]interface{})["queries"] = queries | ||
| // nolint:nilaway // we want to include "reports: null" in the output if there are no reports. | ||
| cmd.FilesToWrite[fileName].(map[string]any)["reports"] = reports | ||
| } |
There was a problem hiding this comment.
Nil-pointer dereference on the error path when team is nil.
When processing the global config (team == nil), the guard on line 507 passes (team == nil || team.ID != 0). If generateQueries returns an error, line 511 dereferences team.Name, which will panic.
Note: the same pre-existing issue exists on line 502 for policies.
Proposed fix
if team == nil || team.ID != 0 {
// Generate reports (except for on No Team).
reports, err := cmd.generateQueries(teamToProcess.ID)
if err != nil {
- fmt.Fprintf(cmd.CLI.App.ErrWriter, "Error generating reports for team %s: %s\n", team.Name, err)
+ teamName := "Global"
+ if team != nil {
+ teamName = team.Name
+ }
+ fmt.Fprintf(cmd.CLI.App.ErrWriter, "Error generating reports for team %s: %s\n", teamName, err)
return ErrGeneric
}🤖 Prompt for AI Agents
In `@cmd/fleetctl/fleetctl/generate_gitops.go` around lines 507 - 516, The
nil-pointer occurs because the error path logs team.Name when team can be nil;
update the error handling in the block that calls
cmd.generateQueries(teamToProcess.ID) to avoid dereferencing team.Name — use a
safe name variable (e.g., derive name := "global" or use teamToProcess.Name if
non-nil) or include teamToProcess.ID in the error message instead, and write
that safe value when reporting the error and when assigning
cmd.FilesToWrite[fileName].(map[string]any)["reports"]; apply the same defensive
change to the analogous policies block (the call to cmd.generatePolicies) so
neither error path dereferences team.Name when team is nil.
| _, err = globalFile.WriteString(fmt.Sprintf(` | ||
| agent_options: | ||
| controls: | ||
| macos_settings: | ||
| custom_settings: | ||
| - path: %s/testdata/gitops/lib/scep-and-digicert.mobileconfig | ||
| org_settings: | ||
| server_settings: | ||
| server_url: $FLEET_URL | ||
| org_info: | ||
| org_name: Fleet | ||
| secrets: | ||
| certificate_authorities: | ||
| digicert: | ||
| - name: DigiCert | ||
| url: %s | ||
| api_token: digicert_api_token | ||
| profile_id: digicert_profile_id | ||
| certificate_common_name: digicert_cn | ||
| certificate_user_principal_names: [%q] | ||
| certificate_seat_id: digicert_seat_id | ||
| custom_scep_proxy: | ||
| - name: CustomScepProxy | ||
| url: %s | ||
| challenge: challenge | ||
| policies: | ||
| queries: | ||
| `, | ||
| dirPath, | ||
| digiCertServer.URL, | ||
| "digicert_upn_2", // minor modification to stored config so gitops run is not a no-op and triggers call to external digicert service | ||
| scepServer.URL+"/scep", | ||
| )) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
File content is appended, not replaced — the YAML file will contain concatenated documents.
globalFile.WriteString(...) at line 296 (and again at line 353) appends to the already-written file because the file handle was never truncated, seeked, or re-created. The resulting file will contain multiple YAML blocks without --- separators, producing either a parse error or silently merging keys in unexpected ways.
Other tests in this file correctly use os.WriteFile(globalFile.Name(), ...) (e.g., line 480) to replace file contents. Apply the same pattern here.
Suggested fix for the second write (line 296)
- _, err = globalFile.WriteString(fmt.Sprintf(`
+ require.NoError(t, os.WriteFile(globalFile.Name(), []byte(fmt.Sprintf(`
agent_options:
...
- ))
- require.NoError(t, err)
+ )), 0o644))Apply the same change for the third write at line 353.
🤖 Prompt for AI Agents
In
`@cmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_deprecated_test.go`
around lines 296 - 329, The test currently uses globalFile.WriteString(...) to
append YAML blocks (the calls using globalFile.WriteString with dirPath,
digiCertServer.URL and scepServer.URL) which leaves the file handle in append
mode and concatenates documents; replace these append-style writes with a full
overwrite using os.WriteFile(globalFile.Name(), []byte(yamlContent), 0o600) (or
appropriate perm) so the file is truncated and contains only the intended YAML;
do this for both occurrences where globalFile.WriteString is used in this test
(the two multi-line YAML writes) and ensure you check and require.NoError on the
os.WriteFile call.
| []string{"gitops", "--config", fleetctlConfig.Name(), "-f", globalFile.Name(), "-f", noTeamFilePath, "--dry-run"}), true) | ||
| s.assertRealRunOutputWithDeprecation(t, fleetctl.RunAppForTest(t, []string{"gitops", "--config", fleetctlConfig.Name(), "-f", globalFile.Name(), "-f", noTeamFilePath}), true) | ||
|
|
||
| // Check script existance |
There was a problem hiding this comment.
Typo in comment: "existance" → "existence" (line 715).
🤖 Prompt for AI Agents
In
`@cmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_deprecated_test.go`
at line 715, Replace the typo in the inline comment "Check script existance"
with the correct spelling "Check script existence" in the
gitops_enterprise_integration_deprecated_test.go test file (search for the exact
comment string to locate it).
| // TestGitOpsSoftwareDisplayName tests that display names for software packages and VPP apps | ||
|
|
There was a problem hiding this comment.
Truncated doc-comments on several test functions.
Lines 1599, 1717, 2093, and 2250 have // comments that end abruptly mid-sentence (e.g., "tests that display names for software packages and VPP apps"). These appear to be copy-paste artifacts with incomplete trailing text. Either complete or remove them.
Also applies to: 1717-1718, 2093-2094, 2250-2251
🤖 Prompt for AI Agents
In
`@cmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_deprecated_test.go`
around lines 1599 - 1600, Update the truncated single-line doc comment above
TestGitOpsSoftwareDisplayName so it is either a complete descriptive sentence or
remove it; do the same for the three other nearby test-function comments that
were cut off (they currently end mid-sentence). Locate the comment tokens
starting with "//" immediately above the test function declarations (e.g.,
TestGitOpsSoftwareDisplayName) and replace each truncated comment with a full,
concise description of what the test verifies or delete the comment entirely to
avoid incomplete/copy-paste artifacts.
| mysql.ExecAdhocSQL(t, s.DS, func(q sqlx.ExtContext) error { | ||
| stmt, args, err := sqlx.In("SELECT filename FROM software_title_icons WHERE team_id = ? AND software_title_id IN (?)", 0, teamTitleIDs) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return sqlx.SelectContext(ctx, q, &teamIconFilenames, stmt, args...) | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'software_title_icons' --type=go -C 3 -g '!*_test.go'Repository: fleetdm/fleet
Length of output: 11379
🏁 Script executed:
sed -n '1850,1920p' cmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_deprecated_test.goRepository: fleetdm/fleet
Length of output: 3133
Use team.ID instead of 0 when querying icons for team-scoped titles.
At line 1892, the query filters team_id = 0 for teamTitleIDs, but these titles were created for a specific team (retrieved at line 1859 with team.ID). The software_title_icons table stores team-scoped icons with the actual team.ID, not 0. Using team_id = 0 will return no results or incorrect icons. Change the query to use team.ID instead:
Diff
- stmt, args, err := sqlx.In("SELECT filename FROM software_title_icons WHERE team_id = ? AND software_title_id IN (?)", 0, teamTitleIDs)
+ stmt, args, err := sqlx.In("SELECT filename FROM software_title_icons WHERE team_id = ? AND software_title_id IN (?)", team.ID, teamTitleIDs)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mysql.ExecAdhocSQL(t, s.DS, func(q sqlx.ExtContext) error { | |
| stmt, args, err := sqlx.In("SELECT filename FROM software_title_icons WHERE team_id = ? AND software_title_id IN (?)", 0, teamTitleIDs) | |
| if err != nil { | |
| return err | |
| } | |
| return sqlx.SelectContext(ctx, q, &teamIconFilenames, stmt, args...) | |
| }) | |
| mysql.ExecAdhocSQL(t, s.DS, func(q sqlx.ExtContext) error { | |
| stmt, args, err := sqlx.In("SELECT filename FROM software_title_icons WHERE team_id = ? AND software_title_id IN (?)", team.ID, teamTitleIDs) | |
| if err != nil { | |
| return err | |
| } | |
| return sqlx.SelectContext(ctx, q, &teamIconFilenames, stmt, args...) | |
| }) |
🤖 Prompt for AI Agents
In
`@cmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_deprecated_test.go`
around lines 1892 - 1898, The SQL query that fetches icon filenames uses a
hardcoded team_id = 0 which is wrong for titles created for a specific team;
update the query invocation inside the anonymous function (the call using
sqlx.In with "SELECT filename FROM software_title_icons WHERE team_id = ? AND
software_title_id IN (?)", 0, teamTitleIDs) to pass team.ID instead of 0 so it
filters by the actual team (use team.ID, keeping teamTitleIDs as before) and
ensure the surrounding call to mysql.ExecAdhocSQL/sqlx.SelectContext remains
unchanged.
| var DeprecatedGitOpsKeyMappings = []DeprecatedKeyMapping{ | ||
| // Top-level keys | ||
| {"team_settings", "settings"}, | ||
| {"queries", "reports"}, | ||
|
|
||
| // Nested keys in org_settings.mdm.apple_business_manager[] | ||
| {"org_settings.mdm.apple_business_manager[].macos_team", "org_settings.mdm.apple_business_manager[].macos_fleet"}, | ||
| {"org_settings.mdm.apple_business_manager[].ios_team", "org_settings.mdm.apple_business_manager[].ios_fleet"}, | ||
| {"org_settings.mdm.apple_business_manager[].ipados_team", "org_settings.mdm.apple_business_manager[].ipados_fleet"}, | ||
|
|
||
| // Nested keys in org_settings.mdm.volume_purchasing_program[] | ||
| {"org_settings.mdm.volume_purchasing_program[].teams", "org_settings.mdm.volume_purchasing_program[].fleets"}, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the gitops_deprecations.go file to see the actual implementation
cat -n pkg/spec/gitops_deprecations.goRepository: fleetdm/fleet
Length of output: 5769
🏁 Script executed:
# Search for test files related to gitops deprecations
find . -type f -name "*gitops*test*.go" -o -name "*test*gitops*.go" | head -20Repository: fleetdm/fleet
Length of output: 397
🏁 Script executed:
# Check if there are any tests exercising the deprecated mappings
rg -l "DeprecatedGitOpsKeyMappings|migrateKeyPath" --type goRepository: fleetdm/fleet
Length of output: 88
🏁 Script executed:
# Look for tests that use the nested deprecation mappings
rg -A 10 -B 5 "apple_business_manager|volume_purchasing_program" pkg/spec/gitops_test.go cmd/fleetctl/fleetctl/gitops_test.go 2>/dev/null | head -100Repository: fleetdm/fleet
Length of output: 7689
🏁 Script executed:
# Check the integration tests for deprecated fields
rg -A 5 "deprecated" cmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_deprecated_test.go 2>/dev/null | head -80Repository: fleetdm/fleet
Length of output: 2558
🏁 Script executed:
# Search for any test that validates the deprecated key mappings are actually applied
rg "DeprecatedGitOpsKeyMappings|ApplyDeprecatedKeyMappings" --type goRepository: fleetdm/fleet
Length of output: 672
Nested array paths won't migrate due to path format mismatch in parser.
The migrateKeyPathRecursive function detects array iteration by checking oldParts[1] == "[]" (line 71). However, strings.Split("org_settings.mdm.apple_business_manager[].macos_team", ".") produces ["org_settings", "mdm", "apple_business_manager[]", "macos_team"]—the [] remains fused with the key name instead of appearing as a standalone part.
When recursion reaches the level where oldKey = "apple_business_manager[]", the code attempts data["apple_business_manager[]"] (line 75), but the actual map key is "apple_business_manager". The lookup returns nil and returns silently (lines 109–111), meaning the nested ABM and VPP mappings (lines 27–32) never trigger array iteration.
Fix: Either separate the array marker with a dot in the path strings ("org_settings.mdm.apple_business_manager.[].macos_team") or update the parser to strip a trailing [] suffix from key names and treat it as an array marker.
🤖 Prompt for AI Agents
In `@pkg/spec/gitops_deprecations.go` around lines 21 - 33, Deprecated
nested-array mappings in DeprecatedGitOpsKeyMappings fail because
migrateKeyPathRecursive expects array markers as separate parts (oldParts[1] ==
"[]") but key parts like "apple_business_manager[]" keep the "[]" suffix; update
migrateKeyPathRecursive to detect and handle a trailing "[]" on a part: when
splitting path parts, if a part endsWith "[]", strip the "[]" to get baseKey,
use data[baseKey] to access the slice (and iterate over its elements
recursively), and ensure newParts/newKey construction preserves or replaces the
array marker consistently; reference the function migrateKeyPathRecursive,
variables oldParts and oldKey, and the DeprecatedGitOpsKeyMappings entries (e.g.
"org_settings.mdm.apple_business_manager[].macos_team") when making this change.
| // Invalid policies in a separate file | ||
| tmpFile, err = os.CreateTemp(t.TempDir(), "*queries.yml") | ||
| tmpFile, err = os.CreateTemp(t.TempDir(), "*reports.yml") | ||
| require.NoError(t, err) | ||
| _, err = tmpFile.WriteString("[2]") | ||
| require.NoError(t, err) | ||
| config = getConfig([]string{"queries"}) | ||
| config += fmt.Sprintf("%s:\n - path: %s\n", "queries", tmpFile.Name()) | ||
| config = getConfig([]string{"reports"}) | ||
| config += fmt.Sprintf("%s:\n - path: %s\n", "reports", tmpFile.Name()) | ||
| _, err = gitOpsFromString(t, config) | ||
| assert.ErrorContains(t, err, "expected type spec.Query but got number") |
There was a problem hiding this comment.
Stale comment: says "Invalid policies" but tests invalid reports.
The comment on line 776 still says "Invalid policies in a separate file" but the test block (lines 777–784) validates invalid reports in a separate file.
Proposed fix
- // Invalid policies in a separate file
+ // Invalid reports in a separate file
tmpFile, err = os.CreateTemp(t.TempDir(), "*reports.yml")🤖 Prompt for AI Agents
In `@pkg/spec/gitops_test.go` around lines 776 - 784, Update the stale test
comment to accurately describe the test: change the comment text that currently
reads "Invalid policies in a separate file" to something like "Invalid reports
in a separate file" so it matches the test block that writes "[2]" to a temp
file and asserts an error from gitOpsFromString; locate the comment above the
tmpFile creation in the test that uses getConfig, gitOpsFromString, and
assert.ErrorContains and adjust the wording accordingly.
|
|
||
| // The query tag now holds the old (deprecated) name. If the old name | ||
| // was used, log a deprecation warning. If not found, check the | ||
| // renameto value (the new name) as a fallback. | ||
| if queryVal != "" { | ||
| if renameTo, hasRenameTo := fp.Sf.Tag.Lookup("renameto"); hasRenameTo { | ||
| // Log deprecation warning - the old name was used. | ||
| logging.WithLevel(ctx, level.Warn) | ||
| logging.WithExtras(ctx, | ||
| "deprecated_param", queryTagValue, | ||
| "deprecation_warning", fmt.Sprintf("'%s' is deprecated, use '%s' instead", queryTagValue, renameTo), | ||
| ) | ||
| } | ||
| } else { | ||
| if renameTo, hasRenameTo := fp.Sf.Tag.Lookup("renameto"); hasRenameTo { | ||
| renameTo, _, err = ParseTag(renameTo) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| queryVal = r.URL.Query().Get(renameTo) | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing conflict check when both deprecated and new query parameter names are provided.
The PR objectives state that if both the deprecated and the new name are provided, an error should be returned. For JSON bodies this is handled by JSONKeyRewriteReader / AliasConflictError, but for query parameters only the deprecated name is used while the new name is silently ignored.
If a client sends ?old_name=foo&new_name=bar, the old value wins without any error. This is inconsistent with the JSON-body behavior and could mask client mistakes.
Suggested fix
if queryVal != "" {
if renameTo, hasRenameTo := fp.Sf.Tag.Lookup("renameto"); hasRenameTo {
+ renameParsed, _, _ := ParseTag(renameTo)
+ if newVal := r.URL.Query().Get(renameParsed); newVal != "" {
+ return &platform_http.BadRequestError{
+ Message: fmt.Sprintf("Specify only one of %q or %q", queryTagValue, renameParsed),
+ }
+ }
// Log deprecation warning - the old name was used.
logging.WithLevel(ctx, level.Warn)🤖 Prompt for AI Agents
In `@server/platform/endpointer/endpoint_utils.go` around lines 310 - 331, The
current query-parameter handling in the block around queryVal/queryTagValue and
fp.Sf.Tag.Lookup("renameto") ignores the new name when both deprecated and new
query params are present; update the logic to detect when both the deprecated
name (queryTagValue) and the parsed renameto name (use ParseTag(renameTo) to get
the actual newName) are present in r.URL.Query(), and if so return the same
AliasConflictError used by JSONKeyRewriteReader (construct and return
AliasConflictError with both names); otherwise preserve existing behavior (warn
when only old present, use new when only new present). Ensure you reference
queryVal, queryTagValue, fp.Sf.Tag.Lookup("renameto"), ParseTag(renameTo), and
r.URL.Query().Get(newName) when implementing the check.
|
|
||
| // Ensure AI features are enabled (may have been disabled by a previous test). | ||
| s.Do("PATCH", "/api/latest/fleet/config", json.RawMessage(`{ | ||
| "server_settings": {"ai_features_disabled": false} | ||
| }`), http.StatusOK) |
There was a problem hiding this comment.
Restore AI feature flag after the test to avoid state leakage.
This test mutates global config and currently leaves AI features disabled for subsequent tests. Capture the original value and restore it in t.Cleanup.
🔧 Suggested fix (restore original config)
func (s *integrationTestSuite) TestAutofillPolicies() {
t := s.T()
// Ensure AI features are enabled (may have been disabled by a previous test).
+ origCfg := s.getConfig()
+ origAIDisabled := origCfg.ServerSettings.AIFeaturesDisabled
+ t.Cleanup(func() {
+ s.Do("PATCH", "/api/latest/fleet/config", map[string]map[string]bool{
+ "server_settings": {"ai_features_disabled": origAIDisabled},
+ }, http.StatusOK)
+ })
s.Do("PATCH", "/api/latest/fleet/config", json.RawMessage(`{
"server_settings": {"ai_features_disabled": false}
}`), http.StatusOK)🤖 Prompt for AI Agents
In `@server/service/integration_core_test.go` around lines 14122 - 14126, The test
mutates the global AI feature flag via s.Do("PATCH", "/api/latest/fleet/config",
...) and leaves it disabled for later tests; capture the original
server_settings.ai_features_disabled value before patching, and register a
t.Cleanup that restores it by issuing a PATCH (using the same s.Do helper and
json.RawMessage payload) to set ai_features_disabled back to the original
boolean; use t.Cleanup to ensure restoration even if the test fails.
| // Duplicate JSON keys so that stored activity details include both the | ||
| // old and new field names (e.g. team_id and fleet_id). | ||
| if rules := endpointer.ExtractAliasRules(activity); len(rules) > 0 { | ||
| detailsBytes = endpointer.DuplicateJSONKeys(detailsBytes, rules) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -name "json_key_duplicator.go" -type fRepository: fleetdm/fleet
Length of output: 287
🏁 Script executed:
rg -n "DuplicateJSONKeys" --type=go -B2 -A15Repository: fleetdm/fleet
Length of output: 17807
🏁 Script executed:
rg -n "WithIndent" --type=go -B2 -A2Repository: fleetdm/fleet
Length of output: 2606
DuplicateJSONKeys hardcodes indentation, creating inconsistent JSON formatting in stored activity details.
json.Marshal on line 51 produces compact JSON, but DuplicateJSONKeys uses jsontext.NewEncoder with jsontext.WithIndent(" ") (line 36 of json_key_duplicator.go). Activities with alias rules are re-encoded to pretty-printed JSON, while activities without rules retain compact JSON. This causes:
- Inconsistent formatting of stored activity details across rows
- Increased storage size for rows with alias rules due to added whitespace
- Potential issues with downstream tools performing string comparison on details
Consider making the indentation configurable or using compact output to maintain consistent storage format.
🤖 Prompt for AI Agents
In `@server/service/modules/activities/activities.go` around lines 55 - 59,
DuplicateJSONKeys currently re-encodes JSON with pretty indentation causing
inconsistent formatting; change DuplicateJSONKeys (json_key_duplicator.go) to
produce compact JSON by removing jsontext.WithIndent(" ") or by adding a
parameter/option (e.g., compact bool or indent string) so callers can request
compact output, then update the caller in activities.go where DuplicateJSONKeys
is invoked (after endpointer.ExtractAliasRules) to call it with the compact
option (or rely on the default compact behavior). Ensure the function still
applies alias rules and returns bytes in compact form to keep stored activity
details consistent with json.Marshal output.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## sgress454/39344-rename-tags-only #39873 +/- ##
====================================================================
- Coverage 64.60% 62.34% -2.26%
====================================================================
Files 2438 2408 -30
Lines 195399 188405 -6994
Branches 8604 8604
====================================================================
- Hits 126230 117458 -8772
- Misses 57569 60255 +2686
+ Partials 11600 10692 -908
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:
|
1 participant
Related issue: For #39344
Details
This PR builds on the previous PR (#39847) which added
renametotags to certain API parameters to mark them as deprecated. How this is used:In requests
jsonorqueryparam is used that has arenametotag, e.g. if ateam_idparam is sent but the related struct hasrenameto:"fleet_id"in it.renamedtoversion (e.g.fleet_id) is sent in the request, rewrite it to the deprecated name so that it can be unmarshalled into the structteam_idANDfleet_id), throw an error and quitIn responses
renametotags, so that we don't break existing workflows expecting the old keys.In fleetctl
fleetctl getandfleetctl generate-gitops, only output the new namesChecklist for submitter
If some of the following don't apply, delete the relevant line.
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements)If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes
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
For unreleased bug fixes in a release candidate, one of:
Database migrations
COLLATE utf8mb4_unicode_ci).New Fleet configuration settings
If you didn't check the box above, follow this checklist for GitOps-enabled settings:
fleetctl generate-gitopsfleetd/orbit/Fleet Desktop
runtime.GOOSis used as needed to isolate changesSummary by CodeRabbit
New Features
Bug Fixes
Refactor