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](4e137d9b00 (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
