Fix certificate verification error when set to False #588
Conversation
|
This PR needs to be closed and reopened because the branch from needs adjustment and the branch it is requesting it be merged into needs adjustment. This is the first PR we've done together so its understandable. Here are three things to consider: (1) The branch it is coming from needs to not be master, but instead a branch dedicated to this fix. By convention we name the branch the BZ number of the bug. That branch needs to live in your forked version of pulp (AndreaGiardini/pulp) and should be named 1155192. (2) The bug is targeting 2.5.1, and currently the branch that carries that version is 2.5-dev. This PR is submitting it back to master, but that would put the bugfix into a > 2.5.1 release, so the PR needs to target the 2.5-dev branch, not master. (3) The branch you make in your fork (1155192) should branch from a version 2.5.1 or earlier. If you branch from an earlier branch (ie: master) and try to merge backwards to 2.5-dev (2.5.1), then you'll get a LOT more than just your change. Since it's going in 2.5.1, I recommend branching 1155192 from 2.5-dev so that the merge back into 2.5-dev will be just your changes. |
|
Thanks for the submission. This fix looks right, but it would be even better with tests! Would you be willing to write test coverage for the PulpBindings init method to ensure that PulpConnection receives the right host and port, cert_filename, and verify_ssl? If the PR can be opened from/to the right branch (comment above) and some tests come with it, then I'll merge it! If you have trouble with the tests write questions on the PR, pulp-list, or the IRC channel. |
|
I forgot to mention that it probably should go somewhere that makes sense in here: https://github.com/pulp/pulp_rpm/tree/master/handlers/test/unit/handlers Typically tests mirror the folder structure of the path to the code that is being tested. |
|
Thank you for your suggestions |
Fixes #1155192