- Notifications
You must be signed in to change notification settings - Fork186
feat: Add database creation/deletion for Influx 1.8#544
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Thanks so much for the pull request! |
!signed-cla |
The fails left are related to a numpy problem. |
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.
Thanks for your PR 👍
Please rebase your sources to fix CI build -#543
influxdb_client/client/bucket_api.py Outdated
| try: | ||
| return self._buckets_service.post_buckets(post_bucket_request=bucket) | ||
| except ApiException: | ||
| # Fall back to v1 API if buckets are not supported | ||
| database_name = bucket_name if bucket_name is not None else bucket |
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 approach is too general and can lead to user confusion. The better way would be to create a separate API for InfluxDB 1.8 something likeinfluxdb_18_api.py.
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.
Thanks for reviewing.
I would disagree to create a separate API (If I understand your suggestion right). The goal should be that code changes are not necessary regardless of what version of InfluxDB a user uses (1.8 or 2.0 and above).
create_bucket() should always succeed for both versions without extra error handling. Otherwise users could continue using the old influxdb-client for 1.8 and this one for >2.0 .
My suggestion would be to implement a more specific exception handler. I agree that justApiException is too general. I will have a look into the returnedApiException.code.
Would this be acceptable for your project?
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.
We can accept this if the behaviour will be described in the documentation. Instead of catching exception you can check the InfluxDB version, something like here:
| def_is_cloud_instance(self)->bool: |
Will do 👍 |
codecov-commenter commentedDec 28, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov ReportBase:90.36% // Head:89.72% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@## master #544 +/- ##==========================================- Coverage 90.36% 89.72% -0.65%========================================== Files 39 39 Lines 3437 3466 +29 ==========================================+ Hits 3106 3110 +4- Misses 331 356 +25
Help us with your feedback. Take ten seconds to tell ushow you rate us. Have a feature suggestion?Share it here. ☔ View full report at Codecov. |
Implement version check
Pitastic commentedDec 28, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
(Sorry for the little mess. I merged my rebased master to early) Thanks for your feedback. I really like your approach and implemented a version check ( I use the docstrings and raise a |
I don't know how to fix the semantic error in the PR title. Wozld you mind to change it to your needs, please? |
ZPascal commentedAug 4, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Hi@Pitastic, I think it would be best to merge the commit and change the commit title to If you need support, feel free to add me as a contributor to the original forked repository. |
Add features related to#541 and#259
Proposed Changes
In order to maintain a minimum of compatibility to Influx v1.8 the creation an deletion of buckets (old: databases) should be supported. With these little enhancements the influxdb-client-python could easily.
I implemented two new methods for calls to v1 API if the method for creating or deleting a bucket fails with an
ApiException. That way no code changes are necessary regardless which version of Influx you are running on.I implemented this with a call to the method
influxdb_client.api_client.call_apiinstead of using the_buckets_service.post_bucketsas this was easier and with less code changes. I had to invoke the creation of some arguments and hope you find it clean enough.Checklist
As I just changed functionallity which wasn't there before and doesn't effect any of the other methods I don't know what to test or how to write a test for this. If this or anything else is needed, please advice me to the right direction.