mirror of
https://github.com/certbot/certbot.git
synced 2026-06-15 11:39:12 -04:00
035a6dcc39
Fixes https://github.com/certbot/certbot/issues/10259 This PR moves post-hook execution from `main.renew` to `renewal.handle_renewal_request` so that failed and renewed domains actually get passed into post-hook execution as promised, even when failures happened. I suspect the original PR was being overly cautious by putting the whole thing into a try/finally so that post-hooks definitely happen, but `handle_renewal_request` is already full of exception catching. I understand the worry about executing a pre-hook and then failing to execute its matching post-hook, but the code really is already structured to make sure that that won't happen. And then when we added `FAILED_DOMAINS` and `RENEWED_DOMAINS`, we both kept that overly-cautious hooks execution location, but also kept the error so we have a summary at the end...which meant that if failures happened, the env vars were never set. If we really want to keep the `hooks.run_saved_post_hooks` call on the outside of everything in main, we can, but then we will have to do one of the following: - pass in the output lists to be filled out during execution. not my favorite pattern - throw the output lists in the error object or make a wrapper error, not sure, haven't looked at `errors.py` too closely - stop raising that final error where we report failures at the very bottom. it's a little outdated maybe but I do like it and I think people are used to it - raise that error in main, returning the number of parse and renewal failures. this is my favorite of the options, but I still like it less than what I've implemented here. Here's the integration/regression test failing on main: https://dev.azure.com/certbot/certbot/_build/results?buildId=9237&view=logs&j=fca58cec-e7ce-563a-f36f-5c233894d750 You can see here that that branch just has the integration test without the fix (and removing other tests for efficiency): https://github.com/certbot/certbot/compare/main...test-fail-env-on-main It's the default, but just to be clear, this should definitely have two reviewers. |
||
|---|---|---|
| .. | ||
| certbot_integration_tests | ||
| snap_integration_tests | ||