Skip to content

Conversation

andrewcheelightstep
Copy link

Modified the argument parser for the --use_tls argument because it wasn't parsing booleans correctly.

@carlosalberto
Copy link
Contributor

Hey @andrewcheelightstep

Thanks for the PR (and sorry for the delayed review). The change looks right, and the only thing I wonder is the default value - before this change, the expected default value was to use TLS, while with this change the default would be to not use it.

Maybe we should rename it to no_tls, so by default we stil use TLS? Else, we can merge, I'd say.

@katiabazzi @frenchfrywpepper

@joeblubaugh
Copy link
Contributor

Agree with @carlos - it's just an example program, but --disable_tls seems like a better name for this behavior. Then we can set store_false and default to TLS

@andrewcheelightstep
Copy link
Author

I have no problems with this. Is this something we can merge soon?

frenchfrywpepper added a commit that referenced this pull request Sep 5, 2018
This was the default before my proto change. And I noticed this
open PR #43
which had some feedback about that.
@andrewhsu andrewhsu added the bug label Jun 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants