https://github.com/certbot/certbot/pull/10146 was supposed to do this,
but because of multiple code paths, it did not. This PR simplifies the
code by creating a single code path.
In particular:
- `hooks.renew_hook()` is removed. There are now only calls to
`hooks.deploy_hook()`, which is called during certonly, run, and renew,
and runs both cli and directory hooks.
- `cli_config.renew_hook` is removed. Both `--renew-hook` (hidden option
kept for backwards compatibility purposes and `--deploy-hook` now set
`cli_config.deploy_hook`, which is used internally. When either or both
flags are used multiple times, the last value is kept, which is the
argparse default.
- references to running a "renew hook" internally are changed to "deploy
hook"
- To maintain downgrade compatibility, `deploy_hook` is written out to
renewal config files as `renew_hook`. This is achieved by translating to
and from `renew_hook` in `storage.py` and changing
`renewal.STR_CONFIG_ITEMS` to contain `deploy_hook`.
This results in the following behavior changes:
- Directory hooks are now run when getting a new cert using certonly/run
- If someone set a renew hook on the cli using `--renew-hook`, it would
previously not be run when getting a new (non-renewed) cert, but now
will be. But this option is hidden and should no longer be used anyway.
- When using `certbot reconfigure`, if someone sets `--renew-hook`
certbot will now also ask if someone would like to do a test run of the
new hook, whereas before it would only do so for `--deploy-hook`.
---------
Co-authored-by: Brad Warren <bmw@users.noreply.github.com>
Fixes#10423.
In
https://github.com/certbot/certbot/pull/10409#issuecomment-3180214385,
we noted that a comment in
`certbot-ci/src/certbot_integration_tests/certbot_tests/test_main.py:test_renew_with_ec_keys`
says:
> since ecdsa is now default, the integration test is not actually
testing "When running non-interactively, if --key-type is unspecified
but the default value differs to the lineage key type, Certbot should
keep the lineage key type." as it says in the comment. To fix that, it
should be initially created with rsa, not ecdsa.
That is no longer accurate, since the default key type changed to
ecdsda.
This PR adds a new test that does what the comment specifies, and
updates the comment to reflect that the existing test no longer does
that.
i don't think the conversation at
https://github.com/certbot/certbot/pull/10495#discussion_r2699618989 is
urgent/important enough to make a github issue for it, but i also feel
like it's worth keeping a link around in case any devs have problems
with this code in the future. i think there was some good ideas in there
i don't think this PR requires two reviews
this is the final part of
https://github.com/certbot/certbot/issues/10195. this fixes
https://github.com/certbot/certbot/issues/10195
the changes in the first commit were done automatically with the
command:
```
ruff check --fix --extend-select UP006 --unsafe-fixes
```
the second commit configures ruff to check for this to avoid regressions
thanks for bearing with me thru these somewhat large automatically
generated PRs ohemorange 🙏
Alternative implementation for #7908.
In this PR:
- set up ruff in CI (add to `tox.ini`, mark dep in `certbot/setup.py`)
- add a `ruff.toml` that ignores particularly annoying errors. I think
line length isn't actually necessary to set with this workflow since
we're not checking it but putting it there for future usage.
- either fix or ignore the rest of the errors that come with the default
linting configuration. fixed errors are mostly unused variables. ignored
are usually where we're doing weird import things for a specific reason.
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.
Previously, we were constructing an ACME client for ARI checking that
used the global value for `server`, not the one recorded in a lineage's
renewal file.
This resulted in errors in the logs and failure to observe ARI for
lineages that used a non-default `--server` (e.g. staging or non-Let's
Encrypt CAs).
---------
Co-authored-by: ohemorange <ebportnoy@gmail.com>
This depends on a pending Pebble pull request and so will fail
integration tests until/unless that lands:
https://github.com/letsencrypt/pebble/pull/501
However, I'd appreciate some eyes on this PR in this regard: is the
interface we're using in Pebble useful and appropriate? If not, we can
adjust the Pebble PR.
Inspired based on conversation on
https://github.com/certbot/certbot/pull/10307, but note that this just
tests the general case; it does not test the "default server differs
from lineage server" case yet; when I try adding that I get some bugs
that may reflect a problem in #10307 I need to fix (or may reflect that
I need to inhibit the `--server` flag rather than trying to override it
late in the command line).
Follow-up to #10241. The acme module code is mostly the same, except the
switch to return a tuple containing Retry-After.
This includes the CLI-side work to call out to the new `renewal_time`
method when checking for renewal.
I moved `should_autorenew` from `storage.py` into `renewal.py`, where it
fits better (and also this solves an import cycle problem). To make the
edits more visible I split this into one commit for the move and [one
commit for the subsequent
edits](https://github.com/certbot/certbot/pull/10272/commits/4e137d9b00fb0a299723978d7a289b881716d36b#diff-fad906e31304c767d620bfd243f4c7adf1e63a3420fd634ee57a0f6651c182cf).
This does not yet attempt to store the Retry-After info, or failure
retries, in renewal configs. I figured since that's a pretty big chunk
of work and design on its own, I wanted to get interim feedback as is. I
think this PR would be okay to land with the current default crons /
systemd timers that run twice a day. I think we should implement storage
of retry information before increasing the frequency of runs. And if the
team would like to hold off on landing any ARI until that storage is
done, I'm good with that too. 👍🏻
Fixes#10252.
See further discussion here: https://github.com/pypa/pip/issues/11457
We are doing option:
> Alternatively, enable the --use-pep517 pip option, possibly with
--no-build-isolation. The --use-pip517 flag will force pip to use the
modern mechanism for editable installs. --no-build-isolation may be
needed if your project has build-time requirements beyond setuptools and
wheel. By passing this flag, you are responsible for making sure your
environment already has the required dependencies to build your package.
Once the legacy mechanism is removed, --use-pep517 will have no effect
and will essentially be enabled by default in this context.
Major changes made here include:
- Add `--use-pep517` to use the modern mechanism, which will be the only
mechanism in future pip releases
- Change to `/src` layout to appease mypy, and because for editable
installs that really is the normal way these days.
- `cd acme && mkdir src && mv acme src/` etc.
- add `where='src'` argument to `find_packages` and add
`package_dir={'': 'src'},` in `setup.py`s
- update `MANIFEST.in` files with new path locations
- Update our many hardcoded filepaths
- Update `importlib-metadata` requirement to fix
double-plugin-entry-point problem in oldest tests
