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

feat: update user-agent and improve transport selection in queryClient #92

Merged
merged 4 commits into from
Jun 11, 2024

Conversation

karel-rehor
Copy link
Contributor

@karel-rehor karel-rehor commented Jun 6, 2024

Proposed Changes

  1. Updates user-agent value to influxdb3-python/{VERSION}
  2. Moves file version.py to directory influxdb_client_3
  3. Adds user-agent value to _flight_client. Note that grpc library underlying pyarrow also adds a user-agent header so the two are merged. e.g. ['grpc-c++/1.51.1 grpc-c/29.0.0 (linux; chttp2) influxdb3-python/0.6.0dev0']
  4. Tests added for these changes

Checklist

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

@karel-rehor karel-rehor requested a review from bednar June 6, 2024 11:44
Copy link

codecov bot commented Jun 6, 2024

Codecov Report

Attention: Patch coverage is 80.61224% with 19 lines in your changes missing coverage. Please review.

Project coverage is 50.52%. Comparing base (933ecb5) to head (8e3197c).

Files Patch % Lines
tests/test_query.py 60.00% 18 Missing ⚠️
tests/test_api_client.py 97.56% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            main      #92       +/-   ##
==========================================
+ Coverage   7.01%   50.52%   +43.51%     
==========================================
  Files         38       39        +1     
  Lines       2196     2276       +80     
==========================================
+ Hits         154     1150      +996     
+ Misses      2042     1126      -916     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@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.

Thank you for your PR! 👍 There is just one requirement that needs to be met before we can merge it into the master branch:

Comment on lines 158 to 161
if scheme == 'https':
self._flight_client = FlightClient(f"grpc+tls://{hostname}:{port}", **self._flight_client_options)
else:
self._flight_client = FlightClient(f"grpc+tcp://{hostname}:{port}", **self._flight_client_options)
Copy link
Member

Choose a reason for hiding this comment

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

Please simplify this to something like:

Suggested change
if scheme == 'https':
self._flight_client = FlightClient(f"grpc+tls://{hostname}:{port}", **self._flight_client_options)
else:
self._flight_client = FlightClient(f"grpc+tcp://{hostname}:{port}", **self._flight_client_options)
if scheme == 'https':
connectionString = f"grpc+tls://{hostname}:{port}"
else:
connectionString = f"grpc+tcp://{hostname}:{port}"
self._flight_client = FlightClient(connectionString, **self._flight_client_options)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. The connection_string is a better solution. Changed.

Copy link
Member

@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 added this to the 0.6.0 milestone Jun 11, 2024
@bednar bednar merged commit c580008 into main Jun 11, 2024
13 checks passed
@bednar bednar deleted the feat/userAgent branch June 11, 2024 12:16
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