This PR is the second part of #6497 to ease the integration, following the new plan propose by @bmw here: #6497 (comment)
This PR creates the module certbot.compat.os, that delegates everything to os, and that will be the safeguard against problematic methods of the standard module. On top of that, a quality check wrapper is called in the lint tox environment. This wrapper calls pylint and ensures that standard os module is no used directly in the certbot codebase.
Finally local oldest requirements are updated to ensure that tests will take the new logic when running.
* Add executable permissions
* Add the delegate certbot.compat.os module, add check coding style to enforce usage of certbot.compat.os instead of standard os
* Load certbot.compat.os instead of os
* Move existing compat test
* Update local oldest requirements
* Import sys
* Update account_test.py
* Update os.py
* Update os.py
* Update local oldest requirements
* Implement the new linter_plugin
* Fix local oldest for nginx
* Remove check coding style
* Update linter_plugin.py
* Add several comments
* Update the setup.py
* Add documentation
* Update acme dependencies
* Update certbot/compat/os.py
* Update docs/contributing.rst
* Update linter_plugin.py
* Handle os.path. Simplify checker.
* Add a comment to a reference implementation
* Update changelog
* Fix module registering
* Update docs/contributing.rst
* Update config and changelog
This PR adds a step to Apache plugin config_test when run on Fedora. Because Fedora now creates self signed certificate and related key material upon first startup of httpd. This was causing issues for users who run certbot-auto or install certbot (and mod_ssl) and run Certbot directly after.
Fixes: #6828
* Try to restart httpd on Fedora if config check fails
* Update CHANGELOG.md
Following #6934, this PR finalize two things, as explained in #6934:
disable the default aggregated report
validate linux and windows reports against the PR base branch
So, we observed lately several inconsistencies in how Codecov behave toward the CI pipeline for PRs in Certbot. One example is #6888. The most annoying thing is that the build of PR is **temporary** marked as failed, until all coverage are run.
The correction on the latter is done in two PRs. This is the first part.
TL;DR
This PR separates the Codecov report in two: one for coverage executed on Windows, one for Linux. This is the correct way to do regarding our current CI pipeline. Actions are required by a GitHub administrator of Certbot once this PR is merged.
Complete explanation
So the failure stated in the introduction is essentially due to several things interacting together:
* AppVeyor generates a coverage report for Windows, that have a coverage value a little lower than on Linux (96%)
* Travis generates a coverage report for Linux. Its coverage is higher, and slowly decrease as more specific Windows code is added to Certbot, that cannot be tested on Travis
* Since AppVeyor saw its capacity increasing, it finishes its coverage job before the one from Travis
* Certbot GitHub repo is configured to require the coverage pipeline to succeed (in whatever that means) to success the overall PR build
So here the suite of events:
1) PR is issued. GitHub expect three pipeline to succeed: AppVeyor CI, Travis CI and Codecov (displayed in the PR page)
2) Codecov receive first the report of AppVeyor coverage. It is 96%. It is a failure for now, because coverage in master (AppVeyor+Travis) is 98.6%.
3) GitHub is reported of the failure on Codecov, so fail the PR build
4) Codecov receive then the report of Travis coverage. It is 98%. It merges it with the report from AppVeyor, leading to the 98.6%. The failure becomes a success.
5) GitHub is reported of the success on Codecov, so, nevermind, the PR build is a success finally!
So we have a CI flow that change its mind. Great. This is because of 2) and 4), and we could expect that Codecov should handle that. This is not the case: it is somewhat misleading, because Codecov adverts a lot about its capability to merge reports, including from different CI. But it is about the final state, not about the transient state, while reports are progressively received.
Two things to things that a transient state is existing, with a result that can change:
* first, from Codecov doc itself, explaining that reports should not be trusted during the CI pipeline execution: https://docs.codecov.io/docs/ci-service-relationship#section-checking-ci-status
* second, is an example of transient state of `cryptography` project, this is advert by Codecov to be a reference of the implementation:
As you can see above, build state of `cryptography` is failing after the first report is received, and until all coverage reports from Travis are received.
So, what can we do about it? Thing is, we are aggregating coverage from very two unrelated sources (two different OS systems), and Codecov has something for that. This is flags: https://docs.codecov.io/docs/flags
Flags allow to flag coverage material depending on any logic you apply to the command that uploaded the coverage report (eg. `codecov -F a_flag`). Then, several logics can be applied on it, for instance having in Codecov UI the capability to filter the coverage other a flag, having status of build for each flag and ... having a report for a specific flag.
So:
1) I modified Travis and AppVeyor to send their report under a specific flag: `linux` or `windows`
2) I created a project specific `.codecov.yml` configuration in Certbot repository, to instruct Codecov to push two separate reports on GitHub build: one for Linux, one for Windows. Each report can be validated against its specific coverage from the `master` branch (more on this just after)
With all of this, now the GitHub is succeeding, because each coverage is validated independently.
I think it is the good approach, because it solves the specific issue here, and because it reflects the logic behind: merging coverage from different OS architectures does not make much sense. It would be a long-term problem, because as I said at the beginning, coverages will slowly decrease as more platform specific code is added in Certbot.
Now, it is not finished. Two things need to be done: an administrator action, and a second PR
Administrator action
Certbot GitHub as a a branch protection rule (Settings > Branches > Branch protection rules). It needs to be changed.
Indeed this rule is expecting the full coverage report (named `codecov/project`) to be valid on a PR. It needs to be changed to expect two coverage reports: `codecov/project/linux` and `codecov/project/windows`. The `codecov/project` needs to be removed.
This can be done once this PR is merged, and the specific coverage reports have been generated on master.
Second PR
Once this PR is merged and administrative actions have been done. I will make a new PR modifying `.codecov.yml` with two things:
* disable the faulty full coverage report, that is not required anymore by GitHub branch protection rules
* modify the `linux` and `windows` reports to validate against the relevant coverage calculated from `master` (indeed, in this PR it is a fixed ratio rule, since the coverage to compare on master is the full coverage one, significantly higher)
* Tag reports
* Set per-project codecov configuration
Fixes#6861.
_venv_common.py is no longer executable. The reason for this is the venv creation logic is now different between Python 2 and Python 3. We could add code that branches on the Python version running the script, but I personally think that's unnecessary.
--setuptools and --no-site-packages is no longer passed to virtualenv either. These flags were made noops in virtualenv 1.10 and 1.7 respectively, but all of CentOS 6, 7, Debian 8+, and Ubuntu 14.04+ have new enough versions of virtualenv where these flags are no longer necessary. They are not even accepted as flags to Python 3's venv module.
Use of VENV_ARGS from test_sdists.sh was also removed because that environment variable hasn't done anything in a while.
I ran test farm tests on test_apache2.sh and test_sdists.sh with these changes and they passed.
* Fixes#6861.
* _venv_common is no longer executable.
When releasing 0.33.1 and resolving merge conflicts between the candidate-0.33.1 branch and master, I had merge conflicts in the local-oldest-requirements.txt files. This is because the point release branch does not contain modifications to these files that landed in master because it happens later in the release script in the commit bumping version numbers which is not included in the point release branch.
I think having to resolve these merge conflicts is unnecessary and even a slight problem because it means that the "oldest" tests on the point release branch may still be using the latest version of certain components when they actually should be using an older version.
I fixed this by moving this code earlier in the script so the local-oldest-requirements.txt files are updated at the same time as the setup.py files.
The changelog should still say <version> - master because it will be fixed up automatically by the release script at https://github.com/certbot/certbot/blob/master/tools/_release.sh#L69.
* Protect certbot-auto against non numerical version release in some RPM distributions (#6913)
Fixes#6912
Bash evaluate all condition in a predicate statement, eg. `"$SOMEVAR" = "test" -a "$ANOTHERVAR" = "test2"`, even if it is not necessary, for instance if the first condition is false in the example here.
As a consequence, on non-Fedora distributions, an evaluation of the distribution version could be done on non numeric value, eg. `"6.7" -eq "29"`, making certbot-auto failing in this case.
This PR fixes that, by evaluating the version on RPM distributions only if we are on Fedora. Otherwise, version will be "0".
(cherry picked from commit c2d9ea1f61)
* Update changelog about #6912 fix. (#6914)
(cherry picked from commit 30eafba997)
* cleanup changelog
Fixes#6912
Bash evaluate all condition in a predicate statement, eg. `"$SOMEVAR" = "test" -a "$ANOTHERVAR" = "test2"`, even if it is not necessary, for instance if the first condition is false in the example here.
As a consequence, on non-Fedora distributions, an evaluation of the distribution version could be done on non numeric value, eg. `"6.7" -eq "29"`, making certbot-auto failing in this case.
This PR fixes that, by evaluating the version on RPM distributions only if we are on Fedora. Otherwise, version will be "0".
Dependencies generated by the script introduced with #6839 were not including anymore the fix about enum34 for CentOS 6.
This PR reinserts this fix, and updates the script overrides to ensure that this fix will stay in next dependencies generation.
* Add the environment marker back. Ensure that it will stay by adding an override to dependencies generator.
* Add comments, for future fix
* Update letsencrypt-auto-source/rebuild_dependencies.py
Co-Authored-By: adferrand <adferrand@users.noreply.github.com>
* Update comment
In CentOS 6 default httpd configuration, the `LoadModule ssl_module ...` is handled in `conf.d/ssl.conf`. As the `VirtualHost` configuration files in `conf.d/` are loaded in alphabetical order, this means that all files that have `<IfModule mod_ssl.c>` and are loaded before `ssl.conf` are effectively ignored. This PR moves the `LoadModule ssl_module` to the main `httpd.conf` while leaving a conditional `LoadModule` directive in `ssl.conf`.
Features
- Reads the module configuration from `ssl.conf` in case some modifications to paths have been made by the user.
- Falls back to default paths if the directive doesn't exist.
- Moves the `LoadModule` directive in `ssl.conf` inside `<IfModule !mod_ssl.c>` to avoid printing warning messages of duplicate module loads.
- Adds `LoadModule ssl_module` inside of `<IfModule !mod_ssl.c>` to the top of the main `httpd.conf`.
- Ensures that these modifications are not made multiple times.
Fixes: #6606
* Fix CentOS6 installer issue
* Changelog entry
* Address review comments
* Do not enable mod_ssl if multiple different values were found
* Add test comment
* Address rest of the review comments
* Address review comments
* Better ifmodule argument checking
* Test fixes
* Make linter happy
* Raise an exception when differing LoadModule ssl_module statements are found
* If IfModule !mod_ssl.c with LoadModule ssl_module already exists in Augeas path, do not create new LoadModule directive
* Do not use deprecated assertion functions
* Address review comments
* Kick tests
* Revert "Kick tests"
This reverts commit 967bb574c2.
* Address review comments
* Add pydoc return value to create_ifmod
This PR is a part of the effort to remove the last broken unit tests in certbot codebase for Windows, as described in #6850.
It solves the problems associated to ErrorHandler in Windows (enlighted by tests errors) by ... wipping out the problem: no signal is handled by ErrorHandler on Windows. See the relevant inline comment in certbot.error_handler for explanation and sources.
This PR is the first part of #6497 to ease the integration, following the new plan propose by @bmw here: #6497 (comment)
This step 1 refactor existing certbot.compat module into certbot.compat.misc, without any logic changed. Package certbot.compat will host the new modules that constitute the security model for Windows.
* Create the certbot.compat package. Move logic in certbot.compat.misc
* Add doc
* Fix lint
* Correct mypy
* Update client.py
Add a new test to make sure that we are covering all the branches of get_virtual_hosts() regardless of the order that Augeas returns the found VirtualHost paths.
Fixes: #6813
* Add a test to ensure test coverage regardless of the order of returned vhosts
* Use deepcopy instead, and increase coverage requirement back to 100%
