Partial revert of #38785 work-in-progress#44061
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #44061 +/- ##
=======================================
Coverage 66.83% 66.83%
=======================================
Files 2609 2609
Lines 210477 210496 +19
Branches 9268 9268
=======================================
+ Hits 140668 140682 +14
- Misses 56992 56996 +4
- Partials 12817 12818 +1
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:
|
![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.
WalkthroughThis pull request removes Windows-specific setup experience cancellation functionality and restricts the "cancel pending steps" behavior to macOS only. The changes eliminate the Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/service/setup_experience.go (1)
240-257: Simplified logic looks correct; consider small readability tweak.The revert correctly drops the
host.Platform/RequireAllSoftwareWindowsbranching and now resolvesRequireAllSoftwarefrom either the app config (no team) or the team config. Logic is fine, but the localrequireAllSoftwarevariable can be returned directly from each branch to avoid the extra assignment-then-return pattern.♻️ Optional simplification
func isAllSetupExperienceSoftwareRequired(ctx context.Context, ds fleet.Datastore, host *fleet.Host) (bool, error) { teamID := host.TeamID - requireAllSoftware := false if teamID == nil || *teamID == 0 { ac, err := ds.AppConfig(ctx) if err != nil { return false, ctxerr.Wrap(ctx, err, "getting app config") } - requireAllSoftware = ac.MDM.MacOSSetup.RequireAllSoftware - } else { - team, err := ds.TeamLite(ctx, *teamID) - if err != nil { - return false, ctxerr.Wrap(ctx, err, "load team") - } - requireAllSoftware = team.Config.MDM.MacOSSetup.RequireAllSoftware + return ac.MDM.MacOSSetup.RequireAllSoftware, nil } - return requireAllSoftware, nil + team, err := ds.TeamLite(ctx, *teamID) + if err != nil { + return false, ctxerr.Wrap(ctx, err, "load team") + } + return team.Config.MDM.MacOSSetup.RequireAllSoftware, nil }Also note: since
hostis no longer dereferenced here, the signature could in principle drop the unused fields, but keeping*fleet.Hostaligns with the caller andIsAllSetupExperienceSoftwareRequiredwrapper, so leaving it is fine.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/setup_experience.go` around lines 240 - 257, The function isAllSetupExperienceSoftwareRequired uses a local requireAllSoftware variable unnecessarily; simplify by returning the resolved value directly from each branch to improve readability: inside the nil/zero teamID branch, after fetching ac := ds.AppConfig(ctx), return ac.MDM.MacOSSetup.RequireAllSoftware (wrapping errors with ctxerr.Wrap as currently done), and in the else branch after loading team via ds.TeamLite(ctx, *teamID), return team.Config.MDM.MacOSSetup.RequireAllSoftware (again preserving error wrapping). Keep the same error handling and function signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/service/setup_experience.go`:
- Around line 240-257: The function isAllSetupExperienceSoftwareRequired uses a
local requireAllSoftware variable unnecessarily; simplify by returning the
resolved value directly from each branch to improve readability: inside the
nil/zero teamID branch, after fetching ac := ds.AppConfig(ctx), return
ac.MDM.MacOSSetup.RequireAllSoftware (wrapping errors with ctxerr.Wrap as
currently done), and in the else branch after loading team via ds.TeamLite(ctx,
*teamID), return team.Config.MDM.MacOSSetup.RequireAllSoftware (again preserving
error wrapping). Keep the same error handling and function signature.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9b37fafb-82ca-41cd-a5c6-10740cf5b919
📒 Files selected for processing (3)
changes/38785-windows-setup-experience-cancelserver/service/setup_experience.goserver/service/setup_experience_test.go
💤 Files with no reviewable changes (2)
- changes/38785-windows-setup-experience-cancel
- server/service/setup_experience_test.go
There was a problem hiding this comment.
Pull request overview
This PR partially reverts work related to Windows “cancel setup if software fails” by removing Windows-specific handling in the setup experience service logic and dropping the accompanying changelog entry.
Changes:
- Remove Windows platform branching for determining “require all software” in setup experience configuration lookup.
- Restrict setup-experience step cancellation to macOS hosts only.
- Remove the
changes/entry that documented the Windows setting/behavior.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| server/service/setup_experience.go | Removes Windows-specific “require all software” lookup and limits cancellation logic to macOS. |
| server/service/setup_experience_test.go | Deletes unit test coverage that verified platform-specific config behavior. |
| changes/38785-windows-setup-experience-cancel | Removes the changelog entry describing the Windows setting/behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ksykulev
left a comment
ksykulev
left a comment
There was a problem hiding this comment.
As far as I can see no docs or api that reference it and nothing in the front end. looks good.
This reverts commit 5b82531. <!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #38785 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **New Features** * Windows setup experience now supports requiring all software installations: enrollment can be configured to cancel if any required software fails to install. * **Tests** * Added test coverage for platform-specific setup software requirements. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Related issue: Resolves #38785
This feature has been pushed to 4.86
Summary by CodeRabbit
Release Notes