-
Notifications
You must be signed in to change notification settings - Fork 314
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
add truststore dependency #725
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.
Using truststore looks fine to me, just needs a few cleanups and it should be fine to merge.
gcalcli/cli.py
Outdated
@@ -20,6 +20,8 @@ | |||
# Everything you need to know (Google API Calendar v3): http://goo.gl/HfTGQ # | |||
# # | |||
# ######################################################################### # | |||
import truststore | |||
truststore.inject_into_ssl() |
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.
Linter doesn't like this. I'd move the inject_into_ssl()
call down into main() and add a small comment about what it does, then move the import down to be sorted.
pyproject.toml
Outdated
@@ -26,6 +26,7 @@ classifiers = [ | |||
"Programming Language :: Python :: 3", | |||
] | |||
dependencies = [ | |||
"truststore", |
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.
Nit: Could you sort these dependencies above the "dev dependencies" comment?
Also might be nice to have an opt-out for this if there's any chance it could behave badly, but I think it's okay to add that later as needed. |
I'm fairly new to this so not sure if I did it correctly, but I attempted to address your feedback. |
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.
Looks great! Merging...
Err, looks like we'll need to either bump the minimum required python version or make this dep optional. I was thinking to bump it to 3.10 anyway. Want me to do that separately and then you can rebase your changes onto that? |
Sure, that works! |
Done: #727. |
gcalcli won't work on machines that have local certificates installed, e.g. in corporate environments where a firewall requires certificates installed on internal boxes. This is a simple fix that tells gcalcli to trust the local cert store. Perhaps this should be made an optional feature or there is some better/more secure way to do it, but I would very much appreciate a way to trust the local certificate store in the upstream code.
Fixes #726.