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

Implement SSL #6

Merged
merged 6 commits into from
Oct 23, 2023
Merged

Conversation

argysamo
Copy link
Contributor

Introduce backwards compatible SSL.

  • [script.py] Add optional argument for ssl-private_key, ssl-public_key and ssl-ca
  • [web.py] Add optional ssl_context
  • [conftest.py] Pytest utilities to test ssl
  • [script_test.py/web_test.py] Add ssl testing

Copy link
Owner

@albertodonato albertodonato 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 the contribution!

Changes look good, I have a few minor comments.

prometheus_aioexporter/script.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
tests/script_test.py Outdated Show resolved Hide resolved
prometheus_aioexporter/script.py Show resolved Hide resolved
@argysamo argysamo force-pushed the feature/support-ssl branch from fda1fc6 to 54bcca8 Compare October 22, 2023 18:14
Copy link
Owner

@albertodonato albertodonato 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 the fixes! A couple of final (and minor) comments

prometheus_aioexporter/script.py Outdated Show resolved Hide resolved
prometheus_aioexporter/script.py Outdated Show resolved Hide resolved
@albertodonato
Copy link
Owner

FWIW the lint issue should go away if you rebase on main

@argysamo
Copy link
Contributor Author

argysamo commented Oct 23, 2023

FWIW the lint issue should go away if you rebase on main

I have branched out main branch and main branch wasn't updated since. Yet, I tried to rebase, but there wasn't anything to rebase onto.
Ignore my comment above. I wasn't fetching with --all flag and I was only fetching my origin remote. I have now rebased on the latest main!

Introduce backwards compatible SSL.
- [script.py] Add optional argument for ssl-private_key, ssl-public_key and ssl-ca
- [web.py] Add optional ssl_context
- [conftest.py] Pytest utilities to test ssl
- [script_test.py/web_test.py] Add ssl testing
- [conftest.py]: Replace all `return` with `yield` statements
- [pyproject.toml]: Add `trustme` as test dependency
- [script.py]: `_configure_ssl` is now `_get_ssl_context` (not a staticmethod anymore). SSL arguments are of type `FileType` (not `str`).
- [script_test.py]: Remove the `xfail` decorator
- [web.py and script.py]: Replace `Optional` with `| None`
- args.FileType returns a FileType object. `load_cert_chain` expects public/private key paths as `str`. `name` attribute (of `FileType`) returns exactly what is needed.
- Sort imports
@argysamo argysamo force-pushed the feature/support-ssl branch from 3031e99 to 43a41ff Compare October 23, 2023 19:36
- [script.py]: Remove `from ssl import SSLContext` statement. Close ssl related FileType objects.
- [README.rst]: Add references to the SSL arguments
@argysamo argysamo force-pushed the feature/support-ssl branch from 43a41ff to 8fc42fc Compare October 23, 2023 19:41
@albertodonato albertodonato merged commit dc6fe22 into albertodonato:main Oct 23, 2023
3 checks passed
@albertodonato
Copy link
Owner

Thank you very much!

@argysamo
Copy link
Contributor Author

argysamo commented Oct 24, 2023 via email

@albertodonato
Copy link
Owner

I plan to make a 2.0 release soon which will also include the changes I made for MetricConfig (which are backwards-incompatible, hence the change). Will have to update query-exporter too for this change.

@argysamo
Copy link
Contributor Author

thanks so much. Please let me know if there's anything I could help with!

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.

2 participants