TLS naming cleanup#166
Merged
Merged
Conversation
added 3 commits
February 22, 2023 00:27
Renames NoTlsVerify to TlsNoVerify, adds Tls prefix to client cert vars & config keys. Adds missing config methods. Update tests to match new keys & methods. Fills in some gaps from first adding the TLS support. Adds comments indicating --no-tls-verify and its related envvar are deprecated.
chuson
approved these changes
Feb 22, 2023
chuson
left a comment
chuson
left a comment
There was a problem hiding this comment.
LGTM, merge if you don't agree w/ my nit otherwise I can approve again.
Comment on lines
+124
to
+127
| | --tls-no-verify | OTEL_CLI_TLS_NO_VERIFY | tls_no_verify | false | | ||
| | --tls-ca-cert | OTEL_EXPORTER_OTLP_CERTIFICATE | tls_ca_cert | | | ||
| | --tls-client-key | OTEL_EXPORTER_OTLP_CLIENT_KEY | tls_client_key | | | ||
| | --tls-client-cert | OTEL_EXPORTER_OTLP_CLIENT_CERTIFICATE | tls_client_cert | | |
There was a problem hiding this comment.
nit: Do you care that the second column isn't aligned here now?
Contributor
Author
There was a problem hiding this comment.
I care, but chose to do that so the whole table wouldn't reformat in the diff. The MD will render the table aligned in the browser so it felt like an ok tradeoff for now.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In #150 I didn't get all of the TLS variable name and config keys to match, which is already confusing me. In order to make things consistent as noted in #164, this renames all variables, template placeholders, and config keys to match the command line flags.
While cleaning up I found a few config things I missed and added them, along with test coverage.
It does change functionality a bit in the config file but no versions have been released with those keys available.