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>
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>
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.
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
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>
on main if you run tools/pinning/current/repin.sh and run our unit
tests, they will fail due to new deprecation warnings from pyparsing.
the cause of these warnings is described at
https://github.com/pyparsing/pyparsing/blob/dc009668d8025b96522eb7503e2ef84c2be58843/docs/whats_new_in_3_0_0.rst?plain=1#L613-L708
this PR fixes these warnings and updates our minimum required pyparsing
version to 3.0 where the new naming convention is available. i ran our
full test suite on the first commit here and it passed
i don't think it's worth trying to keep compatibility with pyparsing<3
unless we get a request for us to do so which i really doubt we will
Fixes #10518.
`tools/pinning/current/repin.sh` is not run; only pytest version is
updated. This is because `pypinning` had a bunch of syntax changes that
seem simply but I believe should be in a separate PR, which I think
should be done after this to collect all repin changes.
As discussed further in #10518, these issues were caused by pytest's
internalization of pytest-subtest, which had several implementation
changes.
To fix these, we simply no longer use subtest in the failing tests. The
test in acme is now parametrized instead, and the tests in apache only
ever had a single parameter.
To use parametrization in the acme test, I converted `DNSTest` from
unittest to pytest style, which was pretty straightforward. The only
note there is that while it would be nice to make `ec_secp384r1_key` a
fixture, you [can't use fixtures in
parameters](https://github.com/pytest-dev/pytest/issues/349). You could
use requests, but that seemed less clear and messier, because then you'd
be checking the value of the parameter and only sometimes loading it.
Could also make it a global variable, but that didn't really seem
necessary, as it's only called twice. Happy to consider other options,
not strongly tied to this one, just seemed nicest to me.
this fixes the dependabot alerts those with access can see at
https://github.com/certbot/certbot/security/dependabot
i don't think those alerts are particularly relevant to us, but i think
it's good for us to update anyway
in https://github.com/canonical/snapcraft/pull/5720, snapcraft made a
change. `snapcraft status certbot` output changed from something like
this:
```
Track Arch Channel Version Revision Progress
latest amd64 stable 5.1.0 5057 -
candidate ↑ ↑ -
beta 5.2.1 5214 -
edge 5.2.0.dev0 5210 -
arm64 stable 5.1.0 5058 -
candidate ↑ ↑ -
beta 5.2.1 5215 -
edge 5.2.0.dev0 5211 -
armhf stable 5.1.0 5056 -
candidate ↑ ↑ -
beta 5.2.1 5213 -
edge 5.2.0.dev0 5212 -
```
to this:
```
Track Arch Channel Version Revision Progress
latest amd64 stable 5.1.0 5057 -
latest amd64 candidate ↑ ↑ -
latest amd64 beta 5.2.1 5214 -
latest amd64 edge 5.2.0.dev0 5210 -
latest arm64 stable 5.1.0 5058 -
latest arm64 candidate ↑ ↑ -
latest arm64 beta 5.2.1 5215 -
latest arm64 edge 5.2.0.dev0 5211 -
latest armhf stable 5.1.0 5056 -
latest armhf candidate ↑ ↑ -
latest armhf beta 5.2.1 5213 -
latest armhf edge 5.2.0.dev0 5212 -
```
when its output is captured like it is in finish_release.py in the lines
above the code i'm modifying here
not matching on the beginning of lines makes this pattern a little less
strict, but based on the rest of the pattern and the output here, i
personally think this is fine
after carefully verifying this works with the current state of things, i
went ahead and finished the release with this change and it worked just
fine. instead, this PR proposes a way to fix things going forward
It's a [drop-in
replacement](https://docs.astral.sh/uv/pip/compatibility/) that speeds
things up. I don't see any reason why not.
`--use-pep517` is [set by default](
https://docs.astral.sh/uv/pip/compatibility/#pep-517-build-isolation),
so we don't need it.
`--disable-pip-version-check` also does nothing on uv.
`uv` [uses
separate](https://docs.astral.sh/uv/pip/compatibility/#build-constraints)
`UV_BUILD_CONSTRAINT` and `UV_CONSTRAINT`. I just added it to both to do
the simplest thing here. We could split them.
We probably don't actually need to pipstrap pip anymore, I could take
that out.
What's happening with `parsedatetime` and `python-digitalocean` is that
they were always secretly wrong. Since `pip` compiles bytecode by
default, it was suppressing the errors. If you add the
`--compile-bytecode` flag to `uv`, it passes, but I don't think we
should do that. You can see the failure happen on main by passing
`--no-compile` to the pip args and running `certbot -r -e oldest`.
Now what I don't understand is that some places seem to say the `'\/'`
error from `parsedatetime` only started in python 3.12, whereas others
see it on earlier python. Perhaps pytest is vendorizing python or
something. Not too worried about that, needed to get updated anyway, and
it's an accurate oldest version based on our oldest OSes.
`python-digitalocean` is techincally newer than debian 11, but we've
made that decision before so it seems fine to me.
Part of https://github.com/certbot/certbot/issues/10403
We were never actually updating the versions in certbot-ci and letstest.
Not that it really matters, but let's do that there as well.
Final part of https://github.com/certbot/certbot/issues/10403
I tested running `tools/snap/generate_dnsplugins_snapcraft.sh
certbot-dns-dnsimple` and it put the correct description in to the
`snapcraft.yaml` file.
this is in response to
https://github.com/certbot/certbot/pull/10399#issuecomment-3166305086
this PR does two things:
1. it clarifies what is meant by "build dependencies" in DESIGN.md
2. fixes our workaround for
https://github.com/python-poetry/poetry/issues/4103 which broke when we
moved most of our code under `src` directories. i kept the previous `rm
-rf ${REPO_ROOT}/*/*.egg-info` line around for `letstest` and to
hopefully add some robustness for us if we ever move our code around
again
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.
blast from the past! resurrects
https://github.com/certbot/certbot/pull/9803 with all of @bmw's changes.
i figured instead of force-pushing a basically brand new branch and
obliterating the old review, i'd just start from a clean slate
fixes#8272
---------
Co-authored-by: Brad Warren <bmw@users.noreply.github.com>
Co-authored-by: Brad Warren <bmw@eff.org>
Another token of gratitude for a super useful tool and service.
More about codespell: https://github.com/codespell-project/codespell .
I personally introduced it to dozens if not hundreds of projects already
and so far only positive feedback.
CI workflow has 'permissions' set only to 'read' so also should be safe.
---------
Signed-off-by: Yaroslav O. Halchenko <debian@onerussian.com>
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
This is a stopgap measure until we upgrade to the newer (but
backwards-incompatible) versions of cloudflare's python library (see
#9938)
---------
Co-authored-by: Brad Warren <bmw@users.noreply.github.com>
ohemorange