Add require_all_software_windows config option - #43011
Merged
Merged
Conversation
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new require_all_software_windows configuration option so Windows setup experience can be canceled when a software install fails (paralleling the existing macOS behavior).
Changes:
- Extend setup experience “require all software” logic to apply to Windows hosts.
- Add
require_all_software_windowsto config/payload structs and wire it through API + GitOps/team spec application paths. - Update fleetctl apply/get expected outputs and add targeted tests.
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/setup_experience.go | Extends require-all-software checks and cancellation gating to Windows. |
| server/service/setup_experience_test.go | Adds unit tests covering macOS/Windows/team/global behavior for the new flag. |
| server/fleet/apple_mdm.go | Extends setup-experience PATCH payload with require_all_software_windows. |
| server/fleet/app.go | Adds RequireAllSoftwareWindows to persisted setup-experience config struct. |
| ee/server/service/teams.go | Applies/validates require_all_software_windows during GitOps/team spec edits and team MDM setup updates. |
| ee/server/service/mdm.go | Supports updating app config via API for require_all_software_windows and validates Windows MDM configuration. |
| cmd/fleetctl/fleetctl/testdata/macosSetupExpectedTeam1Set.yml | Updates expected YAML to include require_all_software_windows. |
| cmd/fleetctl/fleetctl/testdata/macosSetupExpectedTeam1Empty.yml | Updates expected YAML to include require_all_software_windows. |
| cmd/fleetctl/fleetctl/testdata/macosSetupExpectedTeam1And2Set.yml | Updates expected YAML to include require_all_software_windows. |
| cmd/fleetctl/fleetctl/testdata/macosSetupExpectedTeam1And2Empty.yml | Updates expected YAML to include require_all_software_windows. |
| cmd/fleetctl/fleetctl/testdata/macosSetupExpectedAppConfigSet.yml | Updates expected YAML to include require_all_software_windows. |
| cmd/fleetctl/fleetctl/testdata/macosSetupExpectedAppConfigEmpty.yml | Updates expected YAML to include require_all_software_windows. |
| cmd/fleetctl/fleetctl/testdata/expectedGetTeamsYaml.yml | Updates expected teams YAML output with the new field. |
| cmd/fleetctl/fleetctl/testdata/expectedGetTeamsJson.json | Updates expected teams JSON output with the new field. |
| cmd/fleetctl/fleetctl/testdata/expectedGetConfigIncludeServerConfigYaml.yml | Updates expected config YAML output with the new field. |
| cmd/fleetctl/fleetctl/testdata/expectedGetConfigIncludeServerConfigJson.json | Updates expected config JSON output with the new field. |
| cmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigYaml.yml | Updates expected app config YAML output with the new field. |
| cmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigTeamMaintainerYaml.yml | Updates expected team-maintainer YAML output with the new field. |
| cmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigTeamMaintainerJson.json | Updates expected team-maintainer JSON output with the new field. |
| cmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigJson.json | Updates expected app config JSON output with the new field. |
| cmd/fleetctl/fleetctl/apply_test.go | Adds a fleetctl apply roundtrip test covering require_all_software_windows. |
| changes/38785-windows-setup-experience-cancel | Adds a user-visible changelog entry for the new setting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
✅ Actions performedFull review triggered. |
Contributor
WalkthroughThis PR adds a new 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.
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 `@cmd/fleetctl/fleetctl/apply_test.go`:
- Around line 2717-2738: The test currently uses strings.ReplaceAll on
expectedTm1 which flips both fleet- and team-level require_all_software_windows
values; change the test to only update the team occurrence (or add an explicit
assertion) so fleet-level remains false: locate the expectedTm1 string used for
comparison and either perform a targeted replacement that matches the team
section (e.g., replace the block under "team:" or the specific line following
that header) or after RunAppForTest(... "get", "teams", "--yaml") assert that
the fleet section still contains `require_all_software_windows: false`; keep the
existing checks for ds.SaveTeamFuncInvoked and the applied output assertions
(RunAppForTest, windowsRequireSpec, expectedWithWindowsRequire) but ensure only
the team value is flipped in expectedWithWindowsRequire or add a separate
assertion verifying fleet-level remains false.
In `@ee/server/service/teams.go`:
- Around line 1596-1607: The code validates RequireAllSoftwareWindows only on
edits but not on team creation, allowing creation of a team with
spec.MDM.MacOSSetup.RequireAllSoftwareWindows=true even when
appCfg.MDM.WindowsEnabledAndConfigured is false; update the team-creation path
where team.Config.MDM.MacOSSetup is copied/persisted (the assignment of
team.Config.MDM.MacOSSetup.RequireAllSoftwareWindows from
spec.MDM.MacOSSetup.RequireAllSoftwareWindows) to perform the same guard:
determine windowsEnabledAndConfigured (respecting
opts.DryRunAssumptions.WindowsEnabledAndConfigured if present) and if
spec.MDM.MacOSSetup.RequireAllSoftwareWindows is true while
windowsEnabledAndConfigured is false, return the same
fleet.NewInvalidArgumentError("setup_experience.require_all_software_windows",
...) wrapped with ctxerr.Wrap, before assigning/persisting the value.
In `@server/service/setup_experience_test.go`:
- Around line 228-243: The test currently sets both macOS and Windows flags to
the same values in appCfg and the TeamLite returned by ds.TeamLiteFunc so it
doesn't exercise branching; update the fixtures so the app-level and team-level
values differ and also make RequireAllSoftware and RequireAllSoftwareWindows
different from each other (e.g. appCfg.MDM.MacOSSetup.RequireAllSoftware=true
but appCfg.MDM.MacOSSetup.RequireAllSoftwareWindows=false, and the
TeamLite.TeamConfigLite.MDM.MacOSSetup flags set to the opposite) so the code
paths in the functions that read appCfg vs TeamLite and macOS vs Windows flags
(look for appCfg, ds.AppConfigFunc, ds.TeamLiteFunc, fleet.AppConfig,
fleet.TeamLite, RequireAllSoftware, RequireAllSoftwareWindows, MacOSSetup,
TeamMDM) are actually exercised; apply the same pattern for the other affected
block (lines ~250-293).
🪄 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: 0ca9bb85-0237-4606-a16c-d29e7ac3f57b
📒 Files selected for processing (22)
changes/38785-windows-setup-experience-cancelcmd/fleetctl/fleetctl/apply_test.gocmd/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/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.ymlee/server/service/mdm.goee/server/service/teams.goserver/fleet/app.goserver/fleet/apple_mdm.goserver/service/setup_experience.goserver/service/setup_experience_test.go
…win-software # Conflicts: # server/datastore/mysql/schema.sql
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #43011 +/- ##
==========================================
- Coverage 66.85% 66.81% -0.04%
==========================================
Files 2578 2578
Lines 206880 206900 +20
Branches 9166 9166
==========================================
- Hits 138301 138247 -54
- Misses 56003 56102 +99
+ Partials 12576 12551 -25
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:
|
getvictor
marked this pull request as ready for review
April 6, 2026 12:39
![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.
ksykulev
approved these changes
Apr 6, 2026
This was referenced Apr 23, 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
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 #42853
This PR simply adds the
require_all_software_windowsconfig option. It doesn't use it. The logic to use it will be hooked up in subsequent PRs.The fleetctl TestIntegrationsPreview test is expected to fail since it builds the server against main and doesn't know about our new config option.
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.Testing
New Fleet configuration settings
fleetctl generate-gitopsSummary by CodeRabbit
New Features
Tests