-
Notifications
You must be signed in to change notification settings - Fork 13
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% 62.80% +0.88%
==========================================
Files 33 33
Lines 2135 2194 +59
==========================================
+ Hits 1322 1378 +56
- Misses 813 816 +3 ☔ 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):
write_options.gzip_threshold = gzip_threshold | ||
|
||
if os.getenv(INFLUX_PRECISION) is not None: | ||
write_options.write_precision = os.getenv(INFLUX_PRECISION) |
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.
Need to check that the provided value is valid before assignment. Could also be encapsulated in a _parse_precision
function. A value like "foo" could result in unpredictable behavior.
if os.getenv(INFLUX_GZIP_THRESHOLD) is not None: | ||
gzip_threshold = int(os.getenv(INFLUX_GZIP_THRESHOLD)) | ||
write_options.enable_gzip = True | ||
write_options.gzip_threshold = gzip_threshold |
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.
Need to verify that the provided value is valid. This could be encapsulated in a _parse_gzip_threshold
function.
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.
One comment went missing in last review. Retrieved it here.
Closes Issue
Proposed Changes
Checklist