Make most GitOps top-level optional - #41138
Merged
Merged
Conversation
sgress454
marked this pull request as draft
March 6, 2026 17:14
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #41138 +/- ##
==========================================
+ Coverage 66.31% 66.34% +0.03%
==========================================
Files 2469 2471 +2
Lines 197782 198135 +353
Branches 8781 8781
==========================================
+ Hits 131154 131450 +296
- Misses 54761 54785 +24
- Partials 11867 11900 +33 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
WalkthroughThis PR introduces a feature allowing most top-level keys in GitOps YAML files to be omitted instead of requiring empty values. The changes modify the GitOps parsing logic to assign null to missing top-level keys, effectively clearing them when absent. The Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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.
Actionable comments posted: 1
🧹 Nitpick comments (4)
changes/41012-allow-omitting-top-level-gitops-keys (1)
1-1: Clarify that this is parser behavior, not generator output.This reads like
fleetctl generate-gitopsmay also start omitting empty keys. I’d make it explicit that the parser now accepts omitted top-level keys, while the generator still emits empty fields for discoverability.Based on learnings: The generate-gitops command in fleetctl should always emit all fields, even when they are empty, so that users can see what configuration options are available for discovery purposes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@changes/41012-allow-omitting-top-level-gitops-keys` at line 1, Update the release note sentence to explicitly state this is parser behavior: clarify that the input parser (not the generator) now accepts omitted top-level GitOps keys and that the fleetctl generate-gitops command continues to emit all top-level keys (even when empty) for discoverability; reference the parser behavior and the generate-gitops command by name to avoid confusion.pkg/spec/gitops_test.go (1)
908-935: Assert the normalized state, not just parse success.These cases only prove omission is accepted. They won't fail if an omitted key stops clearing state but still parses. Please assert the post-parse shape for at least one global and one team case (
AgentOptions,Policies,Queries,TeamSettings, etc.), and consider addingsoftwareto the matrix sinceGitOpsFromFilenormalizes it too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/spec/gitops_test.go` around lines 908 - 935, The tests currently only check that parsing succeeds for omitted keys; update the cases to assert the normalized post-parse state for one global case (e.g., the "missing_all_global" or "missing_reports" entry) and one team case (e.g., "missing_settings" or "missing_name") by calling GitOpsFromFile and then asserting that the returned struct has normalized empty/default values for AgentOptions, Policies, Queries and TeamSettings (and include Software since GitOpsFromFile normalizes it) rather than merely checking parse success; locate the parsing call to GitOpsFromFile and add assertions that specific fields (AgentOptions, Policies, Queries, TeamSettings, Software) are empty/nil/default as appropriate for the chosen global and team test entries.cmd/fleetctl/fleetctl/gitops_test.go (2)
231-233: Assert the omitted-controlsresult before resettingsavedAppConfig.Right now this branch only proves the command returns
nil; Line 233 then discards the captured state. If omittingcontrolsstarted applying a different config thancontrols:, this test would still pass.Example assertion to keep this branch meaningful
_, err = RunAppNoChecks([]string{"gitops", "-f", tmpFile2.Name()}) require.NoError(t, err) + assert.Equal(t, "Foobar", savedAppConfig.OrgInfo.OrgName) + assert.Equal(t, "https://example.com", savedAppConfig.ServerSettings.ServerURL) + assert.Empty(t, enrolledSecrets) savedAppConfig = &fleet.AppConfig{}🤖 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 231 - 233, Test currently only checks RunAppNoChecks returns nil and then immediately resets savedAppConfig; preserve and assert the captured config before clearing it. After calling RunAppNoChecks([]string{"gitops", "-f", tmpFile2.Name()}) and before assigning savedAppConfig = &fleet.AppConfig{}, add an assertion that savedAppConfig (the value populated by the test harness) matches the expected structure when controls is omitted—e.g., assert required fields and that Controls is nil or empty as appropriate—so the branch verifies the actual config change rather than just a nil error from RunAppNoChecks.
2265-2285: Add observable assertions to this new optional-controlssubtest.Both runs only check that
gitopsdoesn't fail. That makes the test too weak for the PR goal that omitted top-level keys behave the same as explicit empty values.Mirror the neighboring success assertions here
// Dry run, controls is now optional so this should succeed. _ = RunAppForTest(t, []string{ "gitops", "-f", globalFileWithoutControlsAndSoftwareKeys.Name(), "-f", teamFileBasic.Name(), "-f", noTeamFileWithoutControls.Name(), "--dry-run", }) + assert.Equal(t, fleet.AppConfig{}, *savedAppConfig, "AppConfig should be empty") // Real run _ = RunAppForTest(t, []string{ "gitops", "-f", globalFileWithoutControlsAndSoftwareKeys.Name(), "-f", teamFileBasic.Name(), "-f", noTeamFileWithoutControls.Name(), }) + assert.Equal(t, orgName, savedAppConfig.OrgInfo.OrgName) + assert.Equal(t, fleetServerURL, savedAppConfig.ServerSettings.ServerURL) + assert.Len(t, enrolledSecrets, 1) + require.NotNil(t, savedTeam) + assert.Equal(t, teamName, savedTeam.Name) + require.Len(t, enrolledTeamSecrets, 1) + assert.Equal(t, secret, enrolledTeamSecrets[0].Secret)🤖 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 2265 - 2285, The subtest currently only verifies gitops commands don't error by discarding RunAppForTest results; capture the outputs from both the dry-run and real RunAppForTest calls (instead of assigning to _), and add the same observable assertions used in the neighboring successful subtest: assert the output contains the expected success markers/messages and/or exit status that indicate the operation succeeded (use the same require.Contains/require.NoError checks as the adjacent test). Target the RunAppForTest calls that pass globalFileWithoutControlsAndSoftwareKeys.Name(), teamFileBasic.Name(), and noTeamFileWithoutControls.Name() and mirror the exact assertions from the neighboring success test so optional `controls` behavior is validated.
🤖 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/integrationtest/gitops/gitops_enterprise_integration_test.go`:
- Around line 3865-3886: The test's fullTeamConfig (constructed in
fullTeamConfig using teamName) leaves top-level keys like "software:" empty
while assertions only check settings.features, so the test can miss regressions
that preserve enroll secrets or installers when those keys are omitted; either
populate/seed the omitted sections (e.g., add a non-empty software: block and
any installer/enroller keys you expect) in the Step 1 config and assert their
presence after Step 2, or narrow the test comments and assertions to only
validate settings.features (remove claims about software/enrollers/secrets).
Apply the same fix to the similar block around lines referenced 3917-3952.
---
Nitpick comments:
In `@changes/41012-allow-omitting-top-level-gitops-keys`:
- Line 1: Update the release note sentence to explicitly state this is parser
behavior: clarify that the input parser (not the generator) now accepts omitted
top-level GitOps keys and that the fleetctl generate-gitops command continues to
emit all top-level keys (even when empty) for discoverability; reference the
parser behavior and the generate-gitops command by name to avoid confusion.
In `@cmd/fleetctl/fleetctl/gitops_test.go`:
- Around line 231-233: Test currently only checks RunAppNoChecks returns nil and
then immediately resets savedAppConfig; preserve and assert the captured config
before clearing it. After calling RunAppNoChecks([]string{"gitops", "-f",
tmpFile2.Name()}) and before assigning savedAppConfig = &fleet.AppConfig{}, add
an assertion that savedAppConfig (the value populated by the test harness)
matches the expected structure when controls is omitted—e.g., assert required
fields and that Controls is nil or empty as appropriate—so the branch verifies
the actual config change rather than just a nil error from RunAppNoChecks.
- Around line 2265-2285: The subtest currently only verifies gitops commands
don't error by discarding RunAppForTest results; capture the outputs from both
the dry-run and real RunAppForTest calls (instead of assigning to _), and add
the same observable assertions used in the neighboring successful subtest:
assert the output contains the expected success markers/messages and/or exit
status that indicate the operation succeeded (use the same
require.Contains/require.NoError checks as the adjacent test). Target the
RunAppForTest calls that pass globalFileWithoutControlsAndSoftwareKeys.Name(),
teamFileBasic.Name(), and noTeamFileWithoutControls.Name() and mirror the exact
assertions from the neighboring success test so optional `controls` behavior is
validated.
In `@pkg/spec/gitops_test.go`:
- Around line 908-935: The tests currently only check that parsing succeeds for
omitted keys; update the cases to assert the normalized post-parse state for one
global case (e.g., the "missing_all_global" or "missing_reports" entry) and one
team case (e.g., "missing_settings" or "missing_name") by calling GitOpsFromFile
and then asserting that the returned struct has normalized empty/default values
for AgentOptions, Policies, Queries and TeamSettings (and include Software since
GitOpsFromFile normalizes it) rather than merely checking parse success; locate
the parsing call to GitOpsFromFile and add assertions that specific fields
(AgentOptions, Policies, Queries, TeamSettings, Software) are empty/nil/default
as appropriate for the chosen global and team test entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e0c95626-839a-456a-82e6-0301036c3477
📒 Files selected for processing (5)
changes/41012-allow-omitting-top-level-gitops-keyscmd/fleetctl/fleetctl/gitops_test.gocmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.gopkg/spec/gitops.gopkg/spec/gitops_test.go
iansltx
reviewed
Mar 6, 2026
iansltx
previously approved these changes
Mar 6, 2026
iansltx
approved these changes
Mar 9, 2026
37 tasks
33 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 #41012
Details
This PR makes it allowable to leave out almost all top-level keys from GitOps files. The only required keys are either
name:(for a fleet settings file) ororg_settings:(for a global settings file). Omitting a key is identical to supplying it with no value.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
Updated the "missing all global keys test", and added some new tests to verify that omitting the key was the same as supplying it with an empty value.
fleetctl generate-gitopsto get a clean set of GitOps yml filesfleetctl gitopsfleetctl generate-gitopsagain into a different dirfleetctl gitopswith the original files to get back to original statereports:with nothing under it)fleetctl generate-gitopsagain into a third dirSummary by CodeRabbit
Release Notes
New Features
Tests