launcher doctor subcommand #1197
Conversation
| version.PrintFull() | ||
| cRunning.Println("\nRunning Kolide launcher checkups...") | ||
|
|
||
| checkups := []*checkup{ |
There was a problem hiding this comment.
Would we want to also check for --
- Launcher process exists?
- Launcher desktop process exists?
- Is there anything we could check for osquery?
There was a problem hiding this comment.
I'll add osquery binary and logs checks next.
A launcher process report could be useful info
| } | ||
| } | ||
|
|
||
| func (c *checkPointer) logConnections() { | ||
| func (c *checkPointer) Connections() map[string]string { |
There was a problem hiding this comment.
Seems fine. Another route would be to take a logger in.
| // checkup encapsulates a launcher health checkup | ||
| type checkup struct { | ||
| name string | ||
| check func() (string, error) |
There was a problem hiding this comment.
This signature feels very simple, maybe a good enough start. But I think there are a couple things I don't know how to do. (but maybe they don't matter)
First, I don't think we can differentiate between an unexpected failure in code, vs code works but a broken condition. Like, if we failed to open a boltdb connection that feels different than if some piece of data isn't what we need. Both are the same failure to the user, but one feels like a protocol error, and the other feels like the correct reporting of data.
Second, I'm not sure how to represent warnings here.
If I combine those, I wonder about func() (info string, status enum, error)
There was a problem hiding this comment.
But that suggestion makes it hard to differentiate the output for flare vs doctor.
There was a problem hiding this comment.
Oh, I see now. It's in the check's run call. hmmmmm
| name: "Root directory contents", | ||
| check: func() (string, error) { | ||
| return checkupRootDir(getFilepaths(k.RootDirectory(), "*")) | ||
| }, |
There was a problem hiding this comment.
I haven't run this, but thinking aloud I see a difference in doctor and flare.
For doctor, I think this would just want to check if the directory exists. Maybe permissions. But flare would want to display the contents. I suspect with reasonable clever callers and signatures, this is supportable by the same underlying checkup function
There was a problem hiding this comment.
nice work! I like the overall approach and the UX. Looks like it will be straight forward to add new checkups in the future. like the other comments, I think we should get smart about consolidating flags and preventing drift between what launcher needs and what doctor thinks it needs
Co-authored-by: seph <[email protected]>
Co-authored-by: seph <[email protected]>
Co-authored-by: James Pickett <[email protected]>
| // When launcher proper runs, it's expected that these defaults are their zero values | ||
| // However, special launcher subcommands such as launcher doctor can override these |
There was a problem hiding this comment.
Huh, this is an interesting way to do this.
Fixed #1145
This PR adds a
launcher doctorsubcommand which runs several checkups to verify launcher installation health.Notable things in this change:
parseOptionfunction.