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

Use PLATFORM_SCHEMA_BASE as base schema for additional components. #20578

Merged
merged 3 commits into from Feb 5, 2019

Conversation

Projects
None yet
6 participants
@emontnemery
Copy link
Contributor

emontnemery commented Jan 29, 2019

Edit by @balloob: Breaking change should be listed in main release notes. Not just breaking changes section.


Description:

  • Use PLATFORM_SCHEMA_BASE as base schema for remaining components.
    PLATFORM_SCHEMA_BASE doesn't set extra=vol.ALLOW_EXTRA which means misspelled or unknown configuration variables will no longer be silently accepted.

  • Remove PLATFORM_SCHEMA_2 and remove extra=vol.ALLOW_EXTRA from PLATFORM_SCHEMA

#19871 should now be fixed.

This is a follow-up to #20224 .

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.

@emontnemery emontnemery requested review from fabaff and home-assistant/core as code owners Jan 29, 2019

@wafflebot wafflebot bot added the in progress label Jan 29, 2019

@@ -47,7 +48,9 @@
async def async_discover(discovery_payload):
"""Discover and add a MQTT camera."""
try:
discovery_hash = discovery_payload[ATTR_DISCOVERY_HASH]
discovery_hash = discovery_payload.pop(ATTR_DISCOVERY_HASH)

This comment has been minimized.

@emontnemery

emontnemery Jan 29, 2019

Author Contributor

Pop discovery_hash from the config dict since this is used by MQTT discovery, and not part of the schema.

@@ -156,7 +156,9 @@
async def async_discover(discovery_payload):
"""Discover and add a MQTT climate device."""
try:
discovery_hash = discovery_payload[ATTR_DISCOVERY_HASH]
discovery_hash = discovery_payload.pop(ATTR_DISCOVERY_HASH)

This comment has been minimized.

@emontnemery

emontnemery Jan 29, 2019

Author Contributor

Pop discovery_hash from the config dict since this is used by MQTT discovery, and not part of the schema.

vol.Required(CONF_TIMEOUT, default=DEFAULT_TIMEOUT): cv.positive_int,
vol.Optional(CONF_AWAY_TIMEOUT,
default=DEFAULT_AWAY_TIMEOUT): cv.positive_int,
vol.Optional(CONF_NAME, default=DEFAULT_NAME): cv.string
})
}).extend(mqtt.MQTT_RO_PLATFORM_SCHEMA.schema)

This comment has been minimized.

@emontnemery

emontnemery Jan 29, 2019

Author Contributor

The platform schema should extend standard MQTT schema which includes state_topic, qos, etc.

@@ -630,6 +630,7 @@ def _get_attributes(rest):
]

PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
vol.Optional(CONF_ENTITY_NAMESPACE): cv.string,

This comment has been minimized.

@emontnemery

emontnemery Jan 29, 2019

Author Contributor

entity_namespace is used in one of wunderground test cases.
Not sure if it's correct to add it here, or if the testcase is stale?

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jan 29, 2019

Member

I think we should add it to the base platform schema. It's an option used in the entity component code, so all EntityPlatforms are affected.

This comment has been minimized.

@emontnemery

emontnemery Jan 29, 2019

Author Contributor

According to the docs:

These options are being phased out and are only available for single platform integrations.

But sure, maybe makes sense to add to the base schema.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jan 29, 2019

Member

Yes, but now we're talking about potentially breaking all platforms in one go. So it's needs to be a conscious decision.

This comment has been minimized.

@emontnemery

emontnemery Jan 30, 2019

Author Contributor

OK, fixed.

@@ -99,7 +99,6 @@ def test_setup_get(self, mock_req):
'method': 'GET',
'value_template': '{{ value_json.key }}',
'name': 'foo',
'unit_of_measurement': 'MB',

This comment has been minimized.

@emontnemery

emontnemery Jan 29, 2019

Author Contributor

This looks like test case copy/paste error

@@ -416,33 +416,6 @@ def listener(event):
assert hass.states.get(DOMAIN + '.test').state == STATE_OPEN


async def test_disable_automatic_add(hass, monkeypatch):

This comment has been minimized.

@emontnemery

emontnemery Jan 29, 2019

Author Contributor

automatic_add is not supported for RFLink cover so remove the test.
@javicalle Maybe you can confirm it's correct to remove the test case?

This comment has been minimized.

@javicalle

javicalle Jan 29, 2019

Contributor

I think you're right
I have reviewed the cover component and it is as you say.
Sorry for the inconvenience.

