Skip to content

Drop per-request cert, verify, and trust_env - #617

Merged
lovelydinosaur merged 7 commits into
masterfrom
drop/request-cert-verify-trustenv
Dec 29, 2019
Merged

Drop per-request cert, verify, and trust_env#617
lovelydinosaur merged 7 commits into
masterfrom
drop/request-cert-verify-trustenv

Conversation

@florimondmanca

Copy link
Copy Markdown
Contributor

Fixes #554.

This PR restricts configuration to a per-client, per-connection-pool and per-connection basis, instead of allowing to override it per-request:

  • Remove the cert, verify and trust_env parameters from the request methods on Client.
  • Drop cert and verify from the dispatcher API (and concrete dispatchers).

It probably shouldn't be merged before 1.0 lands, though. I just felt like going through this to see how it would end up looking like. 👍

@florimondmancaflorimondmanca added refactor Issues and PRs related to code refactoring api change PRs that contain breaking public API changes labels Dec 7, 2019
@florimondmanca
florimondmanca requested a review from a teamDecember 7, 2019 15:31
@lovelydinosaur

Copy link
Copy Markdown
Contributor

So keen to get this in! 😃

@florimondmanca

Copy link
Copy Markdown
ContributorAuthor

@tomchristie Is that a PR approval in disguise…? :-)

@lovelydinosaur

Copy link
Copy Markdown
Contributor

We shouldn’t merge yet, no, since it’d block us making 0.9.x point releases.

We could potentially have a version-1.0 branch tho?

@florimondmanca

florimondmanca commented Dec 8, 2019

Copy link
Copy Markdown
ContributorAuthor

Ah, of course.

I'm good with leaving this as-is for now. Managing v1 branches is a bit of a pain, especially since we don't know exactly when we're going to get there. But if you feel like it could be beneficial, I'd be okay with it.

@florimondmancaflorimondmanca added the do not merge PRs that should not be merged label Dec 8, 2019
@florimondmancaflorimondmanca added this to the v1.0 milestone Dec 8, 2019
@lovelydinosaur

Copy link
Copy Markdown
Contributor

Might be worth bringing this up to date now and starting a 1.0 branch?

@florimondmanca

Copy link
Copy Markdown
ContributorAuthor

Yup. What workflow would you recommend for managing the 1.0 branch? Merge master into 1.0 after every PR?

@lovelydinosaur

Copy link
Copy Markdown
Contributor

Yup. What workflow would you recommend for managing the 1.0 branch? Merge master into 1.0 after every PR?

I guess so, yea.

We probably want to:

That'll get us into a position where we're ready to start adding in a SyncClient again.

lovelydinosaur added a commit that referenced this pull request Dec 18, 2019
@lovelydinosaur

Copy link
Copy Markdown
Contributor

So, to get this in for a 0.10.0 we may want leave cert/verify/timeout on the public API but have them raise errors now, and don't include them in the docstrings. We can drop them throughout the internals and dispatch API.

Seem like a reasonable approach?

@florimondmanca

Copy link
Copy Markdown
ContributorAuthor

Sounds good - will update the removal of warnings to raise errors instead. 👍

@florimondmanca
florimondmancaforce-pushed the drop/request-cert-verify-trustenv branch from f575216 to 1aa032dCompareDecember 20, 2019 21:26
@florimondmanca
florimondmancaforce-pushed the drop/request-cert-verify-trustenv branch from 1aa032d to 44d550cCompareDecember 20, 2019 21:28
@florimondmanca

Copy link
Copy Markdown
ContributorAuthor

@tomchristie Should be ready for re-review 🎉

@florimondmancaflorimondmanca removed the do not merge PRs that should not be merged label Dec 20, 2019
@florimondmancaflorimondmanca removed this from the v1.0 milestone Dec 20, 2019
@lovelydinosaur

Copy link
Copy Markdown
Contributor

Looks great! How do we want to do this... should we gradually merge everything on #667 into a version-0.10.0 branch, or are we confident enough that it's a small set of stuff now, and just work on getting these bits into master and then releasing?

@florimondmanca

Copy link
Copy Markdown
ContributorAuthor

I'd be in favor of merging bits from #667 into master. :-) We've got the 0.9.5 tag available to us, so if there's any hot fix we need to release as a 0.9.6 patch we can still cherry-pick it there from master.

@lovelydinosaurlovelydinosaur left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic!

@lovelydinosaurlovelydinosaur added this to the 0.10.0 milestone Dec 23, 2019
@lovelydinosaurlovelydinosaur mentioned this pull request Dec 29, 2019
@lovelydinosaur
lovelydinosaur merged commit e9ebd1d into masterDec 29, 2019
@lovelydinosaur
lovelydinosaur deleted the drop/request-cert-verify-trustenv branch December 29, 2019 15:01
This was referenced Dec 31, 2019
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api changePRs that contain breaking public API changesrefactorIssues and PRs related to code refactoring

Development

Successfully merging this pull request may close these issues.

Drop per-request cert/verify/trust_env

2 participants

@florimondmanca@lovelydinosaur