Show .sh scripts in macOS setup experience - #47629
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #47629 +/- ##
==========================================
+ Coverage 67.15% 67.20% +0.04%
==========================================
Files 3616 3617 +1
Lines 229035 229172 +137
Branches 11937 11791 -146
==========================================
+ Hits 153813 154004 +191
+ Misses 61367 61326 -41
+ Partials 13855 13842 -13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@claude review once |
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
This PR fixes macOS Setup experience software listings so .sh script-only packages (stored as platform='linux') appear and can be selected for macOS without interfering with Linux selections, by introducing a dedicated cross-platform selection table.
Changes:
- Add
setup_experience_software_installerstable + migration to track cross-platform setup-experience selections (e.g. linux.shselected for darwin). - Update setup experience datastore logic to read/write cross-platform selections and to prevent deleting installers that are selected cross-platform.
- Add datastore + integration tests covering
.shvisibility, selection, and independence between macOS and Linux.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/integration_mdm_setup_experience_test.go | Adds an end-to-end integration test covering macOS listing/saving of script-only .sh packages. |
| server/datastore/mysql/software_titles.go | Adjusts software title listing SQL to include linux .sh packages in darwin setup-experience queries and compute selection state via the new table. |
| server/datastore/mysql/software_installers.go | Extends delete guard to prevent deleting installers selected via the new cross-platform table. |
| server/datastore/mysql/setup_experience.go | Implements cross-platform selection persistence and enqueuing for darwin hosts. |
| server/datastore/mysql/setup_experience_test.go | Adds datastore-level tests for cross-platform .sh behavior and independence. |
| server/datastore/mysql/schema.sql | Updates schema snapshot with the new table. |
| server/datastore/mysql/migrations/tables/20260615135619_AddSetupExperienceSoftwareInstallers.go | Adds migration creating setup_experience_software_installers. |
| server/datastore/mysql/migrations/tables/20260615135619_AddSetupExperienceSoftwareInstallers_test.go | Adds migration test validating table creation and FK cascade behavior. |
Files excluded by content exclusion policy (1)
- changes/43667-macos-script-only-setup-experience
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughA new MySQL table 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}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/datastore/mysql/software_installers.go (1)
2676-2689:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPreserve or block cross-platform setup selections during batch replacement.
The new direct delete guard covers
DeleteSoftwareInstaller, butBatchSetSoftwareInstallerscan still delete a linux.shinstaller that is selected for macOS setup experience: the pre-delete check only countssoftware_installers.install_during_setup, and the direct delete of replaced installer IDs will cascade-removesetup_experience_software_installersrows. Add the cross-table to this guard and either re-point mappings to the replacement installer ID or returnerrDeleteInstallerInstalledDuringSetupwhen setup configuration is not being replaced. As per coding guidelines, SQL queries intended to return data for a specific entity must apply precise filtering criteria.Directionally, include the cross-table in the guard
var countInstallDuringSetup int if err := sqlx.GetContext(ctx, tx, &countInstallDuringSetup, stmt, args...); err != nil { return ctxerr.Wrap(ctx, err, "check installers installed during setup") } if countInstallDuringSetup > 0 { return errDeleteInstallerInstalledDuringSetup } + + stmt, args, err = sqlx.In(` + SELECT COUNT(*) + FROM setup_experience_software_installers seti + JOIN software_installers si ON si.id = seti.software_installer_id + WHERE si.global_or_team_id = ? + AND si.title_id NOT IN (?)`, + globalOrTeamID, titleIDs, + ) + if err != nil { + return ctxerr.Wrap(ctx, err, "build statement to check cross-platform setup installers") + } + var countCrossInstallDuringSetup int + if err := sqlx.GetContext(ctx, tx, &countCrossInstallDuringSetup, stmt, args...); err != nil { + return ctxerr.Wrap(ctx, err, "check cross-platform installers installed during setup") + } + if countCrossInstallDuringSetup > 0 { + return errDeleteInstallerInstalledDuringSetup + } }For the replaced-installer delete at Line 3180, migrate
setup_experience_software_installersrows from the old installer ID toinstallerIDbefore deleting, or block the replacement when the incoming installer is no longer an eligible linux.shpackage.Also applies to: 3172-3181
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/datastore/mysql/software_installers.go` around lines 2676 - 2689, The pre-delete guard at the countInstallDuringSetupNotInList check only validates software_installers.install_during_setup but ignores cross-table references in setup_experience_software_installers, allowing cascade deletion of setup configurations when installers are replaced. Extend the guard check to include the setup_experience_software_installers table to detect when a replaced installer is selected for setup experience, and at the location where replaced installer IDs are deleted (the direct delete operation around line 3180), either migrate the setup_experience_software_installers rows to point to the replacement installerID before deletion, or block the replacement entirely when the incoming installer is not an eligible linux .sh package. Apply precise SQL filtering criteria as per coding guidelines.Source: Coding guidelines
server/datastore/mysql/setup_experience.go (1)
633-668:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
global_or_team_idconsistently for no-team setup counts.
globalOrTeamIDis derived for the new cross-table count, but the native installer, VPP, and script subqueries still useteam_id = ?with the nullableteamIDpointer. WhenteamID == nil, those predicates won't match no-team rows, so the setup count can report only cross-platform selections and miss native setup items.Proposed fix
( (SELECT COUNT(*) FROM software_installers - WHERE team_id = ? + WHERE global_or_team_id = ? AND install_during_setup = 1 AND platform = ?) + (SELECT COUNT(*) FROM setup_experience_software_installers @@ ( SELECT COUNT(*) FROM vpp_apps_teams - WHERE team_id = ? + WHERE global_or_team_id = ? AND platform = ? AND install_during_setup = 1 ) AS vpp, ( SELECT COUNT(*) FROM setup_experience_scripts - WHERE team_id = ? + WHERE global_or_team_id = ? ) AS scripts` @@ ctx, ds.reader(ctx), sec, stmt, - teamID, platform, + globalOrTeamID, platform, globalOrTeamID, platform, - teamID, platform, - teamID, + globalOrTeamID, platform, + globalOrTeamID, ); err != nil {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/datastore/mysql/setup_experience.go` around lines 633 - 668, The VPP and scripts subqueries in the setup experience count SQL use team_id = ? with the nullable teamID pointer, which fails to match no-team rows when teamID is nil. Update both subqueries to use global_or_team_id = ? instead of team_id = ? in their WHERE clauses. Specifically, change the vpp_apps_teams subquery WHERE clause from team_id = ? to global_or_team_id = ?, and change the setup_experience_scripts subquery WHERE clause from team_id = ? to global_or_team_id = ?. Then update the parameters passed to sqlx.GetContext to use globalOrTeamID instead of teamID for these two subqueries, ensuring consistent use of the derived globalOrTeamID value across all table references.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@server/datastore/mysql/setup_experience.go`:
- Around line 633-668: The VPP and scripts subqueries in the setup experience
count SQL use team_id = ? with the nullable teamID pointer, which fails to match
no-team rows when teamID is nil. Update both subqueries to use global_or_team_id
= ? instead of team_id = ? in their WHERE clauses. Specifically, change the
vpp_apps_teams subquery WHERE clause from team_id = ? to global_or_team_id = ?,
and change the setup_experience_scripts subquery WHERE clause from team_id = ?
to global_or_team_id = ?. Then update the parameters passed to sqlx.GetContext
to use globalOrTeamID instead of teamID for these two subqueries, ensuring
consistent use of the derived globalOrTeamID value across all table references.
In `@server/datastore/mysql/software_installers.go`:
- Around line 2676-2689: The pre-delete guard at the
countInstallDuringSetupNotInList check only validates
software_installers.install_during_setup but ignores cross-table references in
setup_experience_software_installers, allowing cascade deletion of setup
configurations when installers are replaced. Extend the guard check to include
the setup_experience_software_installers table to detect when a replaced
installer is selected for setup experience, and at the location where replaced
installer IDs are deleted (the direct delete operation around line 3180), either
migrate the setup_experience_software_installers rows to point to the
replacement installerID before deletion, or block the replacement entirely when
the incoming installer is not an eligible linux .sh package. Apply precise SQL
filtering criteria as per coding guidelines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b4326520-44c1-4ecc-809a-e309eda6b94b
📒 Files selected for processing (9)
changes/43667-macos-script-only-setup-experienceserver/datastore/mysql/migrations/tables/20260615135619_AddSetupExperienceSoftwareInstallers.goserver/datastore/mysql/migrations/tables/20260615135619_AddSetupExperienceSoftwareInstallers_test.goserver/datastore/mysql/schema.sqlserver/datastore/mysql/setup_experience.goserver/datastore/mysql/setup_experience_test.goserver/datastore/mysql/software_installers.goserver/datastore/mysql/software_titles.goserver/service/integration_mdm_setup_experience_test.go
jkatz01
left a comment
jkatz01
left a comment
There was a problem hiding this comment.
Looks good! Maybe we could drop the software_installers.install_during_setup flag and just use this new table if this gets more complicated in the future?
Related issue: Resolves #43667
Summary
.shscript-only packages weren't appearing in Controls → Setup experience → Software → macOS. The macOS tab filtered titles by exact installer platform (si.platform IN (...)), so.shrows (stored asplatform='linux') only showed up on the Linux tab. A new join tablesetup_experience_software_installersnow tracks linux-.sh-chosen-for-darwin selections independently of the installer's nativeinstall_during_setupflag, so the macOS and Linux tabs no longer interfere with each other.Checklist for submitter
Changes file added for user-visible changes in
changes/.Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements).Testing
CrossPlatformShScriptsdatastore subtests; newTestSetupExperienceMacOSScriptOnlyPackageMDM integration test)Database migrations
COLLATE utf8mb4_unicode_ci).Summary by CodeRabbit
.shsoftware packages, including cross-platform selections. macOS and Linux selections are tracked independently..shscript-only selection behavior and independent macOS vs Linux listings.