-
-
Notifications
You must be signed in to change notification settings - Fork 118
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
Support adding custom CA #454
Conversation
Custom CA could be specified with env var `TAPLO_EXTRA_CA_CERTS`.
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.
Thanks a lot for the PR! It looks good to me.
Regarding where to put the reqwest builder function, I think that's the right place.
Regarding documentation, I would say site/site/configuration/using-schemas.md
might be appropriate since it seems this is mostly useful for the schema catalog download. A sentence at the end of the page saying that the root certificate is customizable with the TAPLO_EXTRA_CA_CERTS
environment variable for both the LSP and CLI is probably enough.
Regarding the error, I think using tracing should be enough like one of my comment shows. At least in Emacs it's easy to access the stderr of the LSP server. And when using the CLI it's directly visible. We could in theory improve the LSP case by producing a diagnostic or something, but that's probably overkill.
Thanks again for your work!
I love your suggestions and I have modified the code according to your comments! |
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.
Thanks!
Resolve #443
Inspired by Node.js setting, I named the env var to be
TAPLO_EXTRA_CA_CERTS
(plural here because one file could contain many certs).I extracted the getting
reqwest
client part as a function though I don't know exactly where the best location for this function is. I temporarily put it intaplo-common/util.rs
.I tested the build on my local environment described in the issue and it worked perfectly.
to: @ia0
How should we document this?
I was thinking about making the error more obvious and adding a suggestion to let the user know they need to add the environment variable. Though I don't really know how to match the
UnknownIssuer
error fromreqwest
.