Update MDM migration flow with offline dialog#21274
Update MDM migration flow with offline dialog#21274gillespi314 merged 23 commits intofeat-mdm-migration-updatesfrom
Conversation
…eetdm/fleet into mdm-migration-offline-watcher
gillespi314
requested review from
getvictor,
lucasmrod and
roperzh
as code owners
roperzh
left a comment
roperzh
left a comment
There was a problem hiding this comment.
looks good, left some comments that can be fixed in a follow up
| func main() { | ||
| // FIXME: we need to do a better job of graceful shutdown, releasing resources, stopping | ||
| // tickers, etc. (https://github.com/fleetdm/fleet/issues/21256) | ||
| ctx, cancel := context.WithCancel(context.Background()) |
There was a problem hiding this comment.
nit and a question:
- Is this meant to be used as a general purpose context? if yes
ctxmakes sense, if no, could we rename it? - Should we rename
cancelnow that's called way down below?
|
|
||
| func (rw *ReadWriter) GetMigrationType() (string, error) { | ||
| data, err := rw.read() | ||
| data, err := rw.read() // TODO: confirm error handling with jahziel, what about other errors? |
There was a problem hiding this comment.
sanity check: have you folks decided here?
There was a problem hiding this comment.
I think this is referring to this function previously swallowing all errors that
weren'tErrNotExist. I updated it in one of my PRs to return the error correctly.
| select { | ||
| case <-ctx.Done(): | ||
| log.Debug().Msg("dialog context canceled") | ||
| // TODO: do we care about this? anything we need to clean up? |
There was a problem hiding this comment.
baseDialog has a method Exit() that's supposed to send a signal to kill swiftDialog, should we call it here?
| if !m.props.DisableTakeover { | ||
| flags = append(flags, | ||
| "--blurscreen", | ||
| "--ontop", | ||
| ) | ||
| } |
There was a problem hiding this comment.
nit: those are handled by the inner baseDialog, correct? so technically not required here? only asking because it was a tiny bit confused and had to go back to the other definition
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat-mdm-migration-updates #21274 +/- ##
=============================================================
Coverage ? 54.10%
=============================================================
Files ? 1458
Lines ? 137833
Branches ? 3349
=============================================================
Hits ? 74577
Misses ? 57198
Partials ? 6058
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| migrateMDMItem.Hide() | ||
| } | ||
|
|
||
| // TODO: we can probably extract this into a function that sets up both the migrator and the |
There was a problem hiding this comment.
Is this worth doing now? I'd say yes but not too big a deal.
There was a problem hiding this comment.
@jahzielv, do you want to give that a try as follow up in the feature branch?
| } | ||
|
|
||
| // StartMDMMigrationOfflineWatcher starts a watcher running on a 3-minute loop that checks if the | ||
| // device goes offline in the process of migrating to Fleet's MDM and offline. If so, it shows a |
There was a problem hiding this comment.
Typo I think?
| // device goes offline in the process of migrating to Fleet's MDM and offline. If so, it shows a | |
| // device goes offline in the process of migrating to Fleet's MDM and is offline. If so, it shows a |
|
|
||
| log.Debug().Msgf("offline dialog, device is unmanaged, migration type %s", mt) | ||
|
|
||
| // TODO: Maybe check show profiles and skip showing the dialog if the device is managed? |
There was a problem hiding this comment.
Alas, this will have to wait for another day as profiles show requires root :'(
|
Lint and other comments will be addressed separately |
3 participants
Replaces #21209 (because the commit history got all messed up by a bad rebase attempt)