this is in response to
https://github.com/certbot/certbot/security/dependabot/126
as you can see by examining the github status checks on this PR, i ran
the full test suite and everything passed
i also don't think this PR requires two reviews
based on the suggestion @bmw made in #10484, this moves nearly
everything from `certbot-apache` and `certbot-nginx` into subdirectories
in `certbot/src/certbot/_internal`, and corresponding "extra"
dependencies are made for the certbot distribution. in their place,
entrypoint shims are made in the old distributions.
this way, installing `certbot[nginx]` will pull in the extra
dependencies needed for the nginx code, and also pull in the shim in
`certbot-nginx`, letting our plugin discovery system work just as it did
before. ditto for apache.
note that this doesn't yet deprecate anything, which was one of the
primary goals of the original issue -- i spun out that work into #10521fixes#10484
---------
Co-authored-by: Brad Warren <bmw@users.noreply.github.com>
Co-authored-by: ohemorange <erica@eff.org>
pass secrets and env vars
set minimum permissions
move permissions to job level, switch to contents read
change name to AWS_TEST_FARM_PEM for consistency
move comments, remove azure pipelines on-pr file
match permissions
remove runs-on: self-hosted. why was that even there?
add shell:bash
the auto-converted decided python 3.1 == 3.10. come on.
put python version in quotes
move python version to matrix
copy from matrix into env
matrix must take a list
update matrix syntax
remove composite
put composite back
Fixes https://github.com/certbot/certbot/issues/10180.
So first of all, the core issue here is that [pyca deliberately
chose](ec80c1c289/src/cryptography/utils.py (L15-L18))
to override the default python functionality and make deprecation
warnings appear by default. This isn't common. If they'd actually used a
`DeprecationWarning`, it wouldn't have shown up to users, at least. That
being said, we should still try to catch it, as we do in fact want to
know about deprecation warnings for our own updates.
To do that, this PR searches upwards for a `pytest.ini` file from the
file's location. If found, it reads the warnings from the file, and
passes them using the `PYTHONWARNINGS` env variable. It also explicitly
sets warnings to `error` always in case we can't find the `pytest.ini`,
and ignores the subsequent unverified-https-on-localhost warning. It
also fixes a warning in our test nginx config that seemed reasonable to
address.
I tested this by adding a temporary warning, which I then removed, but
since it turned out there were two other warnings, that wasn't actually
necessary.
Options I considered and rejected:
- Switch from `atexit` to calling `main` directly. To do this, we'd have
to switch our `main` function to something like a try-finally. That's
complicated by the fact that we call `atexit` from other places in the
code. Also, `exc_info` isn't availabe in `finally` while it is in
`at_exit`, so it's not as versatile. But mostly if we wanted to do this,
we'd have to implement a custom atexit handler, basically, and that
seems worse than this option.
- Looking into pytest-forked. It's apparently buggy and not being
maintained. Not even sure this is what it's for anyway.
- Multiple
[-W](https://docs.python.org/3/using/cmdline.html#cmdoption-W) options
can be given instead of an env variable. The env version seemed cleaner.
- More closely mimicking [how pytest finds ini
files](https://docs.pytest.org/en/stable/reference/customize.html#finding-the-rootdir).
It seemed unnecessary to me.
Potential drawbacks:
- If we move or rename the `pytest.ini` file and for some reason don't
do a reasonable grep for `pytest.ini`, we will no longer catch any
additional `ignore`s in there. But imo we're likely to do that grep, and
also a missing ignore will then show up when we run the tests.
this is part of https://github.com/certbot/certbot/issues/10517
to update this description in response to the discussion below, i'd
recommend reviewing this PR by commit. the first commit just moves
ocsp.py under _internal making no other changes while the second commit
fixes everything else up. the diff really isn't as big here as it looks
i noticed this when reviewing jsha's upcoming blog post
this probably should have been done as part of
https://github.com/certbot/certbot/pull/10544, but we forgot to do it
then
i don't think this PR requires two reviews
