Update backend and GitOps to handle AppleOSUpdateSettings.UpdateNewHosts#37295
Update backend and GitOps to handle AppleOSUpdateSettings.UpdateNewHosts#37295juan-fdz-hawa merged 13 commits intomainfrom
Conversation
…when using gitops
juan-fdz-hawa
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #37295 +/- ##
==========================================
+ Coverage 65.93% 65.95% +0.02%
==========================================
Files 2330 2333 +3
Lines 185238 185788 +550
Branches 7807 7807
==========================================
+ Hits 122140 122542 +402
- Misses 51940 52034 +94
- Partials 11158 11212 +54
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:
|
juan-fdz-hawa
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
juan-fdz-hawa
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
sgress454
left a comment
sgress454
left a comment
There was a problem hiding this comment.
Main logic looks great! Question about the GetMDMAppleOSUpdatesSettingsByHostSerial() update but it's not critical, and one test nit, but otherwise gtg.
| // what we expect from the machine info. But that would involve work to derive the platform from | ||
| // the machine info (presumably from the product name, but that's not a 1:1 mapping). | ||
| settings, err := svc.ds.GetMDMAppleOSUpdatesSettingsByHostSerial(ctx, m.Serial) | ||
| platform, settings, err := svc.ds.GetMDMAppleOSUpdatesSettingsByHostSerial(ctx, m.Serial) |
There was a problem hiding this comment.
A lower-touch alternative to updating the GetMDMAppleOSUpdatesSettingsByHostSerial method to return the platform might be to check the m.Product or m.SoftwareUpdateDeviceID and derive the platform from that. Looks like @gillespi314 implemented this at least one place that I can see but I'd want a second opinion about how reliable it is. Any thoughts Sarah?
There was a problem hiding this comment.
That works for me.
I guess a counter point for doing that is that if Apple comes up with a new device name next year (the iBlock or iRock, w/e) we might need to update the code base to account for that.
There was a problem hiding this comment.
Yeah and if we were gonna go in that direction, best to put it in a separate method. But since GetMDMAppleOSUpdatesSettingsByHostSerial is only used in this one spot anyway, I don't have a lot of energy around refactoring, +1 on keeping it as-is.
| require.Equal(t, "11.11.11", tmResp.Team.Config.MDM.IOSUpdates.MinimumVersion.Value) | ||
| require.Equal(t, "2022-02-02", tmResp.Team.Config.MDM.IOSUpdates.Deadline.Value) | ||
| // UpdateNewHosts values are ignored for iOS | ||
| require.False(t, tmResp.Team.Config.MDM.IOSUpdates.UpdateNewHosts.Value) |
There was a problem hiding this comment.
I think we want to check .Valid instead of .Value here and in the other ios/ipad tests, to validate that it's not getting set to an actual false value.
It seems like it should really have Set: false, but I tried it and it always comes back as Set: true, Valid: false, Value: false, even when I tried adding omitempty to the definition. 🤷
There was a problem hiding this comment.
Sounds good - I think that's because we hard code Set: true when un-marshalling the type (don't know why).
juan-fdz-hawa
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
| // If "Update New Hosts" is checked but no version is set, we force an update | ||
| if !hasMinVersion { | ||
| level.Info(svc.logger).Log( | ||
| "msg", "checking os updates settings, minimum version not set, forcing macos update", | ||
| "serial", m.Serial, | ||
| ) | ||
| return true, nil | ||
| } |
There was a problem hiding this comment.
Shouldn't it always force? (No matter what you have configured on Minimum+Deadline)
There was a problem hiding this comment.
No - if Minimum Version and Update New Hosts are both set, then we need to check the OS version.
juan-fdz-hawa
deleted the
36188-back-end---add-update-new-hosts-config
branch
3 participants
Related issue: Resolves #36188
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.
Testing
New Fleet configuration settings
If you didn't check the box above, follow this checklist for GitOps-enabled settings:
fleetctl generate-gitops