Skip to content
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 client certificates for HTTP requests #767

Merged
merged 4 commits into from
Nov 23, 2016

Conversation

sykaeh
Copy link
Contributor

@sykaeh sykaeh commented Nov 9, 2016

This fixes #737

I also used this opportunity to consolidate the parameters that are supplied for HTTP requests and added the parameter parsing in the general CollectorBot.

There also seemed to be inconsistencies whether http_ssl_proxy or https_proxy should be used. I replaced all of them with https_proxy (seemed to be more common and documented).

@sebix
Copy link
Member

sebix commented Nov 10, 2016

  • Renaming parameters means breaking installations
  • The newly added parameters are not documented in docs/Bots.md
  • http_proxy and https_proxy are provided by defaults as None, you explicitly change it to empty string :(
  • As they are always provided, no need to use the getattr workaround
  • ssl_cl_cert is not self-explaining, better use ssl_client_cert

@sebix sebix added component: bots component: configuration feature Indicates new feature requests or new features labels Nov 10, 2016
@sebix sebix added this to the v1.0 Stable Release milestone Nov 10, 2016
@sebix sebix self-assigned this Nov 10, 2016
@aaronkaplan
Copy link
Member

overall : looks ok. Did a brief review of the code. Please let's test this internally and merge.
Thanks @sykaeh

@codecov-io
Copy link

codecov-io commented Nov 22, 2016

Current coverage is 75.70% (diff: 5.26%)

Merging #767 into master will increase coverage by 0.16%

@@             master       #767   diff @@
==========================================
  Files           223        223          
  Lines          8167       8151    -16   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           6170       6171     +1   
+ Misses         1997       1980    -17   
  Partials          0          0          

Powered by Codecov. Last update 3a92daa...f3114dc

@sykaeh
Copy link
Contributor Author

sykaeh commented Nov 22, 2016

Renaming parameters means breaking installations

I think for the sake of simplicity and clarity it is worth it. Also, if we change it before 1.0, we do not have to try to maintain the inconsistencies in the future.

The newly added parameters are not documented in docs/Bots.md

There are a lot of things that are not documented in docs/Bots.md. I have now added all of the generic collectors along with their parameters. As I have mentioned before in #757, it would be nice to have a better solution to this. Also, I do not understand what exactly is meant by name, lookup and public in the Bots.md file.

http_proxy and https_proxy are provided by defaults as None, you explicitly change it to empty string :(

Sorry, I hadn't noticed that. I changed it to null.

As they are always provided, no need to use the getattr workaround

I just copied what was done before, but I agree and have now changed it.

ssl_cl_cert is not self-explaining, better use ssl_client_cert

Same, I just copied what was done before, but I changed it to ssl_client_cert

@sebix sebix merged commit f3114dc into certtools:master Nov 23, 2016
@sykaeh sykaeh deleted the client-cert branch November 30, 2016 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: bots component: configuration feature Indicates new feature requests or new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants