-
Notifications
You must be signed in to change notification settings - Fork 92
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
server: Removing advanced TLS config parameters - BREAKING CHANGE #245
Conversation
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.
Makes sense to me!
This is a breaking change for someone who used this library between merge of #230 and now? |
Can you add something to the description what is meant by "hide"? Here's what I think: |
@bboreham certainly, I can improve the description of the PR. |
Is it possible to set those parameters at all after this change? If not, I don't think that would be "hide", it would be "remove". |
How can you set these config options using YAML if you use |
I meant someone using this library as code could then make their own arrangements to set the parameters. |
No.
That's a good point, didn't think of it. |
I see @bboreham's point, I think it makes sense. I can make this change. |
Looking at the specifics again, maybe it is ok to remove them completely.
If you update the description, title and git commit message to state what you want to do, and include your justification, I'll come back and look at the PR again. |
@bboreham ok, I can do that. |
Remove advanced TLS config parameters stemming from github.com/prometheus/exporter-toolkit/web, that were introduced in commit 953ac9f. Motivation for their removal being that users would most likely not want to change them, and they add corresponding configuration parameters to the Grafana Mimir project, that we don't want. We also think they're not interesting to the Grafana Tempo and Loki projects. Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
32c0955
to
2e9b589
Compare
Updated PR title and description, plus Git commit message, to clarify the intention and justification. |
I must comment that this project does not exist for the sole benefit of Grafana Labs, but we can take that as some input. |
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.
LGTM; thanks for updating.
Turns out
|
PR to re-introduce some TLS config: #256 |
I would like to remove advanced TLS config parameters stemming from github.com/prometheus/exporter-toolkit/web, that were introduced in commit 953ac9fb41437fee0bffcb364333ba624a35e043:
cipher_suites
curve_preferences
min_version
max_version
prefer_server_cipher_suites
The motivation for removing these advanced parameters is that users would most likely not want to change them, and they add corresponding configuration parameters to the Grafana Mimir project, that we don't want. We also think they're not interesting to the Grafana Tempo and Loki projects.