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

Bump influxdb-client dependency to 1.24.0 #63397

Merged
merged 3 commits into from
Jan 5, 2022

Conversation

mdegat01
Copy link
Contributor

@mdegat01 mdegat01 commented Jan 4, 2022

Proposed change

Update influxdb-client dependency from 1.14.0 to 1.24.0 (full changelog). Used for communication with InfluxDB 2.0 deployments.

Note that there were breaking changes in 1.18.0. I have reviewed them and they were to APIs we don't use. I also tested locally and confirmed things work fine.

One nice addition is influxdata/influxdb-client-python#264 as it means users can now use the more user-friendly and easier to find name instead of tracking down the ID. We don't have to do anything else to make use of this, ID still works and now name does too. Other then that mostly bugfixes from our perspective.

Notes on included code changes:

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@frenck frenck added the smash Indicator this PR is close to finish for merging or closing label Jan 4, 2022
@mdegat01
Copy link
Contributor Author

mdegat01 commented Jan 4, 2022

Ah shoot forgot to update the manifest, so I guess my test was invalid. Give me a moment, sorry about that.

@mdegat01
Copy link
Contributor Author

mdegat01 commented Jan 4, 2022

Ok that's rather amusing. Looks like a bit of a surprise breaking change for us. We were passing timeout as 5 because the V1 InfluxDB client accepted a timeout in seconds. The V2 client had the same input so I assumed it was also in seconds, it didn't specify a unit. Apparently it just didn't work at all influxdata/influxdb-client-python#221 . Now it's fixed and they clarified the unit is in milliseconds.

So I will also have to make a small change as part of this PR to account for that.

@frenck frenck removed the smash Indicator this PR is close to finish for merging or closing label Jan 4, 2022
@mdegat01 mdegat01 requested a review from fabaff as a code owner January 4, 2022 19:19
@mdegat01
Copy link
Contributor Author

mdegat01 commented Jan 4, 2022

So have been digging into this, appears something changed which broke the tests between 1.21.0 and 1.22.0. Went up release by release and that's where the failures start. Will need to update them as well I suppose, hopefully its small. Didn't expect this to be so involved but I guess that's how it goes with dependency upgrades sometimes.

@mdegat01
Copy link
Contributor Author

mdegat01 commented Jan 5, 2022

Ok figured it out. It appears release 1.22.0 brought some significant improvements to error handling. I remember struggling with that initially and figuring out what exceptions to catch in each situation mostly by trial and error. Now there is one exception class the author has normalized everything to which is great, I'll do some refactoring in a future PR.

The one downside is the new Exception class didn't account for scenarios that really only come up in testing. Namely someone tries to create an instance of ApiException but doesn't have an actual HTTPResponse which is what the tests were doing to create mock exceptions. Solving this required a small tweak to the tests, passing a MagicMock as the http response so we don't get "accessing attribute of NoneType" errors.

The issue is actually already fixed in dev tip by influxdata/influxdb-client-python#375 but I'm not sure when the next release is. Figured it was better to get up to date since this only warranted a minor change and now error handling needs a refactor anyway.

@frenck frenck added the smash Indicator this PR is close to finish for merging or closing label Jan 5, 2022
Copy link
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

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

Thanks, @mdegat01 👍

Happy New Year 🍾

@frenck frenck merged commit c1967ab into home-assistant:dev Jan 5, 2022
@mdegat01 mdegat01 deleted the bump-influxdb-client branch January 5, 2022 16:54
@github-actions github-actions bot locked and limited conversation to collaborators Jan 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-signed dependency dependency-bump small-pr PRs with less than 30 lines. smash Indicator this PR is close to finish for merging or closing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants