Skip to content
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

Rework Windows Services #426

Merged
merged 15 commits into from
Feb 20, 2019
Merged

Rework Windows Services #426

merged 15 commits into from
Feb 20, 2019

Conversation

directionless
Copy link
Contributor

@directionless directionless commented Feb 19, 2019

We have a couple of hard to support windows corner cases. I think these stem from the difference between windows services, and traditional unix daemons. This PR aims to make them more approachable.

Moves the windows startup process to being explicit, instead of autotected on whether it's in windows and non-interactive. Now there are svc and svc-debug subcommands. This should remove a source of strange errors when interactivity is mis-detected, and also makes it easier to run launcher in various modes for debugging.

Remove the service management This removes the service management tools. That's better handled by ther packaging.

Change the flag parsing from kit/env to ff. This should allow us to more easily pass in a flag file.

Refactor several things in support of these.

I think this is mergeable now.

Move windows service functionality out of main, and into an svc subcommand.

Remove the service management stuff. That’s better handled by the packaging.

Replace the auto detection stuff with an explicit `svc` or `svc-debug`. The auto-detect is flakey and we should be explicit.
This will facilitate using a flag parsing lib
@directionless directionless changed the title Windows Rework Windows Services Feb 19, 2019
@directionless directionless marked this pull request as ready for review February 20, 2019 00:17
Copy link
Contributor

@blaedj blaedj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine to me, I don't have strong opinions on the flag parsing lib, looks like ff does the 'get default from ENV' behavior that we had.
I really like the tests, looks like they verify that both --flag and -flag work as expected?

Perhaps I'm missing it, but are we not keeping the installService, removeService etc. sub-commands around? If so, I'm sure there is a good reason - I just can't tell what it is from this PR.

cmd/launcher/options.go Outdated Show resolved Hide resolved
cmd/launcher/options.go Outdated Show resolved Hide resolved
cmd/launcher/options.go Show resolved Hide resolved
cmd/launcher/svc_windows.go Outdated Show resolved Hide resolved
cmd/launcher/svc_windows.go Show resolved Hide resolved
blaedj and others added 4 commits February 20, 2019 16:08
Co-Authored-By: directionless <github@directionless.org>
Co-Authored-By: directionless <github@directionless.org>
@directionless
Copy link
Contributor Author

Correct -- it removes the service management commands. I think those are better handled by the package tooling. I'm not opposed to bringing them back if we find we need them.

Yes, the tests are trying to very both - and -- work. I've added a comment. The tests are a bit silly, since that should be tested by the ff library. But it seemed good to ensure we don't break something along the way

Copy link
Contributor

@blaedj blaedj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@directionless directionless merged commit 1e7be61 into kolide:master Feb 20, 2019
@directionless directionless deleted the windows branch February 20, 2019 21:37
directionless added a commit that referenced this pull request Feb 25, 2019
Now that we support flagfiles for configuration (#426) let's start using them.

* moves the launcher options into a flag file
* adds things to wix to correctly create services
* Fixes some bugs around wix template variables
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants