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

Make request timeout configurable for all acme modules #448

Merged
merged 2 commits into from
May 3, 2022
Merged

Make request timeout configurable for all acme modules #448

merged 2 commits into from
May 3, 2022

Conversation

JonasVerhofste
Copy link
Contributor

SUMMARY

Makes request timeouts configurable for all acme_-modules that use the shared acme argspec. Fixes #447.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

plugins/module_utils/acme/acme.py; affected modules:
acme_account.py
acme_account_info.py
acme_inspect.py
acme_certificate.py
acme_certificate_revoke.py

ADDITIONAL INFORMATION

Added timeout as an alias because it seemed logical, not sure if that's allowed (i.e. if that is usually only used for backwards compatible changes).

@JonasVerhofste
Copy link
Contributor Author

Oh right, let me add some tests..

@JonasVerhofste
Copy link
Contributor Author

@felixfontein Where would you suggest adding this to the tests (if needed at all)?

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.

Thanks for your contribution! Could you please add a changelog fragment?

About the alias: I would not add timeout as an alias to make it possible to use that name for something else in the future. For example, acme_certificate could have a timeout for the second stage where it eventually gives up waiting for the finalization to take place. Once such a functionality is there, I would expect timeout to be in control of that one. (It will probably be best to use a different name for that one as well, and not use timeout at all, to make it less confusing.)

plugins/doc_fragments/acme.py Outdated Show resolved Hide resolved
plugins/doc_fragments/acme.py Outdated Show resolved Hide resolved
@felixfontein
Copy link
Contributor

@felixfontein Where would you suggest adding this to the tests (if needed at all)?

Timeouts are hard to test with integration tests (without having an endpoint supporting that usage), so unit tests would be best. But since we right now have to unit tests for anything HTTP related, I think it's ok to not add tests here.

@JonasVerhofste
Copy link
Contributor Author

@felixfontein removed the timeout alias and added the changelog fragment!

@felixfontein felixfontein merged commit c16d9f7 into ansible-collections:main May 3, 2022
@felixfontein
Copy link
Contributor

@JonasVerhofste thanks for implementing this!

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.

Acme_* modules should have configurable timeouts
2 participants