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

Replace clear() method for backward compatibility. #64504

Merged
merged 1 commit into from Nov 7, 2019

Conversation

n0trax
Copy link
Contributor

@n0trax n0trax commented Nov 6, 2019

SUMMARY

The clear() method is only supported from python >= 3.3, but acme_certificate has python >=2.6 as requirement. Therefore we should replace the clear() method.

Fixes #64501

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

acme_certificate

@ansibot
Copy link
Contributor

ansibot commented Nov 6, 2019

@ansibot ansibot added affects_2.10 This issue/PR affects Ansible v2.10 bug This issue/PR relates to a bug. community_review In order to be merged, this PR must follow the community review workflow. crypto Crypto community (ACME, openssl, letsencrypt) module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. small_patch support:community This issue/PR relates to code supported by the Ansible community. labels Nov 6, 2019
@felixfontein
Copy link
Contributor

felixfontein commented Nov 6, 2019

Thanks for spotting this! Unfortunately the integration tests didn't catch this since they don't use ACME v1... BTW, out of curiosity: why are you still using ACME v1? Is it because you didn't upgrade yet, or because the ACME server you're using doesn't support ACME v2? (Edit: from your example in #64501 I see you're using Let's Encrypt. Please note that Let's Encrypt deprecated ACME v1 and will remove it eventually.)

In any case, you need a changelog fragment:

bugfixes:
- "acme_certificate - fix crash when module is used with Python 2.x."

@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Nov 6, 2019
@felixfontein
Copy link
Contributor

(In case you're interested, Let's Encrypt's ACME v1 deprecation.)

@MarkusTeufelberger
Copy link
Contributor

lgtm

@n0trax
Copy link
Contributor Author

n0trax commented Nov 7, 2019

@felixfontein the task uses the ACMEv1 because it is an older playbook and the default acme_version of the acme_certificate module is still ACMEv1.
But thanks for the hint :-) I updated my letsEncrypt playbook to use ACMEv2.

I also added the changelog fragment.

Copy link
Contributor

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

shipit

@ansibot ansibot added automerge This PR was automatically merged by ansibot. shipit This PR is ready to be merged by Core and removed community_review In order to be merged, this PR must follow the community review workflow. labels Nov 7, 2019
@ansibot ansibot merged commit 27d3dd5 into ansible:devel Nov 7, 2019
@felixfontein
Copy link
Contributor

@n0trax thanks for reporting and fixing this! I'll create backport PRs so this will also get fixed in 2.8.x and 2.9.x eventually.
@MarkusTeufelberger thanks for reviewing!

@n0trax actually the default for acme_version will change eventually; from Ansible 2.10 on you'll get a warning if you don't specify it, with a note that there will no longer be a default for Ansible 2.14. From then on, you'll have to specify the version explicitly.

felixfontein pushed a commit to felixfontein/ansible that referenced this pull request Nov 7, 2019
@felixfontein
Copy link
Contributor

(Just noticed, no backport necessary to stable-2.8 since that doesn't happen in Ansible 2.8.x. The broken code was new for 2.9.0.)

mattclay pushed a commit that referenced this pull request Nov 12, 2019
@ansible ansible locked and limited conversation to collaborators Dec 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.10 This issue/PR affects Ansible v2.10 automerge This PR was automatically merged by ansibot. bug This issue/PR relates to a bug. crypto Crypto community (ACME, openssl, letsencrypt) 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

acme_certificate fails on python 2.7
4 participants