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 modules refactor #187

Merged

Conversation

felixfontein
Copy link
Contributor

@felixfontein felixfontein commented Feb 7, 2021

SUMMARY

Some refactoring of module_utils/acme.py: split that monster up, and clean up some of the code. Then it splits up the ACMEAccount object into ACMEClient and ACMEAccount and improves the API w.r.t error handling. Finally it refactors the acme_certificate module to move certain code parts to module_utils.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

acme module_utils
acme_certificate

@felixfontein
Copy link
Contributor Author

recheck

@felixfontein
Copy link
Contributor Author

I used this version to renew all my certificates, worked as expected. So the refactoring cannot be too butched :)

I also did some ACME v1 testing with BuyPass's staging endpoint, which also worked, and I did revoke the old certificates (also worked).

This was referenced Feb 7, 2021
@felixfontein felixfontein force-pushed the acme_certificate-refactor branch from 4c35d48 to 746b3d8 Compare February 11, 2021 06:58
@felixfontein felixfontein changed the title [WIP] acme_certificate refactor ACME modules refactor Mar 8, 2021
@felixfontein
Copy link
Contributor Author

I closed #184 and #186, let's just continue with this one.

@felixfontein felixfontein mentioned this pull request Mar 8, 2021
6 tasks
@felixfontein felixfontein force-pushed the acme_certificate-refactor branch from 746b3d8 to c0d8880 Compare March 8, 2021 06:45
@felixfontein felixfontein force-pushed the acme_certificate-refactor branch from c0d8880 to 021cf60 Compare March 14, 2021 15:38
@felixfontein felixfontein force-pushed the acme_certificate-refactor branch from 021cf60 to ad5dbbb Compare March 14, 2021 16:00
@felixfontein
Copy link
Contributor Author

I've cleaned up the commit history a bit to make it easier to review commit by commit, which is probably a lot easier than reviewing the whole resulting diff.

If anyone would prefer this to be split up into multiple PRs to simplify reviewing, I can happily do that (again). Just tell me what your preferences are.

@rndmh3ro
Copy link

I tested the PR with our Let's Encrypt Collection (https://github.com/T-Systems-MMS/ansible-collection-letsencrypt/). Worked flawlessly.

@felixfontein
Copy link
Contributor Author

Restarting tests for a last time, will merge tomorrow if they passed.

@felixfontein felixfontein reopened this Mar 20, 2021
@felixfontein felixfontein merged commit 5d32937 into ansible-collections:main Mar 21, 2021
@felixfontein
Copy link
Contributor Author

@bmm-alc @rndmh3ro thanks a lot for reviewing and testing!

@felixfontein felixfontein deleted the acme_certificate-refactor branch March 21, 2021 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants