With more and more of our wildcard instructions on https://certbot.eff.org telling people to use these plugins, I think we should get ready to move our DNS plugins to the stable channel. This PR removes grade: devel so the snap store doesn't prevent us from doing that when we want to. See #8128 where we did this to the Certbot snap for more info.
You can see the snap tests passing with this change at https://dev.azure.com/certbot/certbot/_build/results?buildId=2797&view=results.
Fixes https://github.com/certbot/certbot/issues/8292.
This uses the same approach that worked well for us in https://github.com/certbot/certbot/pull/7926. I'm sure we could delete more code or refactor things here, but I think we should make the most conservative changes we can to certbot-auto until we can just delete the entire thing.
I ran the full test suite on these changes at https://dev.azure.com/certbot/certbot/_build/results?buildId=2773&view=results and manually tested things on OpenSUSE and it worked as expected. certbot-auto refused to create new installations and refused to update old ones while continuing to allow the old version of Certbot to run.
* Deprecate cb-auto outside of Debian and RHEL.
* Don't deprecate Amazon Linux yet.
This reverts commit feca125437.
Since this change landed, ARM builds for many of the DNS plugins have failed every night. See https://dev.azure.com/certbot/certbot/_build?definitionId=5 or our public Mattermost channel.
I quickly tried to fix this myself and wasn't trivially able to do so. I tried setting `SNAPCRAFT_PYTHON_VENV_ARGS: --system-site-packages` and adding `python3-wheel` as a build dependency, but it didn't work for some reason. The `python3-wheel` package didn't seem to be installed.
I still suspect something like this is the approach we should take, however, I want to fix the failing tests now so things are no longer broken in `master` and those of us on the Certbot team at EFF stop getting spammed with 54 (!!) emails about failed builds from launchpad every night.
Unfortunately, while I was working on this the queue for ARM machines on Launchpad jumped up to an estimated ~20 hour wait, but I confirmed that this fixes the problem by building on an ARM AMI using the instructions at https://github.com/certbot/certbot/blob/master/tools/snap/README.md#use-testing-and-development. If whoever reviews this would like an ARM machine to test on themselves, please let me know.
Fixes https://github.com/certbot/certbot/issues/8162.
I had to update the base of the Dockerfile to get a new enough version of Python 3. I also simplified things a lot and removed a lot of the comments that were essentially just describing how Dockerfiles work.
The most complicated changes here are in `testdata`. You can find a diff of the changes to `nginx.tar.gz` at https://gist.github.com/c7727db0cecf3f15f02439f085c73848.
The first problem was that there were some complaints from the new Apache/nginx/OpenSSL version about the 1024 bit RSA key so I updated `empty_cert.pem` both inside and outside of the tarball as well as the corresponding private key in the tarball to use a 2048 bit key.
The 2nd problem is trickier to understand. If you look at the output from nginx after loading the config from `lots/` you'll see it complaining about conflicting `server_name` directives for the directives I deleted. See https://dev.azure.com/certbot/certbot/_build/results?buildId=2578&view=logs&j=250aa146-b243-5f8f-bf86-17a529c9fb7e&t=9baa2014-9673-5e78-8f4f-7a463caf2bfa&l=1516.
After switching the tests to Python 3, tests on that domain started failing. What I believe to be happening is we were just lucky these tests were passing to begin with. In both the Apache and Nginx plugin, if there are conflicting virtual hosts like this, we just arbitrarily pick one. The relevant code here for nginx is https://github.com/certbot/certbot/blob/575092d6030330ed8379babaa4cbbfe43e7bf721/certbot-nginx/certbot_nginx/_internal/configurator.py#L455
I played around with a debugger and confirmed that before I removed the conflicting server names, there were two exact matches for the domain we were searching for here.
I think all that's going on is with the switch to Python 3, the vhost we happen to choose changes and "breaks" the test. I suspect this to be due to something like getting values out of a dict somewhere where the order of items in a dict while iterating over it is different between Python 2 and 3. I didn't track where this difference happens down, but I personally don't think it's a good use of time since I think the real problem here is that the nginx config being tested was invalid with conflicting `server` blocks.
I removed all references to the `server_name` causing conflicts in that nginx configuration because both server blocks had other domains that are being tested, but I could add either back if you prefer. You can see the `nginx_compat` test passing with these changes at https://dev.azure.com/certbot/certbot/_build/results?buildId=2587&view=logs&j=250aa146-b243-5f8f-bf86-17a529c9fb7e.
* update Dockerfile
* Fix apache_compat on py3.
* Update empty_cert.pem.
The command used here was `openssl req -key
certbot/certbot/tests/testdata/rsa2048_key.pem -new -subj '/CN=example.com'
-x509 >
certbot-compatibility-test/certbot_compatibility_test/testdata/empty_cert.pem`.
* update nginx.tar.gz
* Remove conflicting server_names
I'm adding this comment as part of the resolution of #8251. I think rewriting the script in Python is something we really should only worry about if we're working on the script in the future. Because of this, I personally prefer a code comment rather than an issue here.
Fixes https://github.com/certbot/certbot/issues/8165.
I moved `prerequisites` up to the "Running a local copy of the client" `contributing.html#prerequisites` still links to information about installing Cerbot's dependencies.
I left all certbot-auto documentation that wasn't explicitly encouraging its use. I think we can rip that out once the script is deprecated.
Fixes https://github.com/certbot/certbot/issues/8022, https://github.com/certbot-docker/certbot-docker/issues/25, and https://github.com/certbot-docker/certbot-docker/issues/20.
This PR builds on https://github.com/certbot/certbot/pull/8192 to set up similar builds in Azure to what we currently have at release time as well as nightly builds allowing us to catch problems in these images before a release. It also fully automates our Docker deployments removing a manual step from our release process. We'll need to update our release instructions once this PR lands.
If you're not familiar with our `certbot-docker` setup, you can read about how these scripts customized the build process on Docker Hub at https://docs.docker.com/docker-hub/builds/advanced/.
You can see the process working properly at:
* Nightly build on my fork: https://dev.azure.com/bmw0523/bmw/_build/results?buildId=345&view=logs&j=70ac378a-cb1f-50d1-b328-169807afbcfa
* Release build on my fork: https://dev.azure.com/bmw0523/bmw/_build/results?buildId=346&view=logs&j=70ac378a-cb1f-50d1-b328-169807afbcfa
* Nightly build on Certbot's Azure setup: https://dev.azure.com/certbot/certbot/_build/results?buildId=2426&view=logs&j=70ac378a-cb1f-50d1-b328-169807afbcfa
The builds on my fork pushed to https://hub.docker.com/u/certbottest. The credentials for this account are in our shared vault in 1password if you want to play around with this.
While the scripts will (almost?) always be run in CI, I tested the scripts successfully on macOS, Ubuntu 18.04, and Ubuntu 20.04, however, **the scripts do not seem to work when using the Docker snap, at least on Ubuntu 20.04.** It does work with the `docker.io` packages from `apt`. I was able to make things work by no longer setting `DOCKER_BUILDKIT`, but as I described in the code comments, this breaks things on Azure.
When writing this PR, I tried to make the minimal modifications to our current set up to get the behavior we want. I'm planning on working on splitting the Docker builds into different Azure jobs so it doesn't increase the overall build time, but this isn't trivial so I figured it would be best done in a separate PR.
* Remove license.
* update build scripts
* write deploy code
* Remove unused READMEs.
* rewrite readme
* Make testing on a fork easier.
* Set up Azure automation.
* fix typo
* Make output more verbose.
* clean up cleanup...everybody everywhere
* separate build and deploy
* Document docker-hub credentials
* Use Docker BuildKit when building.
* Remove unneeded .gitignore files.
* Fix tools/docker/README.md grammar
Co-authored-by: ohemorange <ebportnoy@gmail.com>
* Clarify <TAG> in README.
* no docker snap
* rename docker job
Co-authored-by: Erica Portnoy <ebportnoy@gmail.com>
If you go to a URL like https://snapcraft.io/certbot/releases and try to move the Certbot snap into the candidate or stable channels, you cannot do so. There is a tooltip which says that revisions with the grade devel cannot be promoted to candidate or stable channels.
The documentation for `grade` can be found at https://snapcraft.io/docs/snapcraft-yaml-reference where it says the value is optional and
> Defines the quality grade of the snap.
Type: enum
Can be either devel (i.e. a development version of the snap, so not to be published to the stable or candidate channels) or stable (i.e. a stable release or release candidate, which can be released to all channels)
Example: [stable or devel]
I'm working on a proposal for our next steps for snaps which involves moving the Certbot snap to the stable channel. I of course won't make those changes without giving others a chance to share their opinion, but I'd like to avoid the situation where we're technically unable to move the Certbot 1.6.0 snap to the stable channel despite wanting to do so.
I started to make the same changes to the DNS plugins, but I personally think it's too soon to propose stable versions of those yet and `grade` is a simple way to ensure we don't accidentally promote something there.
You can see the snap being built and run successfully with this change at https://dev.azure.com/certbot/certbot/_build/results?buildId=2246&view=results.
This line seems to refer to [augeas.py](https://github.com/hercules-team/python-augeas/blob/v0.5.0/augeas.py) from the version of Augeas we normally have pinned. This was necessary when we were installing each Certbot component in separate parts and only combining them later to ensure that the Augeas fork (which uses cffi) was used instead of the unmodified pinned version of Augeas.
Since everything is installed in one part and we're removing the Augeas pinning now though, this line is no longer necessary. You can see the snap being built and tested successfully with this change at https://travis-ci.com/github/certbot/certbot/builds/172134518.
* Do not unstage generated cffi augeas
* Add comment about deleted line.
I initially added this when the script was doing things like migrating all LXD containers to the snap. I think the external side effects are now pretty minimal thought so I think we can remove the need for this environment variable which makes it easier to use outside of CI for manual testing.
* Add the content interface to Certbot
This commit contains a subset of the changes from 7076a55fd82116d068e2aca7239209b7203917d2.
* Normalise slot parameters
(cherry picked from commit 810941979bcf609c1e0be18e9263abf046b90e82)
Co-authored-by: Robie Basak <robie.basak@canonical.com>
The error message from `python3 -m venv` when you don't have `python3-venv` installed is pretty good, but lets skip the failure and make sure it is installed the first time.
`tox -e cover` fails for me on macOS. This is due to the differences in the code that is run when on Linux vs. other platforms in `certbot.util` and its tests. Diffing my local coverage with Travis, the only difference in lines missing test coverage is:
```
$ diff travis.txt local.txt
< certbot/certbot/util.py 238 24 90% 132-134, 210, 275-281, 318, 330-340, 347, 381-382
> certbot/certbot/util.py 239 34 86% 30, 132-134, 210, 275-281, 301, 305, 316-318, 330-340, 347, 367-373, 381-382
< certbot/tests/util_test.py 392 0 100%
> certbot/tests/util_test.py 375 26 93% 483-487, 492-502, 507-514, 548-550, 555-557
```
I think tests on `master` should not be failing locally for people.
While there would be other ways to fix this by adding `# pragma: no cover` lines or writing mocked out tests for other platforms, I personally just think dropping the minimum coverage one percentage point is fine at least for now.
Coming out of the conversation at #7863 in the linked Google Doc, we should always have at least 1 release between updating one of our plugins to stop using a deprecated acme/certbot API and removing it from acme/certbot. Doing this gives the plugin changes time to propagate rather than potentially having the plugin break because Certbot was updated before the plugin had made the necessary changes.
This comment here should help ensure this.
* Add pytest warnings warning.
* clarify comment
Despite this PR (only) being ~200 lines containing mostly code copied from another repo, there is a lot going on here. For the sake of making it both easier to review and to remember some of these things in the future by referring back to this PR, I've documented a lot of noteworthy things with section headers below. With that said, it's probably not necessary to read each section unless you're interested in that topic.
The most noteworthy thing for the reviewer is **this PR should be merged and not squashed** to preserve authorship. To merge this code, once we're happy with this PR, I'll probably open a new PR squashing any commits I make in response in review comments back into a single commit to try to keep history somewhat clean. To help prevent this PR from being accidentally squashed, I'm making this a draft PR for now.
### Git history of https://github.com/basak/certbot-snap-build
I think it is worth preserving the git history of https://github.com/basak/certbot-snap-build that this PR is based on in this repo to help us track why things were done a certain way. To do this while keeping our git history somewhat clean, I took the approach described at https://stackoverflow.com/questions/1425892/how-do-you-merge-two-git-repositories/21495718#21495718 to move all history of https://github.com/basak/certbot-snap-build into a `snap` directory. I then squashed all commits so that sequential commits from the same author are one commit. I probably could have reordered commits to try and squash things a little more, but I personally don't think it's worth the trouble. Finally, I merged this rewritten history into this branch of the Certbot repo.
The contents of the `snap` directory are identical to the current contents of https://github.com/basak/certbot-snap-build before my final commit in this PR which makes the changes to make things work in this repo.
### Travis stages
This is described in general at https://docs.travis-ci.com/user/build-stages/, but I don't think we should deploy the snap if any of our tests are failing. To accomplish this, I created a "Snap" stage that builds, tests, and deploys the snap which is only executed after a "Test" stage that contains all of our other tests. The "Snap" stage will not run until the "Test" stage completes successfully.
### snap/local
This directory is ignored by `snapcraft` which I think makes it a good place to store `snap` specific scripts like `build_and_install.sh`.
See https://bugs.launchpad.net/snapcraft/+bug/1792203 for more info.
### Why remove certbot-compatibility-test from apacheconftest toxenvs?
Because it's not used. In theory, it could go in its own PR, but it'll create merge conflicts with this one so I'd personally prefer to include this simple change in this PR as well.
### Checklist for landing this PR
- [x] Squash all of my commits into one commit
- [x] Update the release instructions to have to move the snap to the beta channel
- [x] Shut down Robie's nightly builds probably by updating his repo to say that the code has moved here and deleting everything
In #7925 we accidently changed the logic here. Before it was:
type = cron OR (type = push AND branch NOT IN (apache-parser-v2, master))
now it's
type = cron OR (type = push AND branch = master)
We want to be able to run our full test suite on things like test-* branches. The reason we had been excluding master is it has the full test suite run on it (through what Travis calls cron) nightly and not running on every commit helps prevent us on waiting on CI since our nightly tests spin up so many jobs.
This PR changes things back to the intended behavior.
(We could talk about changing the condition to just type = cron OR type = push if we want, but I'd rather do that in a separate PR once things like test- branches are fixed.)
Python 2 is going to get harder and harder to install locally so I don't think we should assume/require devs to have it installed.
This PR builds on #7905 so our developer guide only has people use Python 3.
This PR fixes the Travis failures that can be seen https://travis-ci.com/certbot/certbot/builds/160258644. Running the tests locally, it looks like Ubuntu has started shutting down the 19.04 repos which makes sense as this release has been EOL'd. See https://wiki.ubuntu.com/Releases.
I have the full suite including the test farm tests running at https://travis-ci.com/github/certbot/certbot/builds/160269969 with this change.
The issue of adding 19.10 to our test farm tests is tracked by #7851. I think that issue is important and it's in our current milestone, but I'd personally rather get our tests passing for now and try to expand them to run on other systems later.
This PR builds on #7657 and cleans up additional unnecessary pylint comments and some stray comments referring to pylint: disable comments that have been deleted that I didn't notice in my review of that PR.
* Remove stray pylint link.
* Cleanup more pylint comments
* Cleanup magic_typing imports
* Remove unneeded pylint: enable comments
After getting a +1 from everyone on the team, this PR removes the use of `codecov` from the Certbot repo because we keep having problems with it.
Two noteworthy things about this PR are:
1. I left the text at https://github.com/certbot/certbot/blob/4ea98d830bcc3d1b980a4055243c6a6a25d8dc54/.azure-pipelines/INSTALL.md#add-a-secret-variable-to-a-pipeline-like-codecov_token because I think it's useful to document how to set up a secret variable in general.
2. I'm not sure what the text "Option -e makes sure we fail fast and don't submit to codecov." in `tox.cover.py` refers to but it seems incorrect since `-e` isn't accepted or used by the script so I just deleted the line.
As part of this, I said I'd open an issue to track setting up coveralls (which seems to be the only real alternative to codecov) which is at https://github.com/certbot/certbot/issues/7810.
With my change, failure output looks something like:
```
$ tox -e py27-cover
...
Name Stmts Miss Cover Missing
------------------------------------------------------------------------------------------
certbot/certbot/__init__.py 1 0 100%
certbot/certbot/_internal/__init__.py 0 0 100%
certbot/certbot/_internal/account.py 191 4 98% 62-63, 206, 337
...
certbot/tests/storage_test.py 530 0 100%
certbot/tests/util_test.py 374 29 92% 211-213, 480-484, 489-499, 504-511, 545-547, 552-554
------------------------------------------------------------------------------------------
TOTAL 14451 647 96%
Command '['/path/to/certbot/dir/.tox/py27-cover/bin/python', '-m', 'coverage', 'report', '--fail-under', '100', '--include', 'certbot/*', '--show-missing']' returned non-zero exit status 2
Test coverage on certbot did not meet threshold of 100%.
ERROR: InvocationError for command /Users/bmw/Development/certbot/certbot/.tox/py27-cover/bin/python tox.cover.py (exited with code 1)
_________________________________________________________________________________________________________________________________________________________ summary _________________________________________________________________________________________________________________________________________________________
ERROR: py27-cover: commands failed
```
I printed the exception just so we're not throwing away information.
I think it's also possible we fail for a reason other than the threshold not meeting the percentage, but I've personally never seen this, `coverage report` output is not being captured so hopefully that would inform devs if something else is going on, and saying something like "Test coverage probably did not..." seems like overkill to me personally.
* remove codecov
* remove unused variable group
* remove codecov.yml
* Improve tox.cover.py failure output.
Related to https://github.com/certbot/certbot/pull/7482, this removes some references to deprecated options in Certbot.
The only references I didn't remove were:
* In `certbot/tests/testdata/sample-renewal*` which contains a lot of old values and I think there's even some value in keeping them so we know if we make a change that suddenly causes old renewal configuration files to error.
* In the Apache and Nginx plugins and I created https://github.com/certbot/certbot/issues/7508 to resolve that issue.
If you run `mypy --platform darwin certbot/certbot/util.py` you'll get:
```
certbot/certbot/util.py:303: error: Name 'distro' is not defined
certbot/certbot/util.py:319: error: Name 'distro' is not defined
certbot/certbot/util.py:369: error: Name 'distro' is not defined
```
This is because mypy's logic for handling platform specific code is pretty simple and can't figure out what we're doing with `_USE_DISTRO` here. See https://mypy.readthedocs.io/en/stable/common_issues.html#python-version-and-system-platform-checks for more info.
Setting `_USE_DISTRO` to the result of `sys.platform.startswith('linux')` solves the problem without changing the overall behavior of our code here though.
This fixes part of https://github.com/certbot/certbot/issues/7803, but there's more work to be done on Windows.
I don't fully understand why, but since I updated my macbook to macOS Catalina, the test script currently fails to run for me with the versions of our dependencies we have pinned. Updating the dependencies solves the problem though and you can see Travis also successfully running tests with these new dependencies at https://travis-ci.com/certbot/certbot/builds/150573696.
I want to do what I did in https://github.com/certbot/certbot/pull/7733 to our Azure Pipelines setup, but unfortunately this isn't currently possible. The only filters available for service hooks for the "build completed" trigger are the pipeline and build status. See
To accomplish this, I propose splitting the "advanced" pipeline into two cases. One is for builds on protected branches where we want to be notified if they fail while the other is just used to manually run tests on certain branches.
This fixes (part of) the problem identified in https://github.com/certbot/certbot/pull/7657#issuecomment-586506340.
When I tested our pylint setup on Python 3.5.9, 3.6.9, or 3.6.10, tests failed with:
```
************* Module acme.challenges
acme/acme/challenges.py:57:15: E1101: Instance of 'UnrecognizedChallenge' has no 'jobj' member (no-member)
************* Module acme.jws
acme/acme/jws.py:28:16: E1101: Class 'Signature' has no '_orig_slots' member (no-member)
```
These errors did not occur for me on Python 3.6.7 or Python 3.7+.
You also cannot run our lint setup on Python 2.7 because our pinned version of pylint's dependency `asteroid` does not support Python 2. Because of this, `pylint` is not installed in the virtual environment created by `tools/venv.py` and our [`lint` environment in tox specifies that Python 3 should be used](https://github.com/certbot/certbot/blob/fd64c8c33b2176e6569d64d30776bd5fc9fd3820/tox.ini#L132).
I tried updating pylint and its dependencies to fix the problem, but they still occur so I think adding back these disable checks on these lines again is the best fix for now.
