core icon indicating copy to clipboard operation
core copied to clipboard

Bump aiotractive, properly handle auth errors when reading events

Open zhulik opened this issue 3 years ago • 4 comments

Breaking change

Proposed change

Update aiotractive, properly handle authorization error when reading events. Please check out the question in the comment.

Type of change

  • [X] Dependency upgrade
  • [ ] Bugfixpylint: error: no such option: --ignore-missing-annotations (non-breaking change which fixes an issue)
  • [ ] New integration (thank you!)
  • [ ] New feature (which adds functionality to an existing integration)
  • [ ] Deprecation (breaking change to happen in the future)
  • [ ] Breaking change (fix/feature causing existing functionality to break)
  • [ ] Code quality improvements to existing code or addition of tests

Additional information

Some time ago I was contacted by a Tractive employee and they said the tractive integration may flood their servers with requests in case if a user changes their password or if the auth token is somehow becomes invalid. The latest version of aiotractive contains a fix for this. Besides that I'd also like to properly handle this situation on the HASS side by disabling the integration when the server responds with an unathorized error, but I'm not quiet sure how to achieve it (see comment in the code).

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

Checklist

  • [X] The code change is tested and works locally.
  • [X] Local tests pass. Your PR cannot be merged unless tests pass
  • [x] There is no commented out code in this PR.
  • [x] I have followed the development checklist
  • [x] 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:

  • [x] The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • [x] 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:

zhulik avatar Aug 13 '22 12:08 zhulik

Hey there @danielhiversen, @bieniu, mind taking a look at this pull request as it has been labeled with an integration (tractive) you are listed as a code owner for? Thanks! (message by CodeOwnersMention)

Now it properly handles authorization errors and triggers reauth workflow. Tested and ready for merging

zhulik avatar Aug 24 '22 20:08 zhulik

Is there something preventing this one from being merged?

zhulik avatar Sep 13 '22 14:09 zhulik

@zhulik Please have patience. Thanks 👍

edit: seems like the build is failing, that is something at least needs to be fixed and prevents it from being merged (besides patience)

frenck avatar Sep 13 '22 15:09 frenck

@bieniu This PR was merged, but is missing critical information in the PR description. Therefore it should not have been merged.

As this PR changes a dependency we require the PR description to contain at least one (or multiple) of the following:

  • A link to the release notes of this package version, and all versions in between.
  • A link to the changelog of this package.
  • A link to a Git(Hub) diff/compare view from the current version to the bumped version.

Not any of these were provided.

Didn't you review upstream?

@zhulik Please update the PR description with the above.

frenck avatar Nov 24 '22 21:11 frenck

@frenck A link to the changelog added to the description. Sorry, didn't notice this missing.

bieniu avatar Nov 24 '22 21:11 bieniu

No problem! Thanks for the adjustment 👍

../Frenck

frenck avatar Nov 24 '22 21:11 frenck