Add GitOps support for macOS script-only packages in setup experience - #49088
Closed
cdcme wants to merge 2 commits into
Closed
Add GitOps support for macOS script-only packages in setup experience#49088cdcme wants to merge 2 commits into
cdcme wants to merge 2 commits into
Conversation
Contributor
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
Adds GitOps support for selecting macOS setup experience software from script-only (.sh) packages via a new setup_experience_platforms field, and reconciles cross-platform selections declaratively on each /software/batch apply.
Changes:
- Extend GitOps software package spec and API payloads with
setup_experience_platforms(tri-state semantics) and validate/canonicalize platform inputs. - Add datastore support for resolving installer IDs and writing cross-platform setup experience selections (
setup_experience_software_installers). - Update
fleetctl generate-gitopsto round-trip cross-platform setup experience selections back into YAML.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| changes/47629-gitops-scriptonly-macos-setup-experience | Release note entry for the feature (content excluded from review). |
| server/service/integration_enterprise_test.go | Adds integration coverage for cross-platform setup experience selection reconciliation for script packages. |
| server/service/client.go | Includes setup_experience_platforms in the batch software payload built from GitOps specs (tri-state preserved). |
| server/mock/datastore_mock.go | Adds mock hooks for new setup experience cross-installer datastore methods. |
| server/fleet/software_installer.go | Adds spec field, payload field, and helper functions for canonicalizing/validating setup experience platforms. |
| server/fleet/scripts.go | Extends SoftwareInstallerPayload with setup_experience_platforms tri-state field. |
| server/fleet/datastore.go | Extends datastore interface for cross-platform setup experience installer reconciliation. |
| server/datastore/mysql/software_installers.go | Adds query to resolve installer IDs by team and (filename, platform). |
| server/datastore/mysql/setup_experience.go | Adds datastore method to replace cross-platform setup experience installer selections for a team+platform. |
| pkg/spec/gitops.go | Hydrates setup_experience_platforms from GitOps spec into the package-level model. |
| ee/server/service/software_installers.go | Validates/canonicalizes setup_experience_platforms and reconciles cross-platform selections on each batch apply. |
| ee/server/service/software_installers_test.go | Adds unit tests for platform normalization/validation behavior. |
| cmd/fleetctl/fleetctl/generate_gitops.go | Emits setup_experience_platforms so UI selections can round-trip via generate → apply. |
Files excluded by content exclusion policy (1)
- changes/47629-gitops-scriptonly-macos-setup-experience
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+3247
to
+3249
| if slices.Contains(normalized, "darwin") && manualAgentInstall { | ||
| return errors.New(`Couldn't edit software. "setup_experience_platforms" cannot include "macos" if "macos_manual_agent_install" is enabled.`) | ||
| } |
Comment on lines
+3459
to
+3462
| } | ||
| rows, err := svc.ds.GetSoftwareInstallerIDsByTeamAndFilenamePlatform(ctx, teamID, filenames, platforms) | ||
| if err != nil { | ||
| return ctxerr.Wrap(ctx, err, "look up installer ids for cross-platform reconcile") |
Comment on lines
+620
to
+626
| // SoftwareInstallerLookupRow projects the columns needed to resolve an | ||
| // installer's identity from its (filename, platform) natural key. | ||
| type SoftwareInstallerLookupRow struct { | ||
| ID uint `db:"id"` | ||
| Filename string `db:"filename"` | ||
| Platform string `db:"platform"` | ||
| } |
7 tasks
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)server/service/integration_enterprise_test.goast-grep timed out on this file 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ee/server/service/software_installers.go (1)
3391-3410: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake the GitOps reconcile part of the same all-or-nothing apply.
BatchSetSoftwareInstallersandBatchSetInHouseAppsInstallerscommit beforereconcileGitOpsSetupExperienceCrossInstallersruns, so a later error leaves the installer rows applied while the cross-platformsetup_experience_software_installersstate stays stale until another successful apply.🤖 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 `@ee/server/service/software_installers.go` around lines 3391 - 3410, The GitOps reconcile is currently happening after the two batch writes, so a failure in reconcile can leave the installer state partially applied. Move the reconcile logic in reconcileGitOpsSetupExperienceCrossInstallers into the same transactional flow as BatchSetSoftwareInstallers and BatchSetInHouseAppsInstallers, or otherwise ensure all three operations share one all-or-nothing unit of work before returning from the apply path.
🤖 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.
Inline comments:
In `@server/datastore/mysql/software_installers.go`:
- Around line 1146-1176: The post-write lookup in
GetSoftwareInstallerIDsByTeamAndFilenamePlatform is using ds.reader(ctx), which
can hit a lagging replica and miss freshly inserted installers. Change this
lookup to use the primary connection path used elsewhere for write-after-read
flows in Datastore, so the reconciliation after BatchSetSoftwareInstallers
always sees the newly created rows. Keep the rest of the query-building logic
intact and update the call site(s) so this method is guaranteed to read from
primary during this reconciliation.
---
Outside diff comments:
In `@ee/server/service/software_installers.go`:
- Around line 3391-3410: The GitOps reconcile is currently happening after the
two batch writes, so a failure in reconcile can leave the installer state
partially applied. Move the reconcile logic in
reconcileGitOpsSetupExperienceCrossInstallers into the same transactional flow
as BatchSetSoftwareInstallers and BatchSetInHouseAppsInstallers, or otherwise
ensure all three operations share one all-or-nothing unit of work before
returning from the apply path.
🪄 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: 9dd75f01-5104-4669-986b-55b84dfc1c9b
📒 Files selected for processing (13)
changes/47629-gitops-scriptonly-macos-setup-experiencecmd/fleetctl/fleetctl/generate_gitops.goee/server/service/software_installers.goee/server/service/software_installers_test.gopkg/spec/gitops.goserver/datastore/mysql/setup_experience.goserver/datastore/mysql/software_installers.goserver/fleet/datastore.goserver/fleet/scripts.goserver/fleet/software_installer.goserver/mock/datastore_mock.goserver/service/client.goserver/service/integration_enterprise_test.go
Comment on lines
+1146
to
+1176
| func (ds *Datastore) GetSoftwareInstallerIDsByTeamAndFilenamePlatform( | ||
| ctx context.Context, teamID uint, filenames []string, platforms []string, | ||
| ) ([]fleet.SoftwareInstallerLookupRow, error) { | ||
| if len(filenames) != len(platforms) { | ||
| return nil, ctxerr.New(ctx, "filenames and platforms slices must have the same length") | ||
| } | ||
| if len(filenames) == 0 { | ||
| return nil, nil | ||
| } | ||
| // sqlx.In can't expand tuple IN, so build the placeholders manually. | ||
| rowPlaceholders := strings.Join(slices.Repeat([]string{"(?,?)"}, len(filenames)), ",") | ||
| args := make([]any, 0, len(filenames)*2+1) | ||
| args = append(args, teamID) | ||
| for i := range filenames { | ||
| args = append(args, filenames[i], platforms[i]) | ||
| } | ||
| stmt := fmt.Sprintf(` | ||
| SELECT | ||
| id, | ||
| filename, | ||
| platform | ||
| FROM software_installers | ||
| WHERE global_or_team_id = ? | ||
| AND is_active = 1 | ||
| AND (filename, platform) IN (%s)`, rowPlaceholders) | ||
| var rows []fleet.SoftwareInstallerLookupRow | ||
| if err := sqlx.SelectContext(ctx, ds.reader(ctx), &rows, stmt, args...); err != nil { | ||
| return nil, ctxerr.Wrap(ctx, err, "look up installer ids by team and filename+platform") | ||
| } | ||
| return rows, nil | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the reader() helper to see if it routes to primary after recent writes in the same context.
ast-grep run --pattern 'func ($_ *Datastore) reader($_) $_' --lang go server/datastore/mysql
rg -n -A 15 'func \(ds \*Datastore\) reader\(' server/datastore/mysqlRepository: fleetdm/fleet
Length of output: 1593
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the call path and any recent-write / primary-required context handling.
ast-grep outline ee/server/service/software_installers.go --view expanded
rg -n -A 40 -B 20 'reconcileGitOpsSetupExperienceCrossInstallers|BatchSetSoftwareInstallers|ctxdb\.WithPrimaryRequired|IsPrimaryRequired|primary required|recent write' ee/server/service/software_installers.go server/datastore/mysql server/libRepository: fleetdm/fleet
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the narrow call path around the batch-set flow and any primary-required context markers.
rg -n -A 25 -B 15 'reconcileGitOpsSetupExperienceCrossInstallers|BatchSetSoftwareInstallers\(|GetSoftwareInstallerIDsByTeamAndFilenamePlatform\(|WithPrimaryRequired|IsPrimaryRequired' ee/server/service/software_installers.go server/datastore/mysql/mysql.go server/lib server/contextRepository: fleetdm/fleet
Length of output: 17858
Use the primary for this post-write lookup
reader(ctx) only switches to primary when ctxdb.RequirePrimary is set, and this reconciliation runs right after BatchSetSoftwareInstallers without setting that hint. A lagging replica can miss newly inserted installers and leave setup_experience_software_installers incomplete on the first apply.
🤖 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 1146 - 1176, The
post-write lookup in GetSoftwareInstallerIDsByTeamAndFilenamePlatform is using
ds.reader(ctx), which can hit a lagging replica and miss freshly inserted
installers. Change this lookup to use the primary connection path used elsewhere
for write-after-read flows in Datastore, so the reconciliation after
BatchSetSoftwareInstallers always sees the newly created rows. Keep the rest of
the query-building logic intact and update the call site(s) so this method is
guaranteed to read from primary during this reconciliation.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #49088 +/- ##
==========================================
+ Coverage 68.03% 68.04% +0.01%
==========================================
Files 3743 3743
Lines 237236 237392 +156
Branches 12380 12380
==========================================
+ Hits 161393 161532 +139
- Misses 61269 61280 +11
- Partials 14574 14580 +6
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:
|
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:
Summary
Adds a
setup_experience_platformsfield to the GitOps software package spec so.shscript-only installers can be selected for macOS setup experience declaratively. Reconciles the cross-platform selection table on every batch apply.Checklist for submitter
If some of the following don't apply, delete the relevant line.
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes. See Changes files for more information.Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.Testing
Added/updated automated tests
QA'd all new/changed functionality manually
New Fleet configuration settings
fleetctl generate-gitopsSummary by CodeRabbit