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.

Sign up for GitHub

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

Jump to bottom

Add zwave to zwave_js migration #56159

Merged
merged 22 commits into from
Sep 29, 2021
Merged

Add zwave to zwave_js migration #56159

merged 22 commits into from
Sep 29, 2021

Conversation

MartinHjelmare
Copy link
Member

@MartinHjelmare MartinHjelmare commented Sep 12, 2021 •
edited
Loading

Breaking change

Proposed change

  • Add migration support for legacy Z-Wave to Z-Wave JS integration.
  • Remove migration support for Z-Wave to OpenZWave integration.
  • Migration mapping is limited to values we can resolve in both Z-Wave and Z-Wave JS. Eg we can not resolve two values in Z-Wave JS which have the same property name but different property key name since there's nothing on the Z-Wave side to map to property key name on the Z-Wave JS side.
  • If we find more information to help resolution we can improve this in the future.

TODO

  • Remove failing zwave test.
  • Test again.
  • Wait for frontend approval.

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

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:

@probot-home-assistant
Copy link

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

@probot-home-assistant
Copy link

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

@probot-home-assistant
Copy link

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

balloob
balloob reviewed Sep 13, 2021
View reviewed changes
homeassistant/components/zwave/migration.py

LEGACY_ZWAVE_MIGRATION = f"{DOMAIN}_legacy_zwave_migration"
STORAGE_WRITE_DELAY = 30
STORAGE_KEY = f"{DOMAIN}.legacy_zwave_migration"
Copy link
Member

Choose a reason for hiding this comment

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

Is it still needed to add legacy_zwave if we already prefix the value with the zwave domain?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe not.

Copy link
Member Author

Choose a reason for hiding this comment

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

One reason too keep it, is it'll be easier to identify the two storage files that are part of the same migration.

balloob
balloob reviewed Sep 13, 2021
View reviewed changes
homeassistant/components/zwave/migration.py
"command_class_label": entity_values.primary.label,
"value_index": entity_values.primary.index,
"device_id": device_entry.id,
"domain": entity_entry.domain,
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this always zwave ?

Copy link
Member Author

@MartinHjelmare MartinHjelmare Sep 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The domain here is the first part of the entity id (before the period).

balloob
balloob reviewed Sep 13, 2021
View reviewed changes
Copy link
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

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

First pass looks great 🎉

@MartinHjelmare MartinHjelmare mentioned this pull request Sep 13, 2021
Open
67 tasks
@Hedda Hedda mentioned this pull request Sep 15, 2021
Closed
@MartinHjelmare MartinHjelmare force-pushed the add-zwave-zwave_js-migration branch from 619dc7d to 8171165 Compare September 16, 2021 13:58
@fermulator
Copy link

Should we update https://www.home-assistant.io/integrations/zwave_js/ ? * Migrating from previous Z-Wave implementations*

"That said, we currently do not provide a full-fledged, worry-free, click-a-button, migration from old to new. We’re exploring options to provide this in the future."

Thinking to either and/or

  1. Add link for "exploring options" to this ticket (if the PR doesn't fully addres sit)
  2. Update documentation to denote the capability that this PR delivers.

MartinHjelmare and others added 22 commits September 29, 2021 15:16
@MartinHjelmare
Add zwave to zwave_js migration
7e78cef
@MartinHjelmare
Remove ozw import flow
1ed039a
@MartinHjelmare
Register zwave start zwave_js import flow
7fd32f4
@MartinHjelmare
Clean up zwave stale migration function
16992a0
@MartinHjelmare
Test zwave zwave_js websocket import flow command
ff724f9
@MartinHjelmare
Remove zwave ozw migration websocket import flow test
88cb092
@MartinHjelmare
Remove ozw migration
3d615a5
@MartinHjelmare
Remove ozw migration websocket command
84b66ec
@MartinHjelmare
Update hassfest deps ignore list
6705ef6
@MartinHjelmare
Remove zwave after deps
2e151ec
@MartinHjelmare
Check if zwave_js migration has been done in zwave
e880660
@MartinHjelmare
Make get_legacy_zwave_migration a coroutine function
9f6b4a8
@MartinHjelmare
Remove not needed error log
5eaba77
@MartinHjelmare
Guard for any number of non resolvable entry duplicates
2185156
@MartinHjelmare @balloob
Use walrus for unit
7aada75 
Co-authored-by: Paulus Schoutsen <balloob@gmail.com>
@MartinHjelmare
Use walrus for unit
d84ae88
@MartinHjelmare
Remove zwave refresh entity test
f3d3a5d
@MartinHjelmare
Fix missing await
1f2395f
@MartinHjelmare
Fix saving data after load
4e589cd
@MartinHjelmare
Add node ready websocket command
5654a97
@MartinHjelmare
Register node ready command
15ba6b3
@MartinHjelmare
Test node ready command
d6e701a
@MartinHjelmare MartinHjelmare force-pushed the add-zwave-zwave_js-migration branch from c63e387 to d6e701a Compare September 29, 2021 13:16
@balloob balloob merged commit 50fffe4 into dev Sep 29, 2021
@balloob balloob deleted the add-zwave-zwave_js-migration branch September 29, 2021 15:55
@github-actions github-actions bot locked and limited conversation to collaborators Sep 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@balloob balloob balloob approved these changes

@cgarwood cgarwood Awaiting requested review from cgarwood

@marcelveldt marcelveldt Awaiting requested review from marcelveldt

Assignees

@marcelveldt marcelveldt

@cgarwood cgarwood

Labels
by-code-owner cla-signed has-tests integration: ozw integration: zwave_js integration: zwave
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

How to ease migration to Z-Wave JS
6 participants
@MartinHjelmare @fermulator @balloob @marcelveldt @cgarwood @homeassistant