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

Updater component is always available and shows on/off depending on whether an update is available or not #25418

Merged
merged 27 commits into from Aug 7, 2019

Conversation

@Santobert
Copy link
Contributor

commented Jul 23, 2019

Breaking Change:

The updater component is now a binary sensor that is always available. The entity ID is binary_sensor.updater. The state is on/off depending on whether an update is available or not. The latest version as well as the release notes are attributes of this binary sensor.

Automations that are listening for the existence of updater.updater should now trigger when binary_sensor.updater changes to on.

This makes the component more transparent and understandable for the user. Additionally it is visible if there is an error or the source (https://updater.home-assistant.io/) is outdated.

Description:

See Breaking Change.

Furthermore the updater logs a warning if the current version is before the newest version. This is the case if the JSON from https://updater.home-assistant.io/ is outdated.

If you don't want the updater component to always be available, I suggest at least keeping the warning if the current version is greater than the newest version. Otherwise, people would wonder if the updater component is broken.

But personally, I think it would be more understandable if the updater component is always visible and displays the newest version. Automations can be triggered by the state and not by the existence of this component.

Related issue (if applicable): fixes #24385

Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#9957

Example entry for configuration.yaml (if applicable):

No changes

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist

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

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@Santobert Santobert requested a review from home-assistant/core as a code owner Jul 23, 2019

@homeassistant

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2019

Hi @Santobert,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@ghost

This comment has been minimized.

Copy link

commented Jul 23, 2019

Hey there @home-assistant/core, mind taking a look at this pull request as its been labeled with a integration (updater) you are listed as a codeowner for? Thanks!

This is a automatic comment generated by codeowners-mention to help ensure issues and pull requests are seen by the right people.

@homeassistant homeassistant added cla-signed and removed cla-needed labels Jul 23, 2019

@Santobert

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

Please give me feedback if this change is acceptable. If so, I will update the documentation as soon as possible.

@frenck

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

Please don't hold off creating a documentation PR, it slows down all progress, since merging without docs is a no go to being with. Adding docs-missing label.

@frenck frenck added the docs-missing label Jul 23, 2019

@Santobert Santobert referenced this pull request Jul 23, 2019
2 of 2 tasks complete
@Santobert

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

The documentation now is up to date: home-assistant/home-assistant.io#9957

@MartinHjelmare MartinHjelmare added this to Needs review in Dev Jul 23, 2019

@MartinHjelmare MartinHjelmare moved this from Needs review to Review in progress in Dev Jul 26, 2019

@Santobert

This comment has been minimized.

Copy link
Contributor Author

commented Jul 28, 2019

The docs-missing label can be removed. I've added the docs here: home-assistant/home-assistant.io#9957

@frenck frenck removed the docs-missing label Jul 28, 2019

@Santobert

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

I guess there is something wrong with the CI? These errors are import errors and they have nothing to do with my changes.

Santobert added 4 commits Jul 31, 2019
Santobert added 2 commits Aug 2, 2019
Santobert added 2 commits Aug 2, 2019
Revert "Add another test"
This reverts commit 3f896a4.
Santobert added 2 commits Aug 2, 2019
@Santobert

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

The platform is already tested in the current tests. Make sure all the new state attributes are tested. The only method not tested is probably the remove method that is called when the entity is removed. I'm not sure when that would happen here, since there's no config entry.

@MartinHjelmare I'm afraid we have to test the remove method since we miss the coverage target by 0.12% 😆 Can we call that method directly or do we have to trigger hass to remove the component?

@Santobert Santobert force-pushed the Santobert:edit_updater_component branch from 0335142 to fa1b618 Aug 6, 2019

tests/components/updater/test_init.py Outdated Show resolved Hide resolved
tests/components/updater/test_init.py Outdated Show resolved Hide resolved
tests/components/updater/test_init.py Outdated Show resolved Hide resolved
@Santobert

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

Thank you @MartinHjelmare for all your help. I wouldn't have been able to do this without you.

Is it possible to merge this into the upcoming version 0.97.0 or do we have to wait for 0.97.1?

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

I think we should wait until 0.98 since it's a new feature and a breaking change.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

Please update the breaking change paragraph in the PR description with what the user needs to do to cope with the breaking change. Ie how to handle the change of entity domain for the updater entity.

@Santobert

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

Please update the breaking change paragraph in the PR description with what the user needs to do to cope with the breaking change. Ie how to handle the change of entity domain for the updater entity.

Done

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

@balloob ok to merge?

@Santobert Santobert changed the title Updater is always available and shows on/off wether an update is available Updater component is always available and shows on/off depending on whether an update is available or not Aug 7, 2019

@balloob balloob merged commit c3455ef into home-assistant:dev Aug 7, 2019

11 checks passed

CI Build #20190806.16 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python36) Tests PyTest Python36 succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
codecov/patch 100% of diff hit (target 94.01%)
Details
codecov/project 94.02% (target 90%)
Details

Dev automation moved this from Reviewer approved to Done Aug 7, 2019

@Santobert Santobert deleted the Santobert:edit_updater_component branch Aug 7, 2019

@lock lock bot locked and limited conversation to collaborators Aug 8, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
5 participants
You can’t perform that action at this time.