Use PLATFORM_SCHEMA_BASE as base schema for additional components. by emontnemery · Pull Request #20578 · home-assistant/core

emontnemery · 2019-01-29T18:57:37Z

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 · 2019-01-29T18:58:49Z

        """Discover and add a MQTT camera."""
        try:
-            discovery_hash = discovery_payload[ATTR_DISCOVERY_HASH]
+            discovery_hash = discovery_payload.pop(ATTR_DISCOVERY_HASH)


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

emontnemery · 2019-01-29T18:59:06Z

        """Discover and add a MQTT climate device."""
        try:
-            discovery_hash = discovery_payload[ATTR_DISCOVERY_HASH]
+            discovery_hash = discovery_payload.pop(ATTR_DISCOVERY_HASH)


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

emontnemery · 2019-01-29T19:00:15Z

                 default=DEFAULT_AWAY_TIMEOUT): cv.positive_int,
    vol.Optional(CONF_NAME, default=DEFAULT_NAME): cv.string
-})
+}).extend(mqtt.MQTT_RO_PLATFORM_SCHEMA.schema)


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

emontnemery · 2019-01-29T19:01:19Z

 ]

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


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?

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.

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.

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

emontnemery · 2019-01-29T19:02:52Z

                    'method': 'GET',
                    'value_template': '{{ value_json.key }}',
                    'name': 'foo',
-                    'unit_of_measurement': 'MB',


This looks like test case copy/paste error

emontnemery · 2019-01-29T19:03:57Z

    assert hass.states.get(DOMAIN + '.test').state == STATE_OPEN


-async def test_disable_automatic_add(hass, monkeypatch):


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?

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

emontnemery · 2019-01-29T19:04:28Z

            'platform': 'mqtt',
            'name': 'test',
-            'state_topic': 'test-topic',
+            'power_state_topic': 'test-topic',


Copy/paste error.

emontnemery · 2019-01-29T19:04:37Z

            'platform': 'mqtt',
            'name': 'test',
-            'state_topic': 'test-topic',
+            'power_state_topic': 'test-topic',


Copy/paste error.

emontnemery · 2019-01-29T19:05:21Z

                'platform': 'filter',
                'name': 'test',
                'entity_id': 'sensor.test_monitored',
-                'history_period': '00:05',


No history_period configuration variable documented, copy/paste error?

emontnemery · 2019-01-29T19:05:49Z

    )
    for value in options:
-        cv.PLATFORM_SCHEMA(value)
+        cv.PLATFORM_SCHEMA_BASE(value)


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

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

OK, that makes sense.

MartinHjelmare

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 · 2019-01-30T03:43:13Z

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

emontnemery · 2019-01-30T06:56:32Z

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 · 2019-01-30T07:48:30Z

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 · 2019-01-31T01:18:31Z

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

emontnemery · 2019-02-01T14:59:13Z

@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 · 2019-02-02T09:33:04Z

Test added which tests entity_namespace is in PLATFORM_SCHEMA

MartinHjelmare · 2019-02-02T13:02:45Z

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 · 2019-02-02T14:06:20Z

@MartinHjelmare Good idea!

Edit: Done in #20693

emontnemery · 2019-02-04T09:32:44Z

@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

Looks good!

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/core#20578

aidbish · 2019-02-11T07:51:26Z

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 · 2019-02-11T08:12:12Z

This hasn't been released yet.

emontnemery · 2019-02-11T08:40:19Z

@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 requested review from a team and fabaff as code owners January 29, 2019 18:57

homeassistant added the cla-signed label Jan 29, 2019

ghost assigned emontnemery Jan 29, 2019

ghost added the in progress label Jan 29, 2019

emontnemery commented Jan 29, 2019

View reviewed changes

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 reviewed Jan 29, 2019

View reviewed changes

emontnemery added 3 commits February 2, 2019 18:43

Disable extra=vol.ALLOW_EXTRA for additional platforms.

9a0896a

Remove PLATFORM_SCHEMA_2

4c9d4bb

Add entity_namespace to base platform schema

9fa549c

emontnemery force-pushed the component_schema_various_2 branch 2 times, most recently from ffe2153 to 9fa549c Compare February 2, 2019 17:50

MartinHjelmare approved these changes Feb 4, 2019

View reviewed changes

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

ghost removed the in progress label Feb 5, 2019

emontnemery added the breaking-change label Feb 5, 2019

This was referenced Feb 8, 2019

Fix integration with Home Assistant v0.87 david-yam/HeatPump#1

Open

Fix integration with Home Assistant v0.87 SwiCago/HeatPump#110

Merged

emontnemery deleted the component_schema_various_2 branch February 13, 2019 15:57

balloob mentioned this pull request Feb 20, 2019

0.88.0 #21238

Merged

renemarc mentioned this pull request Mar 19, 2019

Add PLATFORM_SCHEMA_BASE to platform daemondazz/homeassistant-displays#8

Merged

robbiet480 mentioned this pull request Mar 22, 2019

Config validation doesn't handle non existing keys #19871

Closed

tomhoover mentioned this pull request Mar 26, 2019

Platform configuration will no longer allow to contain keys that are not supported (0.88) tomhoover/home-assistant_config#38

Closed

		assert hass.states.get(DOMAIN + '.test').state == STATE_OPEN


		async def test_disable_automatic_add(hass, monkeypatch):

Uh oh!

Conversation

emontnemery commented Jan 29, 2019 • edited by balloob Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emontnemery Jan 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare Jan 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

balloob commented Jan 30, 2019

Uh oh!

emontnemery commented Jan 30, 2019

Uh oh!

MartinHjelmare commented Jan 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

balloob commented Jan 31, 2019

Uh oh!

emontnemery commented Feb 1, 2019

Uh oh!

emontnemery commented Feb 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MartinHjelmare commented Feb 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emontnemery commented Feb 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emontnemery commented Feb 4, 2019

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

aidbish commented Feb 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MartinHjelmare commented Feb 11, 2019

Uh oh!

emontnemery commented Feb 11, 2019

Uh oh!

emontnemery commented Jan 29, 2019 •

edited by balloob

Loading

emontnemery Jan 29, 2019 •

edited

Loading

MartinHjelmare Jan 29, 2019 •

edited

Loading

MartinHjelmare commented Jan 30, 2019 •

edited

Loading

emontnemery commented Feb 2, 2019 •

edited

Loading

MartinHjelmare commented Feb 2, 2019 •

edited

Loading

emontnemery commented Feb 2, 2019 •

edited

Loading

aidbish commented Feb 11, 2019 •

edited

Loading