Add aliases for macos fields - #40959
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #40959 +/- ##
==========================================
+ Coverage 66.31% 66.32% +0.01%
==========================================
Files 2469 2470 +1
Lines 197782 197934 +152
Branches 8675 8675
==========================================
+ Hits 131154 131279 +125
- Misses 54761 54771 +10
- Partials 11867 11884 +17
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:
|
| macosSettingsT := reflect.TypeFor[fleet.MacOSSettings]() | ||
| windowsSettingsT := reflect.TypeFor[fleet.WindowsSettings]() | ||
| androidSettingsT := reflect.TypeFor[fleet.AndroidSettings]() | ||
|
|
||
| if cmd.AppConfig.MDM.EnabledAndConfigured && profiles != nil { | ||
| if len(profiles["apple_profiles"].([]map[string]interface{})) > 0 { | ||
| result[jsonFieldName(t, "MacOSSettings")] = map[string]interface{}{ | ||
| "custom_settings": profiles["apple_profiles"], | ||
| jsonFieldName(macosSettingsT, "CustomSettings"): profiles["apple_profiles"], |
There was a problem hiding this comment.
Should have done this awhile ago; this just uses reflection to get the key name from the section rather than hardcoding it. Most of generate-gitops already uses this pattern.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis PR adds alternative JSON/YAML key names for several MDM and GitOps control fields and implements key-rewriting so both old and new names are accepted. It introduces 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 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}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/fleetctl/fleetctl/generate_gitops.go (1)
1273-1277:⚠️ Potential issue | 🟡 MinorUpdate the note text to match the emitted key name.
Line 1273 emits
setup_experience, but Line 1276 still tells users to configuremacos_setup. This is user-facing and likely confusing.💡 Suggested text update
- Note: "The macos_setup configuration is not supported by this tool yet. To configure it, please follow the Fleet documentation at https://fleetdm.com/docs/configuration/yaml-files#macos-setup", + Note: "The setup_experience configuration is not supported by this tool yet. To configure it, please follow the Fleet documentation at https://fleetdm.com/docs/configuration/yaml-files#macos-setup (legacy key: macos_setup).",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/fleetctl/fleetctl/generate_gitops.go` around lines 1273 - 1277, The user-facing note references the wrong key name; when emitting result[jsonFieldName(mdmT, "MacOSSetup")] (which produces the setup_experience key) update the Note added to cmd.Messages.Notes (the Note struct created with Filename: teamName) so its Note string mentions "setup_experience" instead of "macos_setup" and otherwise keeps the same guidance/URL; locate the code that constructs that Note (the append to cmd.Messages.Notes) and change the text accordingly.
🤖 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/generate_gitops_test.go`:
- Around line 1388-1391: The test reuses the prior boolean `ok` when asserting
absence of `setup_experience` for the team-2 case; recompute the presence check
on the fresh `controlsRaw` returned by cmd.generateControls(ptr.Uint(2),
"some_team", nil) before the require.False assertion. Locate the team-2 block in
generate_gitops_test.go, re-evaluate the `ok` (the result of checking for the
"setup_experience" key in `controlsRaw`) from the new `controlsRaw` and then
call require.False(t, ok, ...) to correctly assert that setup_experience is not
present.
In `@pkg/spec/gitops.go`:
- Line 760: processControlsPathIfNeeded currently reassigns the local pointer
variable controlsFilePath instead of writing through it, so the caller's
controlsFilePath is never updated; change the function to set the pointee (e.g.,
*controlsFilePath = ptr.String(...)) after nil-checking controlsFilePath (and
allocate if needed) so the caller receives the new path; update the assignment
in processControlsPathIfNeeded and ensure any early returns still preserve the
intended updated value for the caller.
---
Outside diff comments:
In `@cmd/fleetctl/fleetctl/generate_gitops.go`:
- Around line 1273-1277: The user-facing note references the wrong key name;
when emitting result[jsonFieldName(mdmT, "MacOSSetup")] (which produces the
setup_experience key) update the Note added to cmd.Messages.Notes (the Note
struct created with Filename: teamName) so its Note string mentions
"setup_experience" instead of "macos_setup" and otherwise keeps the same
guidance/URL; locate the code that constructs that Note (the append to
cmd.Messages.Notes) and change the text accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b172fe7d-8ee4-4b76-a8dd-9154cbea53bb
📒 Files selected for processing (14)
changes/40488-add-more-aliasescmd/fleetctl/fleetctl/generate_gitops.gocmd/fleetctl/fleetctl/generate_gitops_test.gocmd/fleetctl/fleetctl/testdata/generateGitops/expectedGlobalControls.yamlcmd/fleetctl/fleetctl/testdata/generateGitops/expectedTeamControls.yamlcmd/fleetctl/fleetctl/testdata/generateGitops/test_dir_free/default.ymlcmd/fleetctl/fleetctl/testdata/generateGitops/test_dir_premium/fleets/team-a-thumbsup.ymlcmd/fleetctl/fleetctl/testdata/generateGitops/test_dir_premium/fleets/unassigned.ymlpkg/spec/gitops.gopkg/spec/gitops_test.gopkg/spec/testdata/controls_new_names.ymlserver/fleet/app.goserver/fleet/hosts.goserver/fleet/teams.go
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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/generate_gitops_test.go`:
- Around line 1391-1392: The assertion message for the check on
controlsRaw["setup_experience"] is misleading for the team-2 scenario; update
the require.False call that currently reads require.False(t, ok, "Expected no
setup_experience section for no-team controls") to a message that references
team 2 (e.g., "Expected no setup_experience section for team-2 controls") so
test failures correctly indicate the tested scenario; locate the require.False
invocation that inspects controlsRaw and change only the failure string to
mention team-2.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 380e505a-e2dd-43c1-b342-53aff2203110
📒 Files selected for processing (2)
cmd/fleetctl/fleetctl/generate_gitops_test.goserver/fleet/teams.go
| windows_enabled_and_configured: true | ||
| windows_migration_enabled: false | ||
| enable_turn_on_windows_mdm_manually: false | ||
| windows_entra_tenant_ids: [] |
There was a problem hiding this comment.
@noahtalerman Doesn't make sense to expand scope in this issue but these seem like they need to be looked at from a consistency perspective.
iansltx
left a comment
iansltx
left a comment
There was a problem hiding this comment.
Doesn't look like we have tests for GitOps that will fail if you provide both old and new names. Mind adding those?
No feedback otherwise here.
Added missing unit tests for |
iansltx
left a comment
iansltx
left a comment
There was a problem hiding this comment.
Added a little more oomph to the tests to make sure we're marking the old fields as deprecated (we are) so we're good to go here.
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #40496 # Details This is the first phase of an effort to detect unknown keys in GitOps .yml files. In the regular `fleetctl gitops` case, it will fail when unknown keys are detected. This behavior can be changed with a new `--allow-unknown-keys` flag which will log the issues and continue. In this first phase we are detecting unknown keys in _most_ GitOps sections, other than the top-level `org_settings:` and `settings:` sections which have more complicated typing. I will tackle those separately as they require a bit more thought. Also ultimately I'd like us to be doing this validation in a more top-down fashion in one place, rather than spreading it across the code by doing it in each individual section, but this is a good first step. As a bonus, I invited my pal Mr. Levenshtein to the party so that we can make suggestions when unknown keys are detected, like: ``` * unknown key "queyr" in "./lib/some-report.yml"; did you mean "query"? ``` > Note: the goal is to return as many validation errors as possible to the user, so they don't have to keep running `fleetctl gitops` to get the next error. I did _not_ update any other errors to stop returning early, in an effort to keep this as low-touch as possible. # Checklist for submitter If some of the following don't apply, delete the relevant line. - [X] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. ## Testing - [X] Added/updated automated tests - [X] QA'd all new/changed functionality manually - [X] Tested this against existing it-and-security folder and one with updated keys from #40959; no unknown keys detected - [X] Added unknown keys at various levels, GitOps errored with helpful messages - [X] Same as above but with `--allow-unknown-keys`; GitOps outputted helpful messages but continued. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * GitOps runs now fail when unknown or misspelled keys are present in configuration files. * New CLI flag --allow-unknown-keys lets unknown keys be treated as warnings instead of errors. * Unknown-key messages include suggested valid key names to help correct mistakes. * **Tests** * Expanded test coverage to validate unknown-key detection and the allow-as-warning option. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Ian Littman <iansltx@gmail.com>
2 participants
Related issue: Resolves #40488
Details
Implements the renames requested in #40488:
macos_setup:setup_experiencemacos_settings:apple_settingscustom_settings:configuration_profilesmacos_setup_assistant:apple_setup_assistantPrior names are deprecated and log warnings. This uses the same
renametotags as previous aliases, and adds code in relevant sections in gitops.go to run the existing "rename new to old keys" function so that we can unmarshall into the existing structs (that still have theirjsontags set to the old key names until Fleet 5).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
If you didn't check the box above, follow this checklist for GitOps-enabled settings:
fleetctl generate-gitopsn/a
Summary by CodeRabbit