Allow disabling historical data collection (GitOps / API)#44488
Allow disabling historical data collection (GitOps / API)#44488
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.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds per-dataset historical-data feature flags to Features via a new HistoricalDataSettings type (Uptime, Vulnerabilities, Enabled), persists them in app and team config, and provides partial-patch payloads (HistoricalDataPayload with optjson.Bool) for team updates. Ensures GitOps apply injects defaults for missing historical_data keys. Introduces EmitHistoricalDataActivities and two new activity detail types to emit enable/disable events when historical-data flags change. Integrates emission into ModifyTeam and ModifyAppConfig, and adds unit/integration tests plus updated test fixtures and GitOps/YAML outputs. Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/service/client.go`:
- Around line 3464-3482: The helper ensureHistoricalDataDefaults currently
silently replaces non-map values at features["historical_data"] (e.g., scalars
or lists) with a default map; change it to validate the type instead of
overwriting: update ensureHistoricalDataDefaults to return an error, detect if
features["historical_data"] is present but not a map[string]any (and not nil),
and return a clear fmt.Errorf indicating the malformed value (include the actual
value/type); only when the key is absent or nil create and inject the default
map and continue; update any callers to handle/propagate the returned error from
ensureHistoricalDataDefaults.
In `@server/service/integration_core_test.go`:
- Around line 1039-1061: After reading previousRawConfig, register a guaranteed
cleanup that restores app_config_json so failures in the subsequent assertions
don't leave DB mutated: immediately after capturing previousRawConfig call
t.Cleanup with a closure that uses mysql.ExecAdhocSQL (same restore SQL and
parameters used later) against s.ds to re-insert previousRawConfig; remove or
keep the later ad-hoc restore block as appropriate to avoid duplicate restores.
Ensure the closure captures previousRawConfig and ctx so the restore uses the
original value.
In `@server/service/integration_enterprise_test.go`:
- Around line 1764-1768: The test only asserts the single boolean stayed the
same; instead capture the entire prior features and historical data before the
PATCH (e.g. priorFeatures := modResp.Team.Config.Features and
priorHistoricalData := modResp.Team.Config.HistoricalData), perform the PATCH
via s.DoJSON, then assert the whole features struct and historical data are
unchanged using require.Equal (compare priorFeatures to
modResp.Team.Config.Features and priorHistoricalData to
modResp.Team.Config.HistoricalData) and also assert no new activity was emitted
(check the activity store/listing for no additional entries created during the
request). Use the existing modResp and s.DoJSON call and replace the
single-field assertion on priorEnableHostUsers with these full-struct and
activity checks.
🪄 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: CHILL
Plan: Pro
Run ID: b5276c02-a34e-4871-9db2-216f4d1de26d
📒 Files selected for processing (11)
cmd/fleetctl/fleetctl/gitops_test.goee/server/service/teams.goserver/fleet/activities.goserver/fleet/app.goserver/fleet/app_test.goserver/fleet/historical_data.goserver/fleet/teams.goserver/service/appconfig.goserver/service/client.goserver/service/integration_core_test.goserver/service/integration_enterprise_test.go
There was a problem hiding this comment.
Pull request overview
Adds a new features.historical_data configuration block (global + per-fleet) to control collection for dashboard chart datasets, including GitOps default-injection and audit activities when toggles change.
Changes:
- Introduce
features.historical_data.{uptime,vulnerabilities}with defaults totrue. - Emit
enabled_historical_dataset/disabled_historical_datasetactivities when either sub-key flips (global and team-scoped). - Ensure GitOps applies default
historical_datasub-keys totruewhen omitted in YAML, and add integration/unit test coverage.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/integration_enterprise_test.go | Adds EE integration tests for per-fleet PATCH + activities + spec apply round-trip. |
| server/service/integration_core_test.go | Adds core integration test for global config defaults, PATCH semantics, and activities. |
| server/service/client.go | Injects GitOps defaults for features.historical_data (global + team). |
| server/service/appconfig.go | Emits historical-data enable/disable activities on global config modify. |
| server/fleet/teams.go | Extends TeamPayload to accept per-fleet features.historical_data PATCH payload. |
| server/fleet/historical_data.go | Adds shared helper to emit activities per sub-key flip. |
| server/fleet/app_test.go | Adds unit tests for defaults/copy behavior and dataset-name mapping. |
| server/fleet/app.go | Adds HistoricalDataSettings to Features, defaulting behavior, and internal-name mapping. |
| server/fleet/activities.go | Registers and defines new activity types for historical dataset toggles. |
| ee/server/service/teams.go | Applies per-fleet PATCH merge semantics for historical_data and emits activities. |
| cmd/fleetctl/fleetctl/gitops_test.go | Adds fleetctl GitOps tests for default injection + explicit historical_data application. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #44488 +/- ##
==========================================
- Coverage 66.61% 65.35% -1.26%
==========================================
Files 2657 2658 +1
Lines 214087 214185 +98
Branches 9820 9820
==========================================
- Hits 142605 139977 -2628
- Misses 58510 61220 +2710
- Partials 12972 12988 +16
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:
|
sgress454
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/fleetctl/fleetctl/testdata/expectedGetTeamsJson.json`:
- Around line 11-14: The fixture JSON has inverted defaults for the
historical_data feature: change the "historical_data" object's "uptime" and
"vulnerabilities" values from false to true in the expectedGetTeamsJson.json
test fixtures (update all occurrences where "historical_data": { "uptime":
false, "vulnerabilities": false } appears, e.g., the blocks around lines shown)
so the fixture matches the PR intent that features.historical_data.uptime and
features.historical_data.vulnerabilities default to true.
🪄 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: CHILL
Plan: Pro
Run ID: d4409e9f-5def-44d2-9a13-e28e8e0d0e7c
📒 Files selected for processing (21)
cmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigJson.jsoncmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigTeamMaintainerJson.jsoncmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigTeamMaintainerYaml.ymlcmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigYaml.ymlcmd/fleetctl/fleetctl/testdata/expectedGetConfigIncludeServerConfigJson.jsoncmd/fleetctl/fleetctl/testdata/expectedGetConfigIncludeServerConfigYaml.ymlcmd/fleetctl/fleetctl/testdata/expectedGetTeamsJson.jsoncmd/fleetctl/fleetctl/testdata/expectedGetTeamsYaml.ymlcmd/fleetctl/fleetctl/testdata/generateGitops/expectedOrgSettings-insecure.yamlcmd/fleetctl/fleetctl/testdata/generateGitops/expectedOrgSettings.yamlcmd/fleetctl/fleetctl/testdata/generateGitops/expectedTeamSettings-insecure.yamlcmd/fleetctl/fleetctl/testdata/generateGitops/expectedTeamSettings.yamlcmd/fleetctl/fleetctl/testdata/generateGitops/test_dir_free/default.ymlcmd/fleetctl/fleetctl/testdata/generateGitops/test_dir_premium/default.ymlcmd/fleetctl/fleetctl/testdata/generateGitops/test_dir_premium/fleets/team-a-thumbsup.ymlcmd/fleetctl/fleetctl/testdata/macosSetupExpectedAppConfigEmpty.ymlcmd/fleetctl/fleetctl/testdata/macosSetupExpectedAppConfigSet.ymlcmd/fleetctl/fleetctl/testdata/macosSetupExpectedTeam1And2Empty.ymlcmd/fleetctl/fleetctl/testdata/macosSetupExpectedTeam1And2Set.ymlcmd/fleetctl/fleetctl/testdata/macosSetupExpectedTeam1Empty.ymlcmd/fleetctl/fleetctl/testdata/macosSetupExpectedTeam1Set.yml
✅ Files skipped from review due to trivial changes (19)
- cmd/fleetctl/fleetctl/testdata/expectedGetConfigIncludeServerConfigYaml.yml
- cmd/fleetctl/fleetctl/testdata/generateGitops/expectedOrgSettings.yaml
- cmd/fleetctl/fleetctl/testdata/generateGitops/test_dir_free/default.yml
- cmd/fleetctl/fleetctl/testdata/macosSetupExpectedAppConfigEmpty.yml
- cmd/fleetctl/fleetctl/testdata/generateGitops/test_dir_premium/default.yml
- cmd/fleetctl/fleetctl/testdata/generateGitops/expectedTeamSettings-insecure.yaml
- cmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigJson.json
- cmd/fleetctl/fleetctl/testdata/macosSetupExpectedAppConfigSet.yml
- cmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigTeamMaintainerJson.json
- cmd/fleetctl/fleetctl/testdata/generateGitops/expectedTeamSettings.yaml
- cmd/fleetctl/fleetctl/testdata/macosSetupExpectedTeam1And2Empty.yml
- cmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigYaml.yml
- cmd/fleetctl/fleetctl/testdata/generateGitops/test_dir_premium/fleets/team-a-thumbsup.yml
- cmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigTeamMaintainerYaml.yml
- cmd/fleetctl/fleetctl/testdata/macosSetupExpectedTeam1Set.yml
- cmd/fleetctl/fleetctl/testdata/expectedGetConfigIncludeServerConfigJson.json
- cmd/fleetctl/fleetctl/testdata/expectedGetTeamsYaml.yml
- cmd/fleetctl/fleetctl/testdata/macosSetupExpectedTeam1And2Set.yml
- cmd/fleetctl/fleetctl/testdata/macosSetupExpectedTeam1Empty.yml
sgress454
force-pushed
the
sgress454/44077-chart-disabling-gitops-api
branch
from
fb54888 to
353e4e4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/service/integration_core_test.go`:
- Around line 994-1028: The tests are using lastActivityOfTypeMatches(..., 0)
after PATCHes which can read stale rows; before each mutating Do("PATCH",
"/api/latest/fleet/config") call capture the prior activity id/count with
lastActivityOfTypeMatches for the relevant activity types (use
ActivityTypeEnabledHistoricalDataset{}.ActivityName() and
ActivityTypeDisabledHistoricalDataset{}.ActivityName()) like you already do for
the no-op check (priorActivityID), then perform the PATCH and assert that
lastActivityOfTypeMatches returns a different/new id (or increased count) for
the expected activity type(s); apply this snapshot-and-compare pattern around
the PATCH at the flip-both step (and for both enabled and disabled dataset
checks) and reuse getConfig() to validate toggles as before.
In `@server/service/integration_enterprise_test.go`:
- Around line 1771-1775: Update the PATCH payload in the s.DoJSON call so it
flips "enable_host_users" to true (instead of false) to make the request
non-noop; locate the call to s.DoJSON(fmt.Sprintf("/api/latest/fleet/fleets/%d",
teamID), json.RawMessage(`{"features": {"enable_host_users": false}}`), ...) and
change the JSON to `{"features": {"enable_host_users": true}}` so the assertion
comparing priorFeatures.EnableHostUsers to
modResp.Team.Config.Features.EnableHostUsers will catch accidental acceptance of
unknown/ignored feature sub-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: CHILL
Plan: Pro
Run ID: 70be55f5-6fe5-4de7-9ffb-0aa083c22972
📒 Files selected for processing (3)
server/service/integration_core_test.goserver/service/integration_enterprise_test.goserver/service/teams_test.go
sgress454
requested review from
georgekarrv,
getvictor,
lucasmrod,
lukeheath and
sharon-fdm
as code owners
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/test-go-suite.yaml:
- Around line 249-254: preview_test.go is still using the hardcoded path
"tools/osquery/in-a-box" for --preview-config-path so the /tmp/preview-config
changes are ignored; update preview_test.go to read the
FLEET_PREVIEW_CONFIG_PATH environment variable (and/or use it as the default for
the existing --preview-config-path flag) and fall back to the repo path only if
the env var is unset, ensuring the test uses the /tmp/preview-config when the
workflow sets FLEET_PREVIEW_CONFIG_PATH; locate where the preview config flag or
variable (e.g., the --preview-config-path flag or a variable like
previewConfigPath) is parsed/used in preview_test.go and switch to/env-check or
default-to the env var there.
In `@server/datastore/mysql/schema.sql`:
- Line 224: The seeded row in app_config_json in
server/datastore/mysql/schema.sql incorrectly hardcodes "historical_data":
{"uptime": false, "vulnerabilities": false}—this was introduced by a hand-edited
snapshot instead of the migration and bypasses the legacy/default-injection
path; fix it by updating the migration that adds or backfills the app config
(the migration that defines the default app_config_json or backfills missing
keys) to leave historical_data absent or set to the intended default behavior,
run the project's migration-to-schema generator to regenerate
server/datastore/mysql/schema.sql, and ensure the INSERT for app_config_json in
the regenerated file matches the migration behavior (do not manually edit
schema.sql).
🪄 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: CHILL
Plan: Pro
Run ID: 6e57e8cd-4382-4e75-8378-01543bedad67
📒 Files selected for processing (8)
.github/workflows/test-go-suite.yamlcmd/fleetctl/integrationtest/preview/preview_test.goserver/datastore/mysql/schema.sqlserver/service/integration_core_test.goserver/service/integration_enterprise_test.gotools/cloner-check/generated_files/appconfig.txttools/cloner-check/generated_files/features.txttools/cloner-check/generated_files/teamconfig.txt
✅ Files skipped from review due to trivial changes (3)
- tools/cloner-check/generated_files/appconfig.txt
- tools/cloner-check/generated_files/features.txt
- server/service/integration_core_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- server/service/integration_enterprise_test.go
sgress454
force-pushed
the
sgress454/44077-chart-disabling-gitops-api
branch
from
7429455 to
1644afe
Compare
There was a problem hiding this comment.
This updates our Go Test harness to build a fleet preview image from source, like we now do in https://github.com/fleetdm/fleet/blob/main/.github/workflows/fleetctl-preview-latest.yml, so that new GitOps config doesn't fail the preview test with "unknown keys" errors.
There was a problem hiding this comment.
Update the preview test to use the image we built in the harness, rather than the published image. We have a separate CI test (https://github.com/fleetdm/fleet/actions/workflows/fleetctl-preview.yml) that checks against the published image nightly.
| oldHistoricalData := team.Config.Features.HistoricalData | ||
| if payload.Features != nil && payload.Features.HistoricalData != nil { | ||
| if payload.Features.HistoricalData.Uptime.Valid { | ||
| team.Config.Features.HistoricalData.Uptime = payload.Features.HistoricalData.Uptime.Value | ||
| } | ||
| if payload.Features.HistoricalData.Vulnerabilities.Valid { | ||
| team.Config.Features.HistoricalData.Vulnerabilities = payload.Features.HistoricalData.Vulnerabilities.Value | ||
| } | ||
| } |
There was a problem hiding this comment.
Pretty common pattern ModifyTeam, since it's a PATCH API.
There was a problem hiding this comment.
Set the charts to true (enabled) when we ship, so existing installations get it automatically and we don't have to worry about other migrations accidentally flipping it to false because it doesn't have a value yet.
Since this is unreleased it's far easier to update the existing migration than add a new one.
There was a problem hiding this comment.
:( don't love editing migrations, but definitely easier in this case. 👍
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.
sgress454
force-pushed
the
sgress454/44077-chart-disabling-gitops-api
branch
from
5724565 to
9bd7d06
Compare
ksykulev
left a comment
ksykulev
left a comment
There was a problem hiding this comment.
Few small comments. Mostly looking good though!! 🚀
There was a problem hiding this comment.
:( don't love editing migrations, but definitely easier in this case. 👍
| EnableSoftwareInventory bool `json:"enable_software_inventory"` | ||
| AdditionalQueries *json.RawMessage `json:"additional_queries,omitempty"` | ||
| DetailQueryOverrides map[string]*string `json:"detail_query_overrides,omitempty"` | ||
| HistoricalData HistoricalDataSettings `json:"historical_data"` |
There was a problem hiding this comment.
Do we need an omitempty here
json:"historical_data,omitempty"
There was a problem hiding this comment.
Not for this one, because it's made up of booleans so we always want to show what the values are in responses. That is, we'd want to show historical_data: { uptime: false, vulnerabilities: false } rather than leaving it out. In reality we're coercing empty values to true in ApplyDefaults anyway so this should never actually be empty.
sgress454
force-pushed
the
sgress454/44077-chart-disabling-gitops-api
branch
from
9bd7d06 to
7694700
Compare
Related issue: For #44077
Details
historical_datakey to app and team config (and gitops) withuptimeandvulnerabilitiessubkeys. Keys default totrue, meaning "collect this data"enabled_historical_datasetanddisabled_historical_datasetactivities when these values are flipped via GitOps or the config APIsThe majority of the file changes in here are GitOps test files that need to be updated to have the new config in them.
This PR does not implement using these configs to actually disable data collection or purge data; that will come in a follow-up PR (as well as the front-end)
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
n/a, unreleased
Testing
Added/updated automated tests
QA'd all new/changed functionality manually
Defaults
GET /api/v1/fleet/configreturnsfeatures.historical_data.uptime: trueandfeatures.historical_data.vulnerabilities: truePOST /api/v1/fleet/teams, thenGET /api/v1/fleet/fleets/{id}returnsfeatures.historical_data.uptime: trueandfeatures.historical_data.vulnerabilities: trueGlobal PATCH (
POST /api/v1/fleet/config){"features": {"historical_data": {"vulnerabilities": false}}}—vulnerabilitiesflipped tofalse,uptimeunchanged attrue{"features": {"historical_data": {"uptime": false, "vulnerabilities": true}}}— both values applied as sent{"features": {"historical_data": {"vulnerabilites": false}}}(typo in sub-key) — request rejected with 4xx, stored config unchangedFleet PATCH (
PATCH /api/v1/fleet/fleets/{id}){"features": {"historical_data": {"uptime": false}}}— fleet'suptimeflipped tofalse,vulnerabilitiesunchangedGET /api/v1/fleet/fleets/{id}returns the toggled values underfeatures.historical_data(storage shape is symmetric with global){"features": {"enable_host_users": false}}(a non-historical_datafeatures sub-field) — request returned 200 but the fleet'senable_host_usersis unchanged (silently ignored, per existing endpoint convention)GitOps — global (
fleetctl gitops -f global.yml)features.historical_data: {uptime: true, vulnerabilities: false}—vulnerabilitiesisfalseafter apply,uptimeistrueorg_settingsomitsfeaturesentirely — both sub-keys aretrueafter apply (defaults injected even if previously disabled)historical_dataonly containsuptime: false—uptime: falseis honored,vulnerabilitiesdefaults totruevulnerabilitiesvia the API, then ranfleetctl gitopswith a YAML that doesn't pin it —vulnerabilitiesflips back totrue(this is intentional; gitops is the source of truth)GitOps — fleet
features.historical_data: {uptime: false}— that fleet hasuptime: false,vulnerabilities: trueafter applyteam_settings.featuresomitshistorical_data— both sub-keys aretrueafter applyfeaturesentirely — both sub-keys aretrueafter applyfleetctl apply(legacy, partial-merge)vulnerabilitiesvia the API, then ranfleetctl applywith a YAML that doesn't mentionhistorical_data—vulnerabilitiesis stillfalse(apply leaves omitted fields alone)Activities — global
vulnerabilities, the latest activity isdisabled_historical_datasetwith payload{"dataset": "vulnerabilities", "fleet_id": null, "fleet_name": null}enabled_historical_datasetActivities — per fleet
workstationsto disableuptime, the activity isdisabled_historical_datasetwith payload{"dataset": "uptime", "fleet_id": <workstations id>, "fleet_name": "workstations"}For unreleased bug fixes in a release candidate, one of:
New Fleet configuration settings
If you didn't check the box above, follow this checklist for GitOps-enabled settings:
fleetctl generate-gitopsSummary by CodeRabbit
New Features
Bug Fixes
Tests