-
Notifications
You must be signed in to change notification settings - Fork 755
GitOps support for scheduled auto updates settings #37851
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #37851 +/- ##
==========================================
+ Coverage 65.87% 65.98% +0.10%
==========================================
Files 2392 2372 -20
Lines 190799 189476 -1323
Branches 8364 7940 -424
==========================================
- Hits 125697 125021 -676
+ Misses 53685 53056 -629
+ Partials 11417 11399 -18
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:
|
| AutoUpdateStartTime: app.AutoUpdateStartTime, | ||
| AutoUpdateEndTime: app.AutoUpdateEndTime, | ||
| } | ||
| if err := svc.ds.UpdateSoftwareTitleAutoUpdateConfig(ctx, titleID, tmID, cfg); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is doing a query for each app, which I don't think is the end of the world if the number of apps is not that big.
Perhaps we could have UpdateSoftwareTitleAutoUpdateConfigs (plural) and we can pass a list of apps to have their auto update properties updated.
Also, I don't see a tx being used in this BatchAssociateVPPApps function and there are several mutative calls to the DB. Should we introduce a tx here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we could have UpdateSoftwareTitleAutoUpdateConfigs (plural) and we can pass a list of apps to have their auto update properties updated.
IMO something to optimize later if really need be.
Also, I don't see a tx being used in this BatchAssociateVPPApps function and there are several mutative calls to the DB. Should we introduce a tx here?
txs would generally go within fleet.Datastore methods not here.
Something to iterate for sure (not as part of this PR :)
ee/server/service/vpp.go
Outdated
| if app.AutoUpdateEnabled == nil && app.AutoUpdateStartTime == nil && app.AutoUpdateEndTime == nil { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GitOps works by applying what's set, and removing what's not set.
We should support the following scenario:
- Set these settings on a VPP app.
- Apply GitOps.
- Check on the UI that the update schedule config is set.
- Remove the three settings from the VPP app.
- Apply GitOps.
- Check on the UI that the update schedule config all unset.
Let me know if it makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, I'll update
lucasmrod
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double checking the following
I set (see typo in auto_update_enable, should be auto_update_enabled):
- app_store_id: "302584613"
platform: ipados
auto_update_enable: true
auto_update_start_time: '00:00'
auto_update_end_time: '00:30'
I got:
Error: applying app store apps for team: "📱🏢 Company-owned mobile devices": POST /api/latest/fleet/software/app_store_apps/batch received status 422 Validation Failed: Error 1048 (23000): Column 'enabled' cannot be null
Should probably set auto_update_enabled: false on the database, right?
Not the best UX, because the user won't notice the typo but it aligns to what we expect from GitOps.
| } | ||
| titleID, ok := appStoreIDToTitleID[app.VPPAppID.String()] | ||
| if !ok { | ||
| continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's log an error if this is the case (because it should not happen AFAICS).
| continue | |
| level.Error(svc.logger).Log("msg", "software title missing for vpp app", "adam_id", app.VPPAppID.String()) | |
| continue |
lucasmrod
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Left some comments/questions.
|
@lucasmrod I addressed the two cases you mentioned:
Screen.Recording.2026-01-07.at.3.56.37.PM.mov |
lucasmrod
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left two comments.
| default: | ||
| t.Fatalf("unexpected source: %s", foundTitle.Source) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test looks great.
At the end of it, could you apply with:
- app_store_id: "2"
platform: ipados
self_service: false
(Without the new fields)
And check that the fields are not set for that title anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably enough to check that enabled is false in the DB.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, should we delete the entry from software_update_schedules?
Let's say we do the following:
- Apply GitOps with
- app_store_id: "1016366447"
auto_update_enabled: true
auto_update_end_time: "19:30"
auto_update_start_time: "18:30"
platform: ios
an entry is created in software_update_schedules the values above
- Reapply GitOps with
- app_store_id: "1016366447"
platform: ios
Currently, the end state is that the entry in software_update_schedules is still there but with auto_update_enabled = false, and the times are not modified. Is this OK or should we be stricter and delete the entry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am ok if this iteration just sets the false in the DB for enabled.
ee/server/service/vpp.go
Outdated
|
|
||
| // Validate auto-update window | ||
| schedule := fleet.SoftwareAutoUpdateSchedule{SoftwareAutoUpdateConfig: cfg} | ||
| if err := schedule.WindowIsValid(); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems we can apply invalid settings (when auto_update_enabled: false):
- app_store_id: "302584613"
platform: ipados
auto_update_enabled: false
auto_update_start_time: '00:00'
auto_update_end_time: '00:30'
Seems because scheduled.WindowIsValid() is a no-op when app.AutoUpdateEnabled != nil || !*app.AutoUpdateEnabled.
Could we instead move the initial if in schedule.WindowIsValid() to outside the method where it's currently called in main? (That way WindowIsValid does indeed check the window validity.)
I'm all ears.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lucasmrod are you suggesting to get rid of:
if s.AutoUpdateEnabled == nil || !*s.AutoUpdateEnabled {
return nil
}at the top of WindowIsValid()?
If that's the case, I think we'd still hit the "Start and end time must both be set" error when auto_update_enabled: false and no times are provided.
... Or only call WindowIsValid() in the callers only if auto_update_enabled = true?
Note that we have 3 places where we perform this call
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and add such if in the current use of WindowIsValid in main.
cc @sgress454
Related issue: Resolves #35457
Checklist for submitter
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)If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes
Testing
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually
fleetctl generate-gitopsUI
Generated YAML
fleetctl gitopsSource YAML
I set the start_time to 18:30 and end_time to 19:30 for the first app
After the command ran, verified both on the UI and DB that the new values were set
New Fleet configuration settings
If you didn't check the box above, follow this checklist for GitOps-enabled settings:
fleetctl generate-gitopsfleet/docs/Configuration/yaml-files.md
Lines 538 to 540 in a79ff22
fleetd/orbit/Fleet Desktop
runtime.GOOSis used as needed to isolate changes