fix(cli): add ability to disable SSL verification #2717
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2717 +/- ##
=======================================
Coverage 96.44% 96.44%
=======================================
Files 88 88
Lines 5761 5763 +2
=======================================
+ Hits 5556 5558 +2
Misses 205 205
Flags with carried forward coverage won't be shown. Click here to find out more.
|
3e49170 to
0a1ed42
Compare
There was a problem hiding this comment.
Thanks a lot @JohnVillalovos! This is nice and simple but I think we might need a more general fix, WDYT?
| ssl_verify_group.add_argument( | ||
| "--no-ssl-verify", | ||
| help="Disable SSL verification", | ||
| required=False, | ||
| dest="ssl_verify", | ||
| action="store_false", | ||
| ) | ||
|
|
There was a problem hiding this comment.
I like the simplicity of this, but it doesn't fully fix #2714. We still cannot use environment variables (this is true for all of our booleans on the CLI). We'd need some kind of strtobool logic for those, and that maybe also falls back to strings for the ssl_verify arg. Also, the new flag doesn't have an env var equivalent this way like the others do. WDYT?
I don't disagree that it could be better but it does handle the issue brought up in #2714 As it will allow them to disable At the moment I'm not sure if I personally have the time to start down the path that will be required to make all environment variables work. If someone wants to attempt it then the strtobool could be done similar to this: https://github.com/python/cpython/blob/f55cb44359821e71c29903f2152b4658509dac0d/Lib/configparser.py#L1092-L1097 |
0a1ed42 to
6853bed
Compare
Sorry for the delay here, I was just thinking about whether we should introduce something that would potentially be deprecated again if we have a strtobool solution later, maybe we can go ahead with this if I don't get to it before the 28th 😁 |
Add a `--no-ssl-verify` option to disable SSL verification Closes: #2714
6853bed to
be07c99
Compare
Add a
--no-ssl-verifyoption to disable SSL verificationCloses: #2714
Changes
Documentation and testing
Please consider whether this PR needs documentation and tests. This is not required, but highly appreciated: