Stop setup experience on software install fail: admin#33968
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #33968 +/- ##
==========================================
- Coverage 64.19% 64.19% -0.01%
==========================================
Files 2055 2057 +2
Lines 206470 206650 +180
Branches 6853 6767 -86
==========================================
+ Hits 132544 132659 +115
- Misses 63526 63578 +52
- Partials 10400 10413 +13
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:
|
sgress454
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughAdds a new macOS setup option require_all_software_macos across backend, API payloads, datastore seed, device global config, CLI test fixtures, and frontend (types, UI, service). Frontend introduces an advanced option with save flow to PATCH MDM setup, and backend/team handlers persist and expose the flag to devices. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Admin as Admin (UI)
participant FE as Frontend (AddInstallSoftware)
participant API as PATCH /mdm/setup
participant SVC as Service (Teams/AppConfig)
participant DS as Datastore
Admin->>FE: Open Install Software (macOS)
FE->>FE: Reveal "Advanced options"
Admin->>FE: Toggle "Cancel setup if software install fails"
Admin->>FE: Click Save
FE->>API: PATCH { team_id?, require_all_software_macos }
API->>SVC: updateTeamMDMAppleSetup / updateAppConfigMDMAppleSetup
SVC->>DS: Persist RequireAllSoftware
DS-->>SVC: OK
SVC-->>API: 200
API-->>FE: Success
FE-->>Admin: Success message
sequenceDiagram
autonumber
participant Agent as Agent
participant DEV as GET /device/global_config
participant SVC as Devices Service
participant DS as Datastore
Agent->>DEV: Request global config
DEV->>SVC: Resolve config
SVC->>DS: Load AppConfig + Team config (if host team)
DS-->>SVC: Configs
SVC->>SVC: Determine requireAllSoftware (team overrides global)
SVC-->>Agent: mdm { enabled_and_configured, require_all_software_macos }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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: 2
🧹 Nitpick comments (3)
frontend/interfaces/config.ts (1)
80-80: Consider type consistency with backend.The backend field is a plain
boolin Go (line 471 inserver/fleet/app.go), which means the API will never returnnullfor this field. The TypeScript typeboolean | nullis overly permissive, though this pattern matches other fields inmacos_setup. For strict type safety, consider using justboolean.- require_all_software_macos: boolean | null; + require_all_software_macos: boolean;Note: This is a minor type-safety refinement and follows the pattern used for
enable_end_user_authentication, which also corresponds to a plainboolin Go.frontend/pages/ManageControlsPage/SetupExperience/cards/InstallSoftware/components/AddInstallSoftware/AddInstallSoftware.tsx (2)
53-55: Consider using nullish coalescing for clearer intent.The current logic
savedRequireAllSoftwareMacOS || falsecorrectly defaults tofalsefornull,undefined, andfalsevalues. However, using the nullish coalescing operator would make the intent more explicit.Apply this diff for clarity:
- const [requireAllSoftwareMacOS, setRequireAllSoftwareMacOS] = useState( - savedRequireAllSoftwareMacOS || false - ); + const [requireAllSoftwareMacOS, setRequireAllSoftwareMacOS] = useState( + savedRequireAllSoftwareMacOS ?? false + );Note: The state won't sync if the
savedRequireAllSoftwareMacOSprop changes after mount. Verify this is the intended behavior for your use case.
58-72: Consider capturing error details for debugging.The error handler displays a generic message to the user, which is appropriate for UX. However, consider logging the error for debugging purposes.
Apply this diff to add error logging:
try { await mdmAPI.updateRequireAllSoftwareMacOS( currentTeamId, requireAllSoftwareMacOS ); renderFlash("success", "Successfully updated!"); - } catch { + } catch (error) { + console.error("Failed to update require all software setting:", error); renderFlash("error", "Couldn't update. Please try again."); } finally {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
changes/31110-add-require-all-software-flag(1 hunks)cmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigJson.json(2 hunks)cmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigTeamMaintainerJson.json(2 hunks)cmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigTeamMaintainerYaml.yml(1 hunks)cmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigYaml.yml(1 hunks)cmd/fleetctl/fleetctl/testdata/expectedGetConfigIncludeServerConfigJson.json(2 hunks)cmd/fleetctl/fleetctl/testdata/expectedGetConfigIncludeServerConfigYaml.yml(1 hunks)cmd/fleetctl/fleetctl/testdata/expectedGetTeamsJson.json(2 hunks)cmd/fleetctl/fleetctl/testdata/expectedGetTeamsYaml.yml(2 hunks)cmd/fleetctl/fleetctl/testdata/macosSetupExpectedAppConfigEmpty.yml(1 hunks)cmd/fleetctl/fleetctl/testdata/macosSetupExpectedAppConfigSet.yml(1 hunks)cmd/fleetctl/fleetctl/testdata/macosSetupExpectedTeam1And2Empty.yml(1 hunks)cmd/fleetctl/fleetctl/testdata/macosSetupExpectedTeam1And2Set.yml(1 hunks)cmd/fleetctl/fleetctl/testdata/macosSetupExpectedTeam1Empty.yml(1 hunks)cmd/fleetctl/fleetctl/testdata/macosSetupExpectedTeam1Set.yml(1 hunks)ee/server/service/mdm.go(1 hunks)ee/server/service/teams.go(2 hunks)frontend/__mocks__/configMock.ts(1 hunks)frontend/interfaces/config.ts(2 hunks)frontend/interfaces/team.ts(1 hunks)frontend/pages/ManageControlsPage/SetupExperience/cards/InstallSoftware/InstallSoftware.tsx(1 hunks)frontend/pages/ManageControlsPage/SetupExperience/cards/InstallSoftware/components/AddInstallSoftware/AddInstallSoftware.tests.tsx(5 hunks)frontend/pages/ManageControlsPage/SetupExperience/cards/InstallSoftware/components/AddInstallSoftware/AddInstallSoftware.tsx(5 hunks)frontend/pages/hosts/details/DeviceUserPage/DeviceUserPage.tests.tsx(4 hunks)frontend/services/entities/mdm.ts(1 hunks)server/datastore/mysql/schema.sql(1 hunks)server/fleet/app.go(2 hunks)server/fleet/apple_mdm.go(1 hunks)server/service/devices.go(3 hunks)tools/cloner-check/generated_files/appconfig.txt(1 hunks)tools/cloner-check/generated_files/teamconfig.txt(1 hunks)tools/cloner-check/generated_files/teammdm.txt(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied—especially when a query is intended to return data for a specific entity (e.g., a single host). Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results (e.g., returning the first row instead of the correct one). Flag any queries that may return unintended results due to lack of precise scoping.
Files:
server/fleet/apple_mdm.goserver/fleet/app.goee/server/service/teams.goserver/service/devices.goee/server/service/mdm.go
🔇 Additional comments (30)
cmd/fleetctl/fleetctl/testdata/macosSetupExpectedAppConfigEmpty.yml (1)
48-48: LGTM!The new
require_all_software_macos: falsefield is correctly added to the global macOS setup configuration with an appropriate default value.cmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigTeamMaintainerYaml.yml (1)
57-57: LGTM!The field is correctly added to the team maintainer configuration with the appropriate default value.
changes/31110-add-require-all-software-flag (1)
1-1: LGTM!The changes file clearly describes the new feature being added.
cmd/fleetctl/fleetctl/testdata/expectedGetTeamsYaml.yml (1)
42-42: LGTM!Both teams consistently include the new
require_all_software_macos: falsefield in their macOS setup configurations.Also applies to: 98-98
cmd/fleetctl/fleetctl/testdata/expectedGetConfigIncludeServerConfigYaml.yml (1)
57-57: LGTM!The field is correctly added to the configuration that includes server settings.
cmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigTeamMaintainerJson.json (1)
112-113: LGTM!The new field is correctly added in JSON format with proper comma placement.
cmd/fleetctl/fleetctl/testdata/expectedGetConfigAppConfigYaml.yml (1)
57-57: LGTM!The field is correctly added to the standard application configuration YAML.
cmd/fleetctl/fleetctl/testdata/macosSetupExpectedTeam1And2Empty.yml (1)
29-29: Intentional default fallback The absence ofrequire_all_software_macosin the second team tests the default behavior—no changes required.Likely an incorrect or invalid review comment.
server/fleet/apple_mdm.go (1)
513-513: LGTM! Field addition follows established patterns.The new
RequireAllSoftwarefield is correctly defined as an optional pointer type, consistent with other fields in theMDMAppleSetupPayloadstruct. The JSON tagrequire_all_software_macosclearly indicates this is a macOS-specific setting.cmd/fleetctl/fleetctl/testdata/expectedGetTeamsJson.json (1)
62-63: LGTM! Consistent field addition across teams.The new
require_all_software_macosfield is added consistently to both team specifications with the default value offalse.Also applies to: 154-155
ee/server/service/mdm.go (1)
214-219: LGTM! Implementation follows established patterns.The handling of
payload.RequireAllSoftwareis correctly implemented, following the same pattern as other optional boolean fields in this function. The logic properly checks for nil, compares values, and sets thedidUpdateflag to trigger configuration persistence.tools/cloner-check/generated_files/appconfig.txt (1)
152-152: LGTM! Generated documentation is correct.The field is correctly documented as a
booltype in theMacOSSetupstruct, which is appropriate for the stored configuration (as opposed to the optional*boolused in the payload struct).cmd/fleetctl/fleetctl/testdata/macosSetupExpectedTeam1And2Set.yml (1)
30-30: Verify intentional asymmetry between teams
A grep across all testdata showsrequire_all_software_macosappears only under teamtm1(e.g., macosSetupExpectedTeam1And2Set.yml:30). If omitting it fortm2is deliberate, add a clarifying comment; otherwise, include the field fortm2as well.cmd/fleetctl/fleetctl/testdata/expectedGetConfigIncludeServerConfigJson.json (1)
91-92: LGTM!The new
require_all_software_macosfield is correctly added to the test fixture with a safe default value offalse.cmd/fleetctl/fleetctl/testdata/macosSetupExpectedAppConfigSet.yml (1)
48-48: LGTM!The field addition is consistent with the JSON test fixtures and uses the appropriate YAML formatting.
frontend/pages/ManageControlsPage/SetupExperience/cards/InstallSoftware/InstallSoftware.tsx (1)
181-185: LGTM!The new prop correctly follows the established pattern for team/global config selection and uses safe optional chaining to prevent runtime errors.
server/fleet/app.go (2)
471-471: LGTM!The field addition is correct and follows the pattern of
EnableEndUserAuthentication(line 465) for a mandatory boolean setting with a false default.
1508-1508: LGTM!The field correctly mirrors the
MacOSSetup.RequireAllSoftwarefield and will properly propagate to device endpoints.frontend/services/entities/mdm.ts (1)
278-284: LGTM!The new API method follows the established pattern and correctly structures the request payload for updating the macOS software requirement setting.
server/service/devices.go (1)
174-174: LGTM!The implementation correctly derives the
requireAllSoftwareflag from global config with proper team override, following the established pattern forsoftwareInventoryEnabled.Also applies to: 183-183, 201-201
ee/server/service/teams.go (2)
1380-1387: LGTM!The validation logic correctly ensures MDM features are enabled before allowing the software requirement flag to be turned on, with a clear and helpful error message.
1786-1791: LGTM!The payload handling correctly updates the team configuration only when the field is provided and differs from the current value, following the established pattern.
frontend/interfaces/config.ts (1)
93-96: LGTM!The interface change correctly reflects the backend structure where
DeviceGlobalMDMConfigincludes both fields in a flatmdmobject (seeserver/fleet/app.golines 1506-1509).frontend/pages/ManageControlsPage/SetupExperience/cards/InstallSoftware/components/AddInstallSoftware/AddInstallSoftware.tests.tsx (2)
2-2: LGTM!The
waitForimport is correctly added for the new async test case.
16-16: LGTM!The new required prop is correctly added to maintain test validity.
frontend/pages/ManageControlsPage/SetupExperience/cards/InstallSoftware/components/AddInstallSoftware/AddInstallSoftware.tsx (5)
1-1: LGTM!The React hooks are correctly imported for the new state management.
10-12: LGTM!Context and API imports are correctly added for the new functionality.
35-35: LGTM!The prop type correctly handles the nullable boolean from the config.
155-189: LGTM!The macOS advanced options UI is well-structured with:
- Proper conditional rendering for macOS platform
- Consistent GitOps mode handling (both checkbox and button disabled)
- Appropriate loading state management
- Clear tooltip explaining the feature behavior
166-174: Ignorecheckedprop suggestion
TheCheckboxcomponent’s API uses avalueprop (boolean | null) rather thanchecked, so no change is needed.Likely an incorrect or invalid review comment.
iansltx
left a comment
iansltx
left a comment
There was a problem hiding this comment.
Couple questions, nothing major.
| enable_release_device_manually: false | ||
| macos_setup_assistant: | ||
| manual_agent_install: | ||
| require_all_software_macos: false |
There was a problem hiding this comment.
Does it make sense to skip the require_all on one of these (or did you already do that elsewhere) so we can make sure the BC path works here?
There was a problem hiding this comment.
hm I tested it manually but not sure I did in one of the auto tests, will check
There was a problem hiding this comment.
Actually I think we're looking at it backwards, all of them are testing the case where we don't have the config set. These are the expected results from running fleetctl get config with a minimal config on the server. I updated one of the tests in apply_test.go that was already setting the "manual release" flag to also set this flag, to check that it gets set to true as expected.
| value={requireAllSoftwareMacOS} | ||
| onChange={setRequireAllSoftwareMacOS} | ||
| > | ||
| <TooltipWrapper tipContent="If any software fails, the end user won't be let through, and will see a prompt to contact their IT admin. Remaining software installs will be canceled."> |
There was a problem hiding this comment.
Very tangentially related, but looks like the script batch execution UI uses "cancelled." Mind, erm, taking an "L" there as part of this PR as otherwise it looks like the frontend is consistent with the spelling (didn't check backend...problem for another day).
Only set `didUpdate = true` if we actually, yknow, updated Co-authored-by: Ian Littman <iansltx@gmail.com>
iansltx
left a comment
iansltx
left a comment
There was a problem hiding this comment.
Unblocking merge with the assumption that there's an automated test somewhere that doesn't explicitly set the new config. If there isn't one, please add it and I'll re-approve.
|
JS test failure appears to be flakey/unrelated. If it fails again, mind rebasing off of |
2 participants
Related issue: Resolves #33110
Related issue: Resolves #33109
Details
This PR implements the new "cancel setup if any software fails on macos" flag, including both backend and frontend logic.
Half of the file changes are updating test expectations / auto-generated schema.
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.
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements)Testing
New Fleet configuration settings
fleetctl generate-gitopsmacos_setupis still excluded from generate-girtopsDocumented here
Summary by CodeRabbit