Skip to content

feat: MTLS support for the InfluxDB Python client #509

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

Merged
merged 6 commits into from
Oct 17, 2022
Merged

feat: MTLS support for the InfluxDB Python client #509

merged 6 commits into from
Oct 17, 2022

Conversation

ZPascal
Copy link
Contributor

@ZPascal ZPascal commented Oct 5, 2022

Closes #508

Proposed Changes

Add MTLS support to the InfluxDB Python client as already discussed inside the following issue.

TODO

  • Check the context usage (sync case)
  • Add unit tests
  • Squash the commits together

Checklist

  • CHANGELOG.md updated
  • Rebased/mergeable
  • A test has been added if appropriate
  • pytest tests completes successfully
  • Commit messages are conventional
  • Sign CLA (if not already signed)

@ZPascal ZPascal changed the title Add first version of the MTLS feature MTLS support for the InfluxDB Python client Oct 5, 2022
@ZPascal ZPascal marked this pull request as ready for review October 7, 2022 13:22
@ZPascal
Copy link
Contributor Author

ZPascal commented Oct 7, 2022

Hi @bednar, should I also prepare an integration test scenario, or is it for these optional parameters not necessary?

@ZPascal ZPascal changed the title MTLS support for the InfluxDB Python client feat: MTLS support for the InfluxDB Python client Oct 7, 2022
@ZPascal
Copy link
Contributor Author

ZPascal commented Oct 10, 2022

Hi Jakob @bednar, could you please approve the missing workflows and review the change. Thanks

@bednar bednar self-requested a review October 11, 2022 08:54
Copy link
Contributor

@bednar bednar 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 PR 👍
There are a few requirements that must be be satisfy before we accept the PR:

@codecov-commenter
Copy link

codecov-commenter commented Oct 11, 2022

Codecov Report

Base: 90.11% // Head: 90.14% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (d011df7) compared to base (50f021e).
Patch coverage: 85.18% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #509      +/-   ##
==========================================
+ Coverage   90.11%   90.14%   +0.02%     
==========================================
  Files          39       39              
  Lines        3380     3400      +20     
==========================================
+ Hits         3046     3065      +19     
- Misses        334      335       +1     
Impacted Files Coverage Δ
influxdb_client/_sync/rest.py 86.99% <ø> (ø)
influxdb_client/client/influxdb_client.py 98.73% <ø> (ø)
influxdb_client/client/influxdb_client_async.py 87.93% <ø> (ø)
influxdb_client/_async/rest.py 77.77% <50.00%> (-0.45%) ⬇️
influxdb_client/client/_base.py 97.47% <100.00%> (+0.11%) ⬆️
influxdb_client/configuration.py 87.91% <100.00%> (+0.27%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ZPascal ZPascal requested a review from bednar October 11, 2022 19:38
Copy link
Contributor

@bednar bednar left a comment

Choose a reason for hiding this comment

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

Thanks again for your PR 👍 There are a few requirements that must be be satisfy before we accept the PR:

  1. Rebase your sources with master branch
  2. Add new client's options (:key str cert_file, ...) to the from_config_file and from_env_properties functions. For more info see: feat: allow to use client's optional configs for initialization from file or environment properties #510

and also:

@ZPascal
Copy link
Contributor Author

ZPascal commented Oct 12, 2022

Hi @bednar, I'll adapt the PR. I'm also thinking about the possibility to add all SSL-related optional parameters like verify_ssl or ssl_ca_cert to the docstring documentation of the from_env_properties and from_config_file function. What do think about the documentation feature?

@bednar
Copy link
Contributor

bednar commented Oct 12, 2022

I'll adapt the PR. I'm also thinking about the possibility to add all SSL-related optional parameters like verify_ssl or ssl_ca_cert to the docstring documentation of the from_env_properties and from_config_file function. What do think about the documentation feature?

Yes, it will be useful. We already improve this docs in #510, please rebase your source to check an actual version of the client.

BREAKING CHANGE: Rename `key_file` to `cert_key_file` inside the central configuration class

Co-authored-by: Jakub Bednář <[email protected]>
@ZPascal
Copy link
Contributor Author

ZPascal commented Oct 12, 2022

Yes, it will be useful. We already improve this docs in #510, please rebase your source to check an actual version of the client.

I've handled it.

@ZPascal ZPascal requested a review from bednar October 12, 2022 12:01
Copy link
Contributor

@bednar bednar left a comment

Choose a reason for hiding this comment

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

We are almost ready to merge this PR, but there are a few requirements that must be be satisfy before we accept the PR:

@ZPascal ZPascal requested a review from bednar October 13, 2022 18:55
Copy link
Contributor

@bednar bednar left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@bednar bednar merged commit fab076c into influxdata:master Oct 17, 2022
@bednar bednar added this to the 1.34.0 milestone Oct 17, 2022
@bednar bednar added the enhancement New feature or request label Oct 17, 2022
@ZPascal ZPascal deleted the add-mtls-support branch October 17, 2022 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MTLS support for the Python client
3 participants