-
Notifications
You must be signed in to change notification settings - Fork 14
feat: support env #127
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
base: main
Are you sure you want to change the base?
feat: support env #127
Conversation
26a9f36
to
0cd20ae
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #127 +/- ##
==========================================
+ Coverage 61.92% 63.84% +1.92%
==========================================
Files 33 33
Lines 2135 2246 +111
==========================================
+ Hits 1322 1434 +112
+ Misses 813 812 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I think config from environment variables was already supported (see method
|
Hi Jan |
Hi @jansimonb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for the V2
values in InfluxDBClient.from_env_properties()
these are relics of the original copy and paste from the V2 client libraries roughly two years ago. Perhaps we should mark it as @deprecated
e.g.
@deprecated("Use up to date Env Properties")
@classmethod
def from_env_properties(cls, debug=None, enable_gzip=False, **kwargs):
influxdb_client_3/__init__.py
Outdated
INFLUX_GZIP_THRESHOLD = "INFLUX_GZIP_THRESHOLD" | ||
|
||
|
||
def from_env(**kwargs: Any) -> 'InfluxDBClient3': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be a @classmethod
on InfluxDBClient3
See https://github.com/influxdata/influxdb-client-python/blob/74013566a9df3e41dc1ef67cda0cbd0f6b83c733/influxdb_client/client/influxdb_client.py#L180
Also since most of these options are passed to write_options
maybe it would be cleaner to have a class method there as well. e.g.
class WriteOptions(object):
...
@classmethod
def from_envars(cls, debug=None, enable_gzip=False, **kwargs):
url = os.getenv('INFLUX_HOST', "http://localhost:8086")
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @karel-rehor
I had upgraded the from_env function
Can you check again and see if we still need to implement a separated function in WriteOptions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment went missing in last review. Retrieved it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In from_env_properties
(what we want to deprecate) there are 13 environment variables supported. In the new from_env
we support 7 env variables. Does that mean that we are loosing some functionality? E.g. is it still possible to use the new from_env
method but also configure connection_pool_maxsize
?
Hi @jansimonb |
c7b3101
to
ec8813c
Compare
Closes Issue
Proposed Changes
Checklist