Fixes https://github.com/certbot/certbot/issues/10662
Launchpad is failing, so this should make testing this code pretty easy.
There is only one log per target, no matter how many arches, so we can
skip the per-arch code.
---------
Co-authored-by: Will Greenberg <willg@eff.org>
## Description
removed trailing whitespace on 2 line(s) in `CONTRIBUTING.md`
## Type of change
- [x] Documentation improvement
## Checklist
- [x] I have read the CONTRIBUTING guide
- [x] My changes follow the existing style
Co-authored-by: Mulky Malikul Adhr <mulkymalikuldhrs@users.noreply.github.com>
Item 1 of https://github.com/certbot/certbot/issues/10600
Once this is merged, the [release
instructions](https://github.com/EFForg/certbot-misc/wiki/The-Mystical-Release-Process)
should be updated to no longer say "Make sure Certbot's virtual
environment isn't activated."
Why in `release.sh` instead of `_release.sh`? This seemed to be the
"check the environment status" file.
`venv/bin/activate` does several things.
1. create `deactivate` shell function
2. create `_OLD_VIRTUAL_PATH` and modify `PATH` to prepend `venv/bin`
location. `_OLD_VIRTUAL_PATH` isn't exported.
3. unset `PYTHONHOME` and store the old `PYTHONHOME` in
`_OLD_VIRTUAL_PYTHONHOME`, again not exported.
4. export `VIRTUAL_ENV_PROMPT`
5. call `hash -r 2> /dev/null` for some sort of edge case
6. set `VIRTUAL_ENV`
7. change the prompt appearance (PS1)
1, 4, and 7 don't need to be undone. 2, 4, and 6 are managed here
manually.
3 is the hard one, since we don't have access to
`_OLD_VIRTUAL_PYTHONHOME`, and there's not a great way of grabbing it
from the shell. One thought I had was to modify `venv/bin/activate` in
`venv.py` so that it is exported. That's possible, but at least for me,
`PYTHONHOME` isn't set in the first place and so it doesn't seem worth
doing that. Given that this script only needs to run on a few people's
machines, I would say we can hold off on doing that if and until it
becomes necessary.
Added some prints and an `exit 0` after the relevant code in the script
to test:
```bash
$ RELEASE_GPG_KEY=dontmatter tools/release.sh 1.2.3 4.5.6
$ source venv/bin/activate
(venv) $ RELEASE_GPG_KEY=dontmatter tools/release.sh 1.2.3 4.5.6
Deactivating venv...
previous path:
/Users/erica/certbot/venv/bin:/opt/homebrew/opt/coreutils/libexec/gnubin:[rest of path omitted]
new path:
/opt/homebrew/opt/coreutils/libexec/gnubin:[rest of path omitted]
(venv) $ printenv PATH
/Users/erica/certbot/venv/bin:/opt/homebrew/opt/coreutils/libexec/gnubin:[rest of path omitted]
```
---------
Co-authored-by: Brad Warren <bmw@users.noreply.github.com>
There's no way anyone is using these...
https://github.com/certbot/certbot/pull/9762 was just basing off of
readthedocs' defaults.https://github.com/certbot/certbot/pull/9762 was
just basing off of readthedocs' defaults.
Probably no one is using pdf either, but definitely no one is using
epubs. This is the more cautious version of
https://github.com/certbot/certbot/pull/10670
```
$ git grep epub
acme/docs/Makefile:.PHONY: help clean html dirhtml singlehtml pickle json htmlhelp qthelp devhelp epub latex latexpdf text man changes linkcheck doctest coverage gettext
acme/docs/Makefile: @echo " epub to make an epub"
acme/docs/Makefile:epub:
acme/docs/Makefile: $(SPHINXBUILD) -b epub $(ALLSPHINXOPTS) $(BUILDDIR)/epub
acme/docs/Makefile: @echo "Build finished. The epub file is in $(BUILDDIR)/epub."
acme/docs/make.bat: echo. epub to make an epub
acme/docs/make.bat:if "%1" == "epub" (
acme/docs/make.bat: %SPHINXBUILD% -b epub %ALLSPHINXOPTS% %BUILDDIR%/epub
acme/docs/make.bat: echo.Build finished. The epub file is in %BUILDDIR%/epub.
certbot/docs/Makefile:.PHONY: help clean html dirhtml singlehtml pickle json htmlhelp qthelp devhelp epub latex latexpdf text man changes linkcheck doctest coverage gettext
certbot/docs/Makefile: @echo " epub to make an epub"
certbot/docs/Makefile:epub:
certbot/docs/Makefile: $(SPHINXBUILD) -b epub $(ALLSPHINXOPTS) $(BUILDDIR)/epub
certbot/docs/Makefile: @echo "Build finished. The epub file is in $(BUILDDIR)/epub."
certbot/docs/make.bat: echo. epub to make an epub
certbot/docs/make.bat:if "%1" == "epub" (
certbot/docs/make.bat: %SPHINXBUILD% -b epub %ALLSPHINXOPTS% %BUILDDIR%/epub
certbot/docs/make.bat: echo.Build finished. The epub file is in %BUILDDIR%/epub.
```
Related to https://github.com/certbot/certbot/issues/10581
Following up on https://github.com/certbot/certbot/pull/10631,
https://github.com/certbot/certbot/pull/10622, and
https://github.com/certbot/certbot/pull/10634, this PR converts the
release
[pipeline](https://dev.azure.com/certbot/certbot/_build?definitionId=3)
from Azure to Github Actions.
While this is the last migration PR, I don't think we should close the
issue, as we're still using launchpad for armhf builds. I plan to
continue investigating that and at minimum write up my findings.
To test
[notifications](https://opensource.eff.org/eff-open-source/pl/nfgh6obi8tfqikn4ydp7dshakr)
and creating a [github
release](https://github.com/ohemorange/Things-that-are-gr9/releases/tag/v1.0.19),
I ran workflows that no-oped most the other jobs [in a test
repo](https://github.com/ohemorange/Things-that-are-gr9/actions/runs/25938082721)
(I've made minor changes to names and comments since then, but no code
changes). Everything else is basically the same as nightly, with
different tags. For the docker deployment, `${{ github.ref_name }}` is
the tag name, so `v1.2.3`.
Why not parametrize the tests a bit more, by putting an `env` at the top
with `dockerTag: ${{ github.ref_name }}` and `snapReleaseChannel: beta`?
Because the `env` context is [not
available](https://docs.github.com/en/actions/reference/workflows-and-actions/contexts#context-availability)
to `with`; only `github, needs, strategy, matrix, inputs, vars`. We
could use `vars`, by creating `docker_tag_release` or whatever in the
[variable
section](https://github.com/certbot/certbot/settings/variables/actions)
of the repo settings by using the web interface, but that seems worse to
me than having it in the file but twice.
You will note that the contents of `release.yml` are very similar to
`nightly.yml`. While it would be nice to factor that out and reuse the
code, github actions would then flatten everything in the grouped code
together, making the results much harder to check. You can see what that
flattening would look like
[here](https://github.com/ohemorange/Things-that-are-gr9/actions/runs/25941524972)
(if we put them all in one workflow). Currently, it will look something
like
[this](https://github.com/certbot/certbot/actions/runs/25688414262),
which is much more readable.
We could split it into "stages" like we had in azure pipelines (probably
1. standard and extended tests (and changelog?), 2. snap package and
deploy (depend on tests), 3. docker package and deploy (depend on
tests), 4. github release (depend on snap package and deploy), 5-6.
notify (depend on github release)), but in addition to a minor slowdown
(currently github release only depends on snap and docker package, not
deploy, just so we're trying to do all the deployments simultaneously
and not partially in case of build failures), it would still be only
like three fewer jobs, since we'd still want all the info passing and
dependency relationships.
While it would be nice from a UX perspective to group the two
notification jobs together, you can't do that cleanly using the built-in
`if` key, and I don't think it's worth switching to a messier github
api-based version just to group them.
For mattermost notifications, we currently get the person to tag by
running `AUTHOR_NAME="$(git log -1 --pretty=format:'%an')"` and mapping
that to mattermost username. We could instead use `github.actor` to get
the github handle, and map that instead. I didn't bother since we
already have working, tested code, but can if a reviewer thinks it's
worth it.
---------
Co-authored-by: Will Greenberg <willg@eff.org>
Co-authored-by: Brad Warren <bmw@users.noreply.github.com>
Item 3 of https://github.com/certbot/certbot/issues/10600
If this is merged, the [release
instructions](https://github.com/EFForg/certbot-misc/wiki/The-Mystical-Release-Process)
should be updated to no longer manually create the branch.
I intentionally do not add error checking to `git switch -c` because I
think if the command fails, the script should fail, and we should
manually fix the error, which will probably be something like `fatal: a
branch named 'candidate-5.5.0' already exists` which is clear enough.
I do check if we're already on the intended branch, because we may want
to be able to rerun the release script without having to switch back to
main and delete the candidate branch each time.
I remove the check later on because I believe it is a holdover from a
previous version where it was possible for `RELEASE_BRANCH` to be not
equal to `candidate-$version`, but even in the existing code before the
other change, that should not be possible.
Item 5 of https://github.com/certbot/certbot/issues/10600
When this is merged, the setup section of the [release
process](https://github.com/EFForg/certbot-misc/wiki/The-Mystical-Release-Process)
should be modified.
- `gh` should be added to os packages for mac and debian
- A new step should be added: "Run `gh auth login` and log into a GitHub
account." Technically any account should work here.
The contents of the step saying to post to the community forum should be
shortened to say something like "copy the output from the terminal." We
could add the link to the client-dev tag here, but personally I think
it's easiest to just keep it in the release instructions.
These tests are failing because the runner migrated ownership to github,
and that somehow broke it even though it wasn't supposed to. They're not
responding to issues [other
than](https://github.com/actions/runner-images/issues/14100) "a CVE or
vulnerability" during the transition period (ends june 12). Let's just
turn these off for now. We weren't even running this at all in azure,
it's fine.
Fixes#10598.
## Pull Request Checklist
- [x] 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.
- [x] If you used AI to create this PR, you have done a self-review of
all AI-generated code and disclosed that your contribution was
AI-generated per [EFF's AI-generated contribution
policy](https://www.eff.org/about/opportunities/volunteer/coding-with-eff).
You assert you have thoroughly understood, reviewed, and tested your
entire submission.
- [x] 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.
- [x] Add or update any documentation as needed to support the changes
in this PR.
- [x] Include your name in `AUTHORS.md` if you like.
Fixes#10617
I restructured the conditionals to avoid too much nesting. This should
have the same effect, just with the additional check conditioned on all
the target snap files being available. Here is the logic I used for the
restructuring:
start
```python
dump_output = exit_code != 0 or failed_archs
if exit_code == 0 and not failed_archs:
# We expect to have all target snaps available, or something bad happened.
snaps_list = glob.glob(join(workspace, '*.snap'))
if not len(snaps_list) == len(archs):
print('Some of the expected snaps for a successful build are missing '
f'(current list: {snaps_list}).')
dump_output = True
else:
build_success = True
break
```
note that `(exit_code == 0 and not failed_archs) == not (exit_code != 0
or failed_archs) == not dump_output`
```python
dump_output = exit_code != 0 or failed_archs
if not dump_output:
# We expect to have all target snaps available, or something bad happened.
snaps_list = glob.glob(join(workspace, '*.snap'))
if not len(snaps_list) == len(archs):
print('Some of the expected snaps for a successful build are missing '
f'(current list: {snaps_list}).')
dump_output = True
else:
build_success = True
break
```
distribute the if
```python
dump_output = exit_code != 0 or failed_archs
snaps_list = glob.glob(join(workspace, '*.snap'))
if not dump_output and (not len(snaps_list) == len(archs)):
# We expect to have all target snaps available, or something bad happened.
print('Some of the expected snaps for a successful build are missing '
f'(current list: {snaps_list}).')
dump_output = True
if not dump_output and (len(snaps_list) == len(archs)): # redundant; if it were false, we would have changed dump_output right above this
build_success = True
break
```
remove redundant check
```python
dump_output = exit_code != 0 or failed_archs
snaps_list = glob.glob(join(workspace, '*.snap'))
if not dump_output and (not len(snaps_list) == len(archs)):
# We expect to have all target snaps available, or something bad happened.
print('Some of the expected snaps for a successful build are missing '
f'(current list: {snaps_list}).')
dump_output = True
if not dump_output:
build_success = True
break
```
As this shows, we can now add additional checks that only happen if we
think we're in danger of succeeding based on checks thus far, and simply
change `dump_output` to `True` if the additional check fails.
You can see the build step failing when the [file contains
html](https://github.com/certbot/certbot/compare/html-problem...refs/heads/test-html-problem-2)
at
https://github.com/certbot/certbot/actions/runs/26071811878/job/76654623490#step:5:42
Related to https://github.com/certbot/certbot/issues/10581
Following up on #10631 and
https://github.com/certbot/certbot/pull/10622, this PR converts the
`nightly`
[pipeline](https://dev.azure.com/certbot/certbot/_build?definitionId=5)
from Azure to Github Actions.
`schedule` and `workflow_dispatch` triggers only work on merged
branches, not PRs. To see these tests running, I temporarily added a
`push` trigger in commit
[a2e9c43](a2e9c4303e).
You can see the results of those tests
[here](https://github.com/certbot/certbot/actions/runs/25688414262).
I did not split each file into its own commit this time because I feel
like the general idea is clear. The relevant files in azure pipelines to
reference are:
- the deleted `.azure-pipelines/nightly.yml` -->
`.github/workflows/nightly.yml`
- `.azure-pipelines/templates/jobs/common-deploy-jobs.yml` -->
`.github/workflows/deploy_docker_images.yml` and
`.github/workflows/deploy_snaps.yml`
- `.azure-pipelines/templates/stages/changelog-stage.yml` -->
`.github/workflows/create_changelog.yml`
I chose to split `common-deploy-jobs` into `deploy_docker_images` and
`deploy_snaps`. This is because the docker arm32v6 build takes a long
time, but uploading to docker is quick, while the armhf snaps build
varies but is often quicker, but uploading the snaps can take some time.
By splitting them, we can specify the dependencies more precisely, and
hopefully shave some time off the total. Without the split, tests took
[53 minutes
total](https://github.com/certbot/certbot/actions/runs/25684264622).
After the split, tests took [33 minutes
total](https://github.com/certbot/certbot/actions/runs/25688414262)!
As before, the "nightly deploy stage" from azure has been omitted for
clarity.
`rerun.yml` did not exist before. There's not a great built-in way to
rerun individual jobs in github actions, which I wanted for the snap
builds specifically, since other timeouts can still happen. I could have
made an action or additional workflow and wrapped that in a script to
retry it, but I figured actually it's nicer to have the ability to rerun
anything. This is equivalent to clicking "rerun all failed jobs," which
I feel is usually what we want. Unfortunately, I am pretty sure that to
test it, the rerun script will need to be merged first, since it relies
on `workflow_dispatch`.
You can see that packages were successfully uploaded to
[dockerhub](https://hub.docker.com/r/certbot/certbot/tags) and the [snap
store](https://dashboard.snapcraft.io/stores/snaps/); looking at the
timestamps is probably the easiest way to confirm (about 11:45am
Monday).
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