@@ -679,7 +679,8 @@ def test_temp_step_custom(self):
climate.DOMAIN: {
'platform': 'mqtt',
'name': 'test',
'state_topic': 'test-topic',
'power_state_topic': 'test-topic',

This comment has been minimized.

@emontnemery

emontnemery Jan 29, 2019

Author Contributor

Copy/paste error.

@@ -697,7 +698,8 @@ def test_temp_step_custom(self):
climate.DOMAIN: {
'platform': 'mqtt',
'name': 'test',
'state_topic': 'test-topic',
'power_state_topic': 'test-topic',

This comment has been minimized.

@emontnemery

emontnemery Jan 29, 2019

Author Contributor

Copy/paste error.

@@ -59,7 +59,6 @@ def test_chain(self):
'platform': 'filter',
'name': 'test',
'entity_id': 'sensor.test_monitored',
'history_period': '00:05',

This comment has been minimized.

@emontnemery

emontnemery Jan 29, 2019

Author Contributor

No history_period configuration variable documented, copy/paste error?

@@ -110,7 +110,7 @@ def test_platform_config():
{'platform': 'mqtt', 'beer': 'yes'},
)
for value in options:
cv.PLATFORM_SCHEMA(value)
cv.PLATFORM_SCHEMA_BASE(value)

This comment has been minimized.

@emontnemery

emontnemery Jan 29, 2019

Author Contributor

I'm not so sure about this one.
What's the point of this test case?

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Jan 29, 2019

Member

I think it's that the base platform schema should require 'platform' key, and allow extra.

This comment has been minimized.

@emontnemery

emontnemery Jan 29, 2019

Author Contributor

OK, that makes sense.

@emontnemery emontnemery changed the title Use PLATFORM_SCHEMA_BASE as base schema for additional platforms. Use PLATFORM_SCHEMA_BASE as base schema for additional components. Jan 29, 2019

@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

I've looked it over and it looks good besides the entity namespace key. We probably need to add that to PLATFORM_SCHEMA in config_validation.py.

@balloob should comment on that.

@balloob

This comment has been minimized.

Copy link
Member

balloob commented Jan 30, 2019

Yeah, we should add it to the base schema validation.

@emontnemery

This comment has been minimized.

Copy link
Contributor Author

emontnemery commented Jan 30, 2019

OK, fixed now.

I'm also a bit worried about breaking platforms which don't have tests and where configuration keys are missing from the schema. Maybe that simply has to be worked out in the beta cycle?

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Jan 30, 2019

We should add a test for the entity namespace key, since it's a global option. Platform specific options aren't that dangerous. So I think it's ok to go ahead with this, after adding that test. We're hopefully improving config validation this way.

@balloob

This comment has been minimized.

Copy link
Member

balloob commented Jan 31, 2019

Not having this go into the next release, 86 was a bit rough on config validation already.

@emontnemery

This comment has been minimized.

Copy link
Contributor Author

emontnemery commented Feb 1, 2019

@MartinHjelmare I'm really not sure how to write the new test..

As discussed on discord, this test tests entity_namespace but it just doesn't validate the config, so I think it's a start.

How to rewrite it to use async_setup_component though?

@emontnemery

This comment has been minimized.

Copy link
Contributor Author

emontnemery commented Feb 2, 2019

Test added which tests entity_namespace is in PLATFORM_SCHEMA

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Feb 2, 2019

Should we separate entity namespace key addition to a separate PR that we include in 0.87, to avoid breaking any alarm_control_panel platforms?

@emontnemery

This comment has been minimized.

Copy link
Contributor Author

emontnemery commented Feb 2, 2019

@MartinHjelmare Good idea!

Edit: Done in #20693

@emontnemery emontnemery force-pushed the emontnemery:component_schema_various_2 branch 2 times, most recently from ffe2153 to 9fa549c Feb 2, 2019

@emontnemery

This comment has been minimized.

Copy link
Contributor Author

emontnemery commented Feb 4, 2019

@balloob

Not having this go into the next release, 86 was a bit rough on config validation already.

So can this be merged now that 0.87 beta is cut?

@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

Looks good!

@balloob balloob merged commit b1faad0 into home-assistant:dev Feb 5, 2019

5 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.005%) to 93.354%
Details

@wafflebot wafflebot bot removed the in progress label Feb 5, 2019

david-yam added a commit to david-yam/HeatPump that referenced this pull request Feb 8, 2019

Fix integration with Home Assistant v0.87
Fix the configuration validation error issue due to Home Assistant disallow arbitrary configuration attributes in MQTT platforms.  See the issue log of HA for details: home-assistant/home-assistant#20578
@aidbish

This comment has been minimized.

Copy link

aidbish commented Feb 11, 2019

Hi awesome dev folks,
Would it be prudent with this potentially breaking a lot of configs, to be a lot more visible in the release notes. The MQTT changes recently seem to have been missed by a fair few people from comments on the community pages/FB/reddit etc. Just a thought?

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Feb 11, 2019

This hasn't been released yet.

@emontnemery

This comment has been minimized.

Copy link
Contributor Author

emontnemery commented Feb 11, 2019

@MartinHjelmare I think it's a request to make this change extra visible in the future 0.88 release notes, to increase the chance that users take notice.

@emontnemery emontnemery deleted the emontnemery:component_schema_various_2 branch Feb 13, 2019

@balloob balloob referenced this pull request Feb 20, 2019

Merged

0.88.0 #21238

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