-
Notifications
You must be signed in to change notification settings - Fork 103
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
Windows services and move to flag files #415
Conversation
6f7c338
to
ef28caf
Compare
1ac2e36
to
037fe3a
Compare
c6f5672
to
3d38dc7
Compare
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.
overall lgtm, just a few comments/questions you can take or leave
} | ||
|
||
if p.InsecureGrpc { | ||
launcherFlags = append(launcherFlags, "--insecure_grpc") | ||
launcherBoolFlags = append(launcherBoolFlags, "insecure_grpc") |
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.
Are we going to want to support the -insecure_jsonrpc
flag?
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.
Erm. Yes, but I feel like we might want to merge -insecure_grpc
and -insecure_jsonrpc
into insecure_transport
. Thoughts?
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.
Yea that makes sense I think
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.
Though, I'd probably rather re-plumb that in a separate PR
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.
yea it can go in another PR
Now that we support flagfiles for configuration (#426) let's start using them.