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

Fix regression of the xiaomi_aqara config validation #22435

Merged
merged 4 commits into from Mar 29, 2019
Merged

Fix regression of the xiaomi_aqara config validation #22435

merged 4 commits into from Mar 29, 2019

Conversation

syssi
Copy link
Member

@syssi syssi commented Mar 26, 2019 •
edited

Breaking Change:

The option key mac isn't allowed to be empty now. Please remove the key if it's empty.

# not allowed anymore
xiaomi_aqara:
    gateways:
        - key: gkdaptzfdjl48ds9
          mac:

# valid
xiaomi_aqara:
    gateways:
        - key: gkdaptzfdjl48ds9

# valid
xiaomi_aqara:
    gateways:
        - key: gkdaptzfdjl48ds9
          mac: 0242cba9b678

Description:

Fixes a regression introduces by #21834:

2019-03-26 17:53:25 ERROR (MainThread) [homeassistant.config] Invalid config for [xiaomi_aqara]:
[mac] is an invalid option for [xiaomi_aqara]. Check: xiaomi_aqara->xiaomi_aqara->gateways->0->mac. (See /home/homeassistant/.homeassistant/configuration.yaml, line 1025). Please check the docs at https://home-assistant.io/components/xiaomi_aqara/

@karlkar Could you review/test this fix?

Allowed configurations are

# Valid
xiaomi_aqara:
    gateways:
        - key: gkdaptzfdjl48ds9

# Valid
xiaomi_aqara:
    gateways:
        - mac: 0242cba9b678
          key: gkdaptzfdjl48ds9

# Valid
xiaomi_aqara:
    gateways:
        - mac: 0242cba9b678
          key: gkdaptzfdjl48ds9
        - mac: 9242cba9b679
          key: vfdaptzfdjl48d3j

# Invalid
xiaomi_aqara:
    gateways:
        - key: gkdaptzfdjl48ds9
        - mac: 9242cba9b679
          key: vfdaptzfdjl48d3j

# Invalid
xiaomi_aqara:
    gateways:
        - key: gkdaptzfdjl48ds9
        - key: vfdaptzfdjl48d3j

@ghost ghost assigned syssi Mar 26, 2019
@ghost ghost added the in progress label Mar 26, 2019
@karlkar
Copy link
Contributor

karlkar commented Mar 26, 2019

Woopsie. Looks like I forgot to commit some local changes.
Now it is ok. Sorry for that.

@syssi syssi added this to the 0.91.0 milestone Mar 28, 2019
amelchio
amelchio reviewed Mar 29, 2019
View reviewed changes
homeassistant/components/xiaomi_aqara/__init__.py Outdated
vol.Optional(CONF_PORT, default=9898): cv.port,
vol.Optional(CONF_DISABLE, default=False): cv.boolean,
GATEWAY_CONFIG_MAC_OPTIONAL = GATEWAY_CONFIG.extend({
vol.Optional(CONF_MAC, default=None): vol.Any(GW_MAC, None)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is pop'ed with a default value, I don't think you need a default here(?)

Copy link
Member Author

Choose a reason for hiding this comment

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

If I remove the default this configuration isn't valid anymore:

xiaomi_aqara:
  gateways:
    - key: ffffffffffffffff
      mac:

This configuration is still valid:

xiaomi_aqara:
  gateways:
    - key: ffffffffffffffff

Do you suggest a removal?

Copy link
Member Author

Choose a reason for hiding this comment

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

To avoid a (small?) breaking change it shouldn't be changed cp. HA 0.91:

https://github.com/home-assistant/home-assistant/blob/0.90.2/homeassistant/components/xiaomi_aqara/__init__.py#L65

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I thought that would be an explicit None and thus accepted. I may be wrong, I am not so strong with schemas.

I will tag @MartinHjelmare because he may like to comment. But this is such a minor issue, I think we should merge to get the fix into a beta and a follow-up PR can clean it up if we want.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should allow specifying an option key without a value. The only case where that is useful is where we want to activate components, ie the top level keys in the config. All other cases are just confusing and error prone.

Remove the default and mark it a breaking change.

Copy link
Member

Choose a reason for hiding this comment

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

vol.Optional(CONF_MAC): GW_MAC,

Copy link
Member

Choose a reason for hiding this comment

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

And use dict.get when accessing that config key.

@balloob balloob merged commit f46a837 into home-assistant:dev Mar 29, 2019
@ghost ghost removed the in progress label Mar 29, 2019
balloob pushed a commit that referenced this pull request Mar 29, 2019
@syssi @balloob
Fix regression of the xiaomi_aqara config validation (#22435)
9aa5b90 
* Fix regression of the xiaomi_aqara config validation

* Make the key optional again

* Add base schema

* Remove the GW_MAC default
@balloob balloob mentioned this pull request Apr 3, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MartinHjelmare MartinHjelmare MartinHjelmare left review comments

@balloob balloob balloob approved these changes

@amelchio amelchio amelchio approved these changes

@Danielhiversen Danielhiversen Awaiting requested review from Danielhiversen Danielhiversen is a code owner

Assignees

@syssi syssi

Labels
breaking-change cherry-picked cla-signed integration: xiaomi_aqara small-pr PRs with less than 30 lines.
Projects
None yet
Milestone
0.91.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@syssi @karlkar @balloob @MartinHjelmare @amelchio @rohankapoorcom @homeassistant