Related to https://github.com/certbot/certbot/issues/10581
Following up on #10622, this PR converts the `full-test-suite`
[pipeline](https://dev.azure.com/certbot/certbot/_build?definitionId=4)
from Azure to Github Actions.
Nightly test changes for context not included in this PR are available
[here](https://github.com/certbot/certbot/compare/test-convert-full-pipeline...convert-all-pipelines).
Since this branch is named `test-convert-full-pipeline`, these tests
will show up in the checks section of this PR.
The major changes I made here are splitting the docker and snaps tests
for a better github actions UX, and removing the intermediate "stage"
file, since stages are not a concept in GHA. This means that we get the
nice dropdowns for the different categories on the left bar of the [test
run
page](https://github.com/certbot/certbot/actions/runs/25139155528/job/73684692548)
so it's easier to see each type of test. The very slight drawback is
that the four jobs listed in `.github/workflows/full_test_suite.yml` do
need to be duplicated in `nightly.yml`, but that's a reasonable tradeoff
to me.
Also, we now test our certbot and dns plugin snaps on all architectures
for the first time (using `dpkg --add-architecture` to run armhf tests
on an arm64 machine), which is very nice and in my opinion worth the
very slightly extra time and code.
In this PR, we build arm64 and amd64 snaps directly on github's runners.
armhf snaps are built using launchpad as before. This makes the workflow
file a little long. There are perhaps some micro-optimizations for code
deduplication I could make, like creating an action to install
dependencies based on the architecture, but I don't think it's super
worth it, especially since the dependencies vary enough that we'd still
need some code (for example, even between installing deps for certbot
and dns runs, we'd still need to additionally install `nginx-light`).
A very slight potential time improvement we could make here would be to
optionally depend on the different architectures before running their
respective tests. I'm not sure if this can be done without writing
different jobs, and since once those jobs start they run in parallel, it
didn't really seem worth looking into for me. I am of course open to
alternate points of view here and in general.
Another potential change to bring the two build strategies more in line
would be to stop using the python script to send off all the launchpad
builds, and instead put each in a separate, matrixed job like the github
jobs. We could even continue retrying the builds within each job. This
would mean that if one dns plugin build happens to fail three times, all
the builds wouldn't have to be retried. While I think that's not the
worst idea, I personally think that belongs in a separate PR, as this PR
is already quite long.
Speaking of the PR length, I can undo the changes made here to build
arm64 and amd64 snaps on github actions, to have a simpler
conversion-only PR to review. Some of the choices I made here,
particularly around UX, were based on the fact that the jobs would look
like this, so it might not be as clear why I made those choices, but if
it's easier to review it's no problem to put it back. I could also
remove the code that tests the other snaps since it's new, but I figured
it'd be nice to show that they are in fact being built correctly, since
otherwise the built snaps wouldn't be consumed anywhere.
---------
Co-authored-by: Brad Warren <bmw@users.noreply.github.com>
Related to https://github.com/certbot/certbot/issues/10581
This is the first step of migrating to github actions.
Nightly and full tests have been converted on branch
`convert-all-pipelines`; you can see additional changes to do those for
context
[here](https://github.com/certbot/certbot/compare/convert-pr-tests...convert-all-pipelines).
Some notes:
- All github workflows must be flat in the `.github/workflows/`
directory.
- Github actions doesn't have a concept of "stages." Instead, it
generates a dependency graph, which is kind of nice. You can see an
example of a more complicated one
[here](https://github.com/certbot/certbot/actions/runs/24580625688).
- I don't know why the actions in the left bar (under Actions tab -->
All workflows) are using the path instead of the listed name. I suspect
it has something to do with not being run on main. Once it's merged, if
the name doesn't change, we can delete previous runs and that will clear
the entry on the left.
- "permissions" is for the fine-grained github PAT. contents: read is
needed for the "checkout" action, which basically everything uses. it's
still best practice to define per-workflow. it can also be defined
per-job, but per-workflow seemed nicer to me.
[This](https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#permissions)
is the best permissions explanation I've found; [some
actions](https://github.com/actions/checkout) mention what permissions
they need.
- For definitions of the keywords to `on`, see
[here](https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows).
- Some of the potential inputs in tox steps are not used in this PR
because we're not running the AWS tests. It seemed messier to take them
out here and put them back later when the extended tests need them, but
I can do that on request.
We currently have a `main` [protection
rule](https://github.com/certbot/certbot/settings/branch_protection_rules/5466)
set that Azure pipelines PR test suite must pass before merging.
Obviously I don't want to turn that off before this PR is reviewed. In
github actions, it can only require a specific job to pass, though you
can have multiple. To address this, I've created a job that requires all
other jobs to pass, and that can be set at the required job. We probably
do not want to list every individual job, as that includes every job
generated by a matrix strategy. To find it in the protection rules page,
start typing "PR test suite success" and it will show up.
---------
Co-authored-by: Brad Warren <bmw@users.noreply.github.com>
Co-authored-by: Will Greenberg <willg@eff.org>
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>
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
i just updated this credential in CI and created a calendar event to
help us remember to update it
with the calendar event, i don't think we need the code comment here. it
wasn't updated last time and it's one less thing for us to remember to
do next year
i don't think this PR requires two reviews
with the changes i made to CI and our calendar, i think i can say this
fixes#10563
Updated the link to the Cloudflare API tokens page for accuracy.
## Pull Request Checklist
- [ ] The Certbot team has recently expressed interest in reviewing a PR
for this. If not, this PR may be closed due our limited resources and
need to prioritize how we spend them.
- [ ] If the change being made is to a [distributed
component](https://certbot.eff.org/docs/contributing.html#code-components-and-layout),
add a description of your change to the `newsfragments` directory. This
should be a file called `<title>.<type>`, where `<title>` is either a
GitHub issue number or some other unique name starting with `+`, and
`<type>` is either `changed`, `fixed`, or `added`.
* For example, if you fixed a bug for issue number 42, create a file
called `42.fixed` and put a description of your change in that file.
- [ ] Add or update any documentation as needed to support the changes
in this PR.
- [x] Include your name in `AUTHORS.md` if you like.
## Pull Request Checklist
- [ ] The Certbot team has recently expressed interest in reviewing a PR
for this. If not, this PR may be closed due our limited resources and
need to prioritize how we spend them.
- [ ] If the change being made is to a [distributed
component](https://certbot.eff.org/docs/contributing.html#code-components-and-layout),
add a description of your change to the `newsfragments` directory. This
should be a file called `<title>.<type>`, where `<title>` is either a
GitHub issue number or some other unique name starting with `+`, and
`<type>` is either `changed`, `fixed`, or `added`.
* For example, if you fixed a bug for issue number 42, create a file
called `42.fixed` and put a description of your change in that file.
- [ ] Add or update any documentation as needed to support the changes
in this PR.
- [ ] Include your name in `AUTHORS.md` if you like.
