-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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 async interface for finalization to acme.client.ClientV2 #9622
Conversation
Add `begin_order_finalization()`/`poll_finalization()` to `acme.client.ClientV2`, which are directly analogous to `answer_challenge()`/`poll_authorizations()`. This allows us to finalize an order and then later poll for its completion as separate steps.
@alexzorin , this is ready for review at your convenience. |
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 good, thanks! Just some feedback for naming and the changelog.
certbot/CHANGELOG.md
Outdated
* `acme.client.ClientV2` now provides an asynchronous interface for | ||
order finalization (`begin_order_finalization()` and | ||
`poll_finalization()`) |
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.
There is a small potential ambiguity here with Python's async
, and it would be good to mention that these relate to finalize_order
. Maybe something like:
acme.client.ClientV2
now provides separatebegin_order_finalization
andpoll_finalization
methods, in addition to the existingfinalize_order
method.
acme/acme/client.py
Outdated
def finalize_order(self, orderr: messages.OrderResource, deadline: datetime.datetime, | ||
fetch_alternative_chains: bool = False) -> messages.OrderResource: | ||
"""Finalize an order and obtain a certificate. | ||
def begin_order_finalization(self, orderr: messages.OrderResource |
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.
It would be good to align this name either with poll_finalization
or finalize_order
. Perhaps submit_finalization
or begin_finalization
.
Rename `begin_order_finalization` -> `begin_finalization` and tweak wording of changelog entry
OK, renamed the method and adopted your proposed wording for the changelog entry. Anything else? |
Add
begin_order_finalization()
/poll_finalization()
toacme.client.ClientV2
, which are directly analogous toanswer_challenge()
/poll_authorizations()
. This allows us to finalize an order and then later poll for its completion as separate steps. Also rewritefinalize_order()
to call these two methods (which are just the previous body offinalize_order()
split in two.)I've not added new test cases because the existing test cases for
finalize_order()
exercise all of the same code paths, but I can if we want to belt-and-suspenders this.Fixes #9620
Pull Request Checklist
master
section ofcertbot/CHANGELOG.md
to include a description of the change being made.AUTHORS.md
if you like.