core icon indicating copy to clipboard operation
core copied to clipboard

Add new method version_is_newer to Update platform

Open chemelli74 opened this issue 1 year ago • 13 comments

Breaking change

Proposed change

Update platform compare versions using AwesomeVersion library. For some devices this comparison always fails with AwesomeVersionCompareException.

Introducing a method that can be overwritten in order to address those scenarios. Nothing changes for current implementations as default is the previous code path.

Type of change

  • [ ] Dependency upgrade
  • [ ] Bugfix (non-breaking change which fixes an issue)
  • [ ] New integration (thank you!)
  • [x] 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

  • 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] I have followed the perfect PR recommendations
  • [x] The code has been formatted using Ruff (ruff format 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.

To help with the load of incoming pull requests:

chemelli74 avatar Aug 28 '24 14:08 chemelli74

Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration (update) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of update can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign update Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

home-assistant[bot] avatar Aug 28 '24 14:08 home-assistant[bot]

Please explain what fails and why that isn't possible to solve with AwesomeVersion.

MartinHjelmare avatar Aug 28 '24 14:08 MartinHjelmare

Shelly gen 1 devices have the following format:

  • latest_version: "20230913-111730/v1.14.0-gcb84623"
  • installer_version: "20231107-162609/v1.14.1-rc1-g0617c15"

Which such strings , AwesomeVersion always fails with an exception.

Using instead simple string comparison works fine.

Note: Shelly gen2/3 devices work fine as the strings are "2.1.0" and "2.2.0-beta7".

chemelli74 avatar Aug 28 '24 15:08 chemelli74

I think it should have tests added to the platform, even if coverage doesn't see it as missing (due to the inline if).

epenet avatar Aug 28 '24 15:08 epenet

I think it should have tests added to the platform, even if coverage doesn't see it as missing.

Ok, once we agree on proceeding I'll work on them :blush:

chemelli74 avatar Aug 28 '24 15:08 chemelli74

But, this only works because 20231107 is a larger number than 20230913. So why don't you just do version.split("-")[0] for both values and have awesome version handle it?

joostlek avatar Aug 28 '24 15:08 joostlek

But, this only works because 20231107 is a larger number than 20230913. So why don't you just do version.split("-")[0] for both values and have awesome version handle it?

We don't manipulate the fw version to be able to show the complete string in the UI.

chemelli74 avatar Aug 28 '24 15:08 chemelli74

Just a thought: I wonder if maybe moving adding a custom function would be better than this boolean.

if self._attr_compare_fn:
    newer = self._attr_compare_fn(latest_version, installed_version)
else:
    try:
        newer = _version_is_newer(latest_version, installed_version)
    except AwesomeVersionCompareException:
        # Can't compare versions, already tried exact match
        return STATE_ON

epenet avatar Aug 28 '24 15:08 epenet

Please open an architecture discussion. I don't think this is a small fix we can just accept without discussion.

MartinHjelmare avatar Aug 28 '24 15:08 MartinHjelmare

Please open an architecture discussion. I don't think this is a small fix we can just accept without discussion.

https://github.com/home-assistant/architecture/discussions/1130

chemelli74 avatar Aug 28 '24 15:08 chemelli74

This needs updated developer documentation

emontnemery avatar Sep 09 '24 13:09 emontnemery

Please update the PR description with the latest approach.

MartinHjelmare avatar Sep 09 '24 13:09 MartinHjelmare

I've removed the milestone, as this isn't resolved at this point

frenck avatar Sep 16 '24 13:09 frenck

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks :+1:

Learn more about our pull request process.

home-assistant[bot] avatar Sep 19 '24 07:09 home-assistant[bot]

This is a new feature, and shouldn't go in a patch release.

MartinHjelmare avatar Sep 19 '24 09:09 MartinHjelmare

The dev docs PR wasn't linked in the description. I added it.

MartinHjelmare avatar Sep 19 '24 09:09 MartinHjelmare

This is a new feature, and shouldn't go in a patch release.

Fine for me, but as it fixes a issue with version comparison of some firmware versions ( current code shows a new version available that is actually a downgrade), I though it could be categorized as bug fix.

chemelli74 avatar Sep 19 '24 10:09 chemelli74

This doesn't fix anything on its own. We can't categorize it as a fix.

MartinHjelmare avatar Sep 19 '24 11:09 MartinHjelmare