Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ACME: add support for IP identifiers #53660

Merged
merged 11 commits into from
Mar 13, 2019

Conversation

felixfontein
Copy link
Contributor

SUMMARY

According to https://tools.ietf.org/html/draft-ietf-acme-ip-05. First interoperability tests with letsencrypt/pebble#221 are looking good. This is essentially the acme_certificate part of #53228.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

acme_certificate

@ansibot
Copy link
Contributor

ansibot commented Mar 11, 2019

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 community_review In order to be merged, this PR must follow the community review workflow. crypto Crypto community (ACME, openssl, letsencrypt) feature This issue/PR relates to a feature request. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. support:community This issue/PR relates to code supported by the Ansible community. labels Mar 11, 2019
@ansibot

This comment has been minimized.

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Mar 11, 2019
@mattclay mattclay added the ci_verified Changes made in this PR are causing tests to fail. label Mar 12, 2019
@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Mar 12, 2019
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests. and removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Mar 12, 2019
@ansibot ansibot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. and removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Mar 12, 2019
@ansibot
Copy link
Contributor

ansibot commented Mar 12, 2019

The test ansible-test sanity --test pylint [explain] failed with 3 errors:

test/units/module_utils/test_acme.py:207:44: trailing-whitespace Trailing whitespace
test/units/module_utils/test_acme.py:261:16: trailing-whitespace Trailing whitespace
test/units/module_utils/test_acme.py:304:44: trailing-whitespace Trailing whitespace

click here for bot help

@ansibot ansibot added ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Mar 12, 2019
@felixfontein
Copy link
Contributor Author

@mattclay I want to preserve the trailing whitespaces in the OpenSSL outputs which are part of the unit tests. I've tried to skip the pep8 tests for this unit tests (df7e282d87f5cdf0d01cbe65492e2412ea7ca470), but this apparently didn't work. Do you know what went wrong? I could also do this by adding """ + r""" at the end of the line. Is this maybe a better approach (because it also will stop random editor settings to cut them off)?

@mattclay
Copy link
Member

@felixfontein Definitely don't skip the pep8 checks. For large static content like that, I'd put it in a separate file and load it during the test instead of trying to embed it in the code.

@felixfontein
Copy link
Contributor Author

@mattclay I've rearranged it as follows: a27afa7 Is the folder structure ok? (I.e. will the CI system find the tests if lib/ansible/module_utils/acme.py is changed?)

@mattclay
Copy link
Member

@felixfontein Yes, the folder structure is correct. The unit tests to run will be determined based on import analysis, so there's no issue there. It's actually more important that the fixtures are in the correct location so changes to them will run the tests. Those are OK as well.

You can check with the following commands:

ansible-test units --changed-path test/units/module_utils/acme/fixtures/cert_1.pem -e
ansible-test units --changed-path lib/ansible/module_utils/acme.py -e

Those will show what tests will run based on files changing.

@felixfontein
Copy link
Contributor Author

@mattclay great, thanks a lot!

@ansibot ansibot added botmeta This PR modifies the BOTMETA.yml and this requires special attention! core_review In order to be merged, this PR must follow the core review workflow. and removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Mar 12, 2019
@felixfontein
Copy link
Contributor Author

ready_for_review

Copy link
Contributor

@resmo resmo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't see anything to block this from getting in. Good job.

shipit

@ansibot ansibot added shipit This PR is ready to be merged by Core and removed core_review In order to be merged, this PR must follow the core review workflow. labels Mar 12, 2019
@resmo resmo merged commit c2cb82e into ansible:devel Mar 13, 2019
@felixfontein
Copy link
Contributor Author

@resmo thanks for reviewing and merging!
@mattclay thanks for helping with the tests!

@felixfontein felixfontein deleted the acme-ip-identifiers branch March 13, 2019 09:25
@ansible ansible locked and limited conversation to collaborators Jul 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.8 This issue/PR affects Ansible v2.8 botmeta This PR modifies the BOTMETA.yml and this requires special attention! crypto Crypto community (ACME, openssl, letsencrypt) feature This issue/PR relates to a feature request. module This issue/PR relates to a module. shipit This PR is ready to be merged by Core support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants