Add include_all label scope to GitOps and fleetctl (#41566) - #44534
Conversation
GitHub Actions
Inactive
GitHub Actions
Inactive
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
juan-fdz-hawa
changed the title
|
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 a new labels_include_all label scope for policies and queries across CLI (fleetctl), GitOps generation/parsing, service API, and datastore layers. Changes include new exported fields on QuerySpec and PolicySpec, spec-level Verify validation enforcing mutual exclusivity of label scopes, premium license gating for labels_include_all at service and GitOps entrypoints, serialization/deserialization and GitOps YAML/JSON generation for labels_include_all, datastore persistence and policy membership updates for include-all, SQL filtering for host reports to support require-all, helper conversion functions, and unit and integration tests. 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
🧹 Nitpick comments (5)
changes/41566-policy-report-labels-gitops (1)
1-1: ⚡ Quick winUse user-facing language in the changelog entry.
The word "Wires" is developer jargon that doesn't clearly communicate the user-visible change. Changelog entries should describe what users can now do, not implementation details.
📝 Suggested rewording for clarity
-- Wires labels_include_all to GitOps/fleetctl for policies and reports. +- Added support for `labels_include_all` label scope in GitOps and fleetctl for policies and reports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@changes/41566-policy-report-labels-gitops` at line 1, Change the changelog entry that currently reads "Wires labels_include_all to GitOps/fleetctl for policies and reports" to user-facing language that describes the new capability; for example, replace "Wires" with a clear phrase such as "Add support for" or "Allow users to include" so the entry reads like "Add support for labels_include_all in GitOps/fleetctl for policies and reports" (referencing the labels_include_all flag, GitOps/fleetctl integration, and the policies and reports feature to locate the entry).cmd/fleetctl/fleetctl/get_test.go (1)
1891-1945: ⚡ Quick winAdd one default table-output assertion for
labels_include_all.These tests cover
--yaml/--json, but the table behavior added at Line 352 incmd/fleetctl/fleetctl/get.goisn’t exercised yet. A plainfleetctl get reportsassertion forlabels_include_allwould close that gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/fleetctl/fleetctl/get_test.go` around lines 1891 - 1945, TestGetReportsLabelsIncludeAll currently asserts only YAML/JSON outputs but not the default table output; add a table-output assertion by calling RunAppForTest(t, []string{"get", "reports"}) and asserting the returned string contains the labels (e.g. assert.Contains(t, out, "labelA") and assert.Contains(t, out, "labelB") or a combined "labelA,labelB" as appropriate) so the new table formatting in the get reports command is exercised; update TestGetReportsLabelsIncludeAll to include these assert calls referencing RunAppForTest and the "get reports" invocation.server/service/queries.go (1)
824-827: ⚡ Quick winReturn the missing label names here.
label not foundis too generic now that bothlabels_include_anyandlabels_include_allfunnel through this branch. Including the missing names would make GitOps/API failures much easier to fix.Suggested change
- for _, name := range allLabelNames { - if _, ok := labelsMap[name]; !ok { - return nil, ctxerr.New(ctx, "label not found") - } - } + missing := make([]string, 0) + for _, name := range allLabelNames { + if _, ok := labelsMap[name]; !ok { + missing = append(missing, name) + } + } + if len(missing) > 0 { + return nil, ctxerr.New(ctx, fmt.Sprintf("labels not found: %s", strings.Join(missing, ", "))) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/queries.go` around lines 824 - 827, The loop over allLabelNames that currently returns a generic ctxerr.New(ctx, "label not found") should collect the actual missing label names (e.g., build a slice missing := []string{} when iterating for _, name := range allLabelNames and checking labelsMap[name]) and return an error that includes them (e.g., fmt.Join or formatted message) instead of the generic string; update the return to use ctxerr.Newf or ctxerr.New with a formatted message like "labels not found: <names>" so callers of the function that contains this loop (the block using labelsMap and allLabelNames) get the specific missing label names.cmd/fleetctl/fleetctl/gitops_test.go (1)
295-334: ⚡ Quick winAdd a free-tier
labels_include_allGitOps test.These new cases only exercise
labels_include_allwith a premium license. The existing free-tier coverage still only protectslabels_include_any, so the new license-gated branch can regress without a failing test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/fleetctl/fleetctl/gitops_test.go` around lines 295 - 334, Add a new test function (e.g., TestGitOpsQueryLabelsIncludeAllFreeTier) that mirrors TestGitOpsQueryLabelsIncludeAllUnknownLabel but sets the license to fleet.TierFree (use RunServerWithMockedDS/TestServerOpts like the existing test), creates the same GitOps YAML that uses labels_include_all, calls RunAppNoChecks with that file, and assert that it errors and the error message mentions the license/gating of labels_include_all (check Error and ErrorContains against text including "labels_include_all" and "license" or "premium"). This ensures the labels_include_all license-gated branch is covered; reference the existing TestGitOpsQueryLabelsIncludeAllUnknownLabel, labels_include_all, RunServerWithMockedDS, and RunAppNoChecks to locate where to add the duplicate test.server/service/integration_enterprise_test.go (1)
29473-29493: ⚡ Quick winAdd explicit non-persistence assertions for rejected single-spec GitOps requests
These negative tests currently validate only the error response. Please also assert the rejected policy/query names were not created, so partial-write regressions are caught (same guarantee you already enforce in the batch test).
Also applies to: 29589-29599
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/integration_enterprise_test.go` around lines 29473 - 29493, Add explicit assertions that the rejected spec names were not persisted: after each POST that returns BadRequest (the requests captured in rejSpecResp and rejSpecResp2 sent with fleet.ApplyPolicySpecsRequest containing fleet.PolicySpec named "spec-rej-any-"+t.Name() and "spec-rej-excl-"+t.Name()), call the API to fetch specs (e.g., via s.Do GET /api/latest/fleet/spec/policies or the single-spec read endpoint) and assert the returned list does not contain those policy names; use the same helper extractServerErrorText and compare against the PolicySpec.Name values and fleet.ErrPolicyConflictingLabels to ensure no partial write occurred.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@changes/41566-policy-report-labels-gitops`:
- Line 1: Change the changelog entry that currently reads "Wires
labels_include_all to GitOps/fleetctl for policies and reports" to user-facing
language that describes the new capability; for example, replace "Wires" with a
clear phrase such as "Add support for" or "Allow users to include" so the entry
reads like "Add support for labels_include_all in GitOps/fleetctl for policies
and reports" (referencing the labels_include_all flag, GitOps/fleetctl
integration, and the policies and reports feature to locate the entry).
In `@cmd/fleetctl/fleetctl/get_test.go`:
- Around line 1891-1945: TestGetReportsLabelsIncludeAll currently asserts only
YAML/JSON outputs but not the default table output; add a table-output assertion
by calling RunAppForTest(t, []string{"get", "reports"}) and asserting the
returned string contains the labels (e.g. assert.Contains(t, out, "labelA") and
assert.Contains(t, out, "labelB") or a combined "labelA,labelB" as appropriate)
so the new table formatting in the get reports command is exercised; update
TestGetReportsLabelsIncludeAll to include these assert calls referencing
RunAppForTest and the "get reports" invocation.
In `@cmd/fleetctl/fleetctl/gitops_test.go`:
- Around line 295-334: Add a new test function (e.g.,
TestGitOpsQueryLabelsIncludeAllFreeTier) that mirrors
TestGitOpsQueryLabelsIncludeAllUnknownLabel but sets the license to
fleet.TierFree (use RunServerWithMockedDS/TestServerOpts like the existing
test), creates the same GitOps YAML that uses labels_include_all, calls
RunAppNoChecks with that file, and assert that it errors and the error message
mentions the license/gating of labels_include_all (check Error and ErrorContains
against text including "labels_include_all" and "license" or "premium"). This
ensures the labels_include_all license-gated branch is covered; reference the
existing TestGitOpsQueryLabelsIncludeAllUnknownLabel, labels_include_all,
RunServerWithMockedDS, and RunAppNoChecks to locate where to add the duplicate
test.
In `@server/service/integration_enterprise_test.go`:
- Around line 29473-29493: Add explicit assertions that the rejected spec names
were not persisted: after each POST that returns BadRequest (the requests
captured in rejSpecResp and rejSpecResp2 sent with fleet.ApplyPolicySpecsRequest
containing fleet.PolicySpec named "spec-rej-any-"+t.Name() and
"spec-rej-excl-"+t.Name()), call the API to fetch specs (e.g., via s.Do GET
/api/latest/fleet/spec/policies or the single-spec read endpoint) and assert the
returned list does not contain those policy names; use the same helper
extractServerErrorText and compare against the PolicySpec.Name values and
fleet.ErrPolicyConflictingLabels to ensure no partial write occurred.
In `@server/service/queries.go`:
- Around line 824-827: The loop over allLabelNames that currently returns a
generic ctxerr.New(ctx, "label not found") should collect the actual missing
label names (e.g., build a slice missing := []string{} when iterating for _,
name := range allLabelNames and checking labelsMap[name]) and return an error
that includes them (e.g., fmt.Join or formatted message) instead of the generic
string; update the return to use ctxerr.Newf or ctxerr.New with a formatted
message like "labels not found: <names>" so callers of the function that
contains this loop (the block using labelsMap and allLabelNames) get the
specific missing label names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 86db80c1-58bc-4b6d-91b0-7cc95e932b01
📥 Commits
Reviewing files that changed from the base of the PR and between f70a02a and 6a694b3118cae68f425c18b03c4958b1a0efb636.
📒 Files selected for processing (13)
changes/41566-policy-report-labels-gitopscmd/fleetctl/fleetctl/get.gocmd/fleetctl/fleetctl/get_test.gocmd/fleetctl/fleetctl/gitops.gocmd/fleetctl/fleetctl/gitops_test.goserver/datastore/mysql/policies.goserver/datastore/mysql/policies_test.goserver/fleet/policies.goserver/fleet/queries.goserver/service/global_policies.goserver/service/integration_core_test.goserver/service/integration_enterprise_test.goserver/service/queries.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #44534 +/- ##
==========================================
- Coverage 66.68% 66.67% -0.02%
==========================================
Files 2664 2664
Lines 214602 214653 +51
Branches 9839 9839
==========================================
+ Hits 143112 143123 +11
- Misses 58470 58502 +32
- Partials 13020 13028 +8
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:
|
6a694b3 to
d59fe27
Compare
![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.
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 `@server/service/queries.go`:
- Around line 711-728: Reject nil/malformed query specs before dereferencing
them: in the loops that iterate over specs (the pre-license premium check and
the subsequent conversion loop that calls spec.Verify()), first check for spec
== nil and if so call setAuthCheckedOnPreAuthErr(ctx) and return a
fleet.BadRequestError (via ctxerr.Wrap with a message like "invalid query spec:
nil"). Ensure both places reference the same behavior so no nil dereference
occurs in the license check (where LabelsIncludeAny/All are read) and before
calling spec.Verify().
🪄 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: 08d3b968-9bea-4132-8b72-c763c8718142
📥 Commits
Reviewing files that changed from the base of the PR and between 6a694b3118cae68f425c18b03c4958b1a0efb636 and d59fe27.
📒 Files selected for processing (14)
changes/41566-policy-report-labels-gitopscmd/fleetctl/fleetctl/get.gocmd/fleetctl/fleetctl/get_test.gocmd/fleetctl/fleetctl/gitops.gocmd/fleetctl/fleetctl/gitops_test.goserver/datastore/mysql/policies.goserver/datastore/mysql/policies_test.goserver/fleet/labels.goserver/fleet/policies.goserver/fleet/queries.goserver/service/global_policies.goserver/service/integration_core_test.goserver/service/integration_enterprise_test.goserver/service/queries.go
✅ Files skipped from review due to trivial changes (2)
- changes/41566-policy-report-labels-gitops
- cmd/fleetctl/fleetctl/get_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- server/fleet/policies.go
- cmd/fleetctl/fleetctl/get.go
- cmd/fleetctl/fleetctl/gitops.go
- cmd/fleetctl/fleetctl/gitops_test.go
5755053 to
0e9e38a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/service/integration_enterprise_test.go (1)
29458-29471: ⚡ Quick winAssert the exact label set, not just the count.
These checks would still pass if the wrong labels were persisted or if the same label appeared twice. Verifying the actual label names/IDs here would make the new coverage catch mapping regressions instead of only cardinality regressions. The same tightening applies to the similar
Len(...)assertions in the batch-success case.Also applies to: 29581-29588, 29764-29775
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/service/integration_enterprise_test.go` around lines 29458 - 29471, The test currently only asserts counts for specPolicy.LabelsIncludeAll/LabelsIncludeAny/LabelsExcludeAny which can miss wrong or duplicate labels; update the assertions to verify the exact label contents (e.g., compare the expected label names/IDs against specPolicy.LabelsIncludeAll using an unordered comparison like ElementsMatch or by sorting then Equal) and do the same tightening for the other occurrences noted (the batch-success checks around the other ranges); locate checks referencing s.ds.ListGlobalPolicies, the specPolicy variable and the struct fields LabelsIncludeAll/LabelsIncludeAny/LabelsExcludeAny to make these replacements.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/service/integration_enterprise_test.go`:
- Around line 29458-29471: The test currently only asserts counts for
specPolicy.LabelsIncludeAll/LabelsIncludeAny/LabelsExcludeAny which can miss
wrong or duplicate labels; update the assertions to verify the exact label
contents (e.g., compare the expected label names/IDs against
specPolicy.LabelsIncludeAll using an unordered comparison like ElementsMatch or
by sorting then Equal) and do the same tightening for the other occurrences
noted (the batch-success checks around the other ranges); locate checks
referencing s.ds.ListGlobalPolicies, the specPolicy variable and the struct
fields LabelsIncludeAll/LabelsIncludeAny/LabelsExcludeAny to make these
replacements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ba7dad56-2e04-4d14-adf8-27d7677c9439
📥 Commits
Reviewing files that changed from the base of the PR and between 0e9e38a8dc91494dbbe9ea3d0606a8a46e1ddc59 and 6daa27464e4fe6c9ed6812f6aaf34933264c6890.
📒 Files selected for processing (4)
server/datastore/mysql/query_results.goserver/datastore/mysql/query_results_test.goserver/service/integration_core_test.goserver/service/integration_enterprise_test.go
Resolves #41566 Wires labels_include_all to GitOps and fleetctl for policies and reports.
0e8f5ba to
bc98a3f
Compare
2 participants
Related issue: Resolves #41566
Wires labels_include_all to GitOps and fleetctl for policies and reports.
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.
Testing
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Tests