GitOps: combined include/exclude policy label targeting + labels_exclude_all (#33441)#47505
Merged
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #47505 +/- ##
==========================================
- Coverage 67.18% 67.18% -0.01%
==========================================
Files 3547 3547
Lines 228820 228820
Branches 11872 11872
==========================================
- Hits 153728 153726 -2
Misses 61247 61247
- Partials 13845 13847 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| @@ -1056,21 +1056,20 @@ func getLabelUsage(config *spec.GitOps) (map[string][]LabelUsage, error) { | |||
|
|
|||
| // Get policy label usage. | |||
| for _, policy := range config.Policies { | |||
Contributor
There was a problem hiding this comment.
This whole block can be simplified if you use the fleet.VerifyPolicyLabelScopes validator here, no?
Contributor
|
Could we add a pre-flight check in here to make sure non-premium users can't use policy.LabelsExcludeAll ? |
nulmete
force-pushed
the
33441-policies-custom-targets-gitops
branch
from
0c7b9c9 to
877723f
Compare
…ude_all Update fleetctl gitops label-usage validation to allow one include scope (any/all) combined with one exclude scope (any/all) — rejecting two include scopes, two exclude scopes, and overlapping labels (via fleet.LabelOverlap). Emit labels_exclude_all in generate-gitops output (premium-gated, mirroring labels_include_all). Adds validation + emission tests. For #33441.
Exercises the real GitOps apply path: a global policy combining labels_include_any with the premium labels_exclude_all scope, a team policy combining labels_include_all with labels_exclude_any, and rejection of a policy that declares two include scopes.
getLabelUsage now validates policy label scopes via PolicySpec.VerifyLabelScopes instead of a hand-rolled mutex/overlap block, and a non-premium pre-flight check rejects policies using the premium labels_include_all / labels_exclude_all scopes.
Matches PolicySpec.VerifyLabelScopes so both policy types expose label-scope validation under the same descriptive name (PolicySpec.Verify is already taken by full validation).
nulmete
force-pushed
the
33441-policies-custom-targets-gitops
branch
from
877723f to
c4d946a
Compare
Contributor
WalkthroughThis PR adds support for the 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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.go (2)
5082-5082: 💤 Low valueConsider adding dry-run before the real run for consistency.
Most tests in this file perform a dry-run before the real run (e.g., lines 1004-1005, 1141-1142, 1178-1181). Adding a dry-run assertion here would be consistent with the established pattern and provide an extra validation layer.
s.assertDryRunOutput(t, fleetctltest.RunAppForTest(t, []string{"gitops", "--config", fleetctlConfig.Name(), "-f", globalFile, "-f", teamFile, "--dry-run"})) s.assertRealRunOutput(t, fleetctltest.RunAppForTest(t, []string{"gitops", "--config", fleetctlConfig.Name(), "-f", globalFile, "-f", teamFile}))🤖 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 `@cmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.go` at line 5082, Add a dry-run assertion prior to the real run to match the pattern used elsewhere: call s.assertDryRunOutput with the same fleetctltest.RunAppForTest invocation but include the "--dry-run" flag, then keep the existing s.assertRealRunOutput call unchanged; this uses the same helper functions s.assertDryRunOutput, s.assertRealRunOutput and fleetctltest.RunAppForTest so you only need to insert the dry-run invocation before the existing real-run line.
5112-5143: 💤 Low valueConsider adding test coverage for conflicting exclude scopes.
The test currently verifies rejection of two include scopes (lines 5133-5137), but doesn't test the analogous case of two exclude scopes. For completeness, you could add a test case with both
labels_exclude_anyandlabels_exclude_allto verify thatErrPolicyConflictingExcludeLabelsis properly enforced.🤖 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 `@cmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.go` around lines 5112 - 5143, Add a parallel negative test for conflicting exclude scopes: create a new YAML string (e.g., conflictExcludeConfig) modeled after conflictConfig but using labels_exclude_any and labels_exclude_all together, write it to a file (like conflictExcludeFile similar to conflictFile), call fleetctltest.RunAppNoChecks with that file, and assert the run returns an error containing the exclude-conflict message or constant (e.g., require.ErrorContains(t, err, "at most one of labels_exclude_any or labels_exclude_all") or the ErrPolicyConflictingExcludeLabels symbol) using the same patterns (os.WriteFile, require.NoError, RunAppNoChecks, require.ErrorContains) as the existing include-scope test.
🤖 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 `@cmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.go`:
- Line 5082: Add a dry-run assertion prior to the real run to match the pattern
used elsewhere: call s.assertDryRunOutput with the same
fleetctltest.RunAppForTest invocation but include the "--dry-run" flag, then
keep the existing s.assertRealRunOutput call unchanged; this uses the same
helper functions s.assertDryRunOutput, s.assertRealRunOutput and
fleetctltest.RunAppForTest so you only need to insert the dry-run invocation
before the existing real-run line.
- Around line 5112-5143: Add a parallel negative test for conflicting exclude
scopes: create a new YAML string (e.g., conflictExcludeConfig) modeled after
conflictConfig but using labels_exclude_any and labels_exclude_all together,
write it to a file (like conflictExcludeFile similar to conflictFile), call
fleetctltest.RunAppNoChecks with that file, and assert the run returns an error
containing the exclude-conflict message or constant (e.g.,
require.ErrorContains(t, err, "at most one of labels_exclude_any or
labels_exclude_all") or the ErrPolicyConflictingExcludeLabels symbol) using the
same patterns (os.WriteFile, require.NoError, RunAppNoChecks,
require.ErrorContains) as the existing include-scope test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aa6479b1-ec61-4930-a3ec-faa2e1a7f729
📒 Files selected for processing (9)
cmd/fleetctl/fleetctl/generate_gitops.gocmd/fleetctl/fleetctl/generate_gitops_test.gocmd/fleetctl/fleetctl/gitops.gocmd/fleetctl/fleetctl/gitops_test.gocmd/fleetctl/fleetctl/testdata/generateGitops/expectedGlobalPolicies.yamlcmd/fleetctl/fleetctl/testdata/generateGitops/test_dir_premium/default.ymlcmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.goserver/datastore/mysql/policies.goserver/fleet/policies.go
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
juan-fdz-hawa
approved these changes
Jun 15, 2026
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issue: Resolves #46584
Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
(Already added in main.)
Testing
Added/updated automated tests
QA'd all new/changed functionality manually
generate-gitops:Screen.Recording.2026-06-12.at.9.29.29.AM.mov
gitops:Screen.Recording.2026-06-12.at.9.31.58.AM.mov
Summary by CodeRabbit
New Features
labels_exclude_allfield support for refining policy label scopes (available with Fleet Premium license).Bug Fixes