check_config script evolution by kellerza · Pull Request #12792 · home-assistant/core

kellerza · 2018-02-28T21:38:06Z

Description:

The check_config script #2657 relies on some serious patching to test user configuration. It certainly helped me to understand unit testing... In #5609 it got incorporated into services/check upon restart, spawning another process and capturing the script output. It works, but it's slow... this also contains a record amount of slowest entries in all Travis jobs.

This PR will ~~replace async_check_ha_config_file used by the check_config service,~~ removing the need for patches, or the speed penalty of the current script method.

Using the commandline script you can still access ---files, --secrets and --info all to print options. These however still require patching to extract the interesting parts from the yaml processor.

Test behaviour using:

hass --script check_config

Todo:

Add the actual validation of the component and platform SCHEMAs.
Add some tests
~~Eventually make --new the standard and remove the old parts of the script.~~

OttoWinter · 2018-02-28T21:50:19Z

homeassistant/scripts/check_config.py

Can be simplified :)

parser.add_argument('--new', action='store_true', help="...")

Thanks, it will be removed eventually, but good to know :-)

balloob · 2018-02-28T23:19:35Z

homeassistant/config.py

No need to copy it here, we just created the dict.

balloob · 2018-02-28T23:20:06Z

homeassistant/config.py

Please don't call it "new", that name doesn't age…

It will completely replace the function above without the "_new" once done

balloob · 2018-02-28T23:21:00Z

homeassistant/config.py

Use config.pop(CONF_CORE, {}) to remove it from the config dict.

balloob · 2018-02-28T23:21:13Z

homeassistant/config.py

stale print

balloob · 2018-02-28T23:22:04Z

homeassistant/config.py

Config entries are a complete standalone configuration channel from normal config. They also don't have validation as they are read and written by code.

balloob · 2018-02-28T23:23:07Z

homeassistant/scripts/check_config.py

Why would we even need to instantiate hass to check configuration? That is not necessary.

Indeed, only need loop and config

Why would you need loop or config? You really only need config path.

You only need loop if you plan on using await, but just validating some dicts doesn't require any awaits.

I was thinking load_yaml_config_file should actually be awaited, sice it does IO?
But otherwise there is no IO (except for importing all the plaforms, which I would think also needs awaiting)

balloob · 2018-02-28T23:24:23Z

May I suggest you take a slightly different approach to this problem:

Build a standalone config checker (without hass dependency) from scratch that validates CONFIG_SCHEMA and PLATFORM_SCHEMA without trying to share any code with the current bootstrap process (just copy paste the pieces from bootstrap.py and setup.py). Then afterwards, try to see which pieces of code can be shared.

kellerza · 2018-03-01T07:03:04Z

This is the framework for *_SCHEMA validation.

The standalone checker will be a single method in config.py called: async def check_ha_config_file(onfig_dir), it will:

load config using from hass.config.core_dir
merge packages
run all *_Schema validation
Returns ~~a list of issues, or None/[]~~ (success, failed)

This method can then be (a) used by async_check_ha_config_file [new PR] or (b) by check_config script, which will still provide the config display tools

There is no other easy way to create the scaffolding to test it on, since I'll have to copy almost the complete current check_config.py, 90% if that code is displaying and the various comdline switches, but required to test this new version. This allows to switch between existing&new behaviour during testing and to try retain all the current options from check_config.

I can start that work as a second commit to this PR (preferred) or as another PR based of this one?

balloob · 2018-03-01T22:31:11Z

If you pass in hass.config.core_dir, you don't require a hass instance at all for validation.

You can do it as a second commit to this PR.

kellerza · 2018-03-02T20:57:47Z

homeassistant/bootstrap.py

    conf_util.merge_packages_config(
        config, core_config.get(conf_util.CONF_PACKAGES, {}))

-    # Make a copy because we are mutating it.


This was already mutated by merge_packages in the past. Loading will ensure values are {} and never None

kellerza · 2018-03-02T21:01:44Z

homeassistant/config.py

+
+    # Merge packages
+    with patch('homeassistant.config._log_pkg_error', side_effect=_pack_error):
+        merge_packages_config(config, core_config.get(CONF_PACKAGES, {}))


Patching the _log_pkg_error else I have to rewrite the complete method.

Another approach is to add the logging function to the funtion definition

def merge_packages_config(config, packages, _log_pkg_error=_log_pkg_error):

and then call it with _log_pkg_error=_pack_error

passing method in is a 1000x better solution than patching.

balloob · 2018-03-02T23:17:31Z

homeassistant/config.py


+def check_ha_config_file(config_dir):
+    """Check if Home Assistant configuration file is valid."""
+    from unittest.mock import patch


We should never use the unittest module in normal code. It is a big red flag of poor design.

Suggested another aproach at the patch...

Yes, I saw it later. Like it!

balloob · 2018-03-02T23:20:58Z

homeassistant/config.py

+            return {}, [("File configuration.yaml not found.", None, None)]
+        config = load_yaml_config_file(config_path)
+    except HomeAssistantError as err:
+        return {}, [(str(err), None, None)]


Let's not return tuples, they are confusing and also mean that any code depending on this function will need to be updated if we want to add another parameter. Please consider using a namedtuple or an attr class.

I like the idea behind namedtuples. The current double tuple approach can be confusing, especially in the unit tests

kellerza · 2018-03-03T07:23:08Z

Thanks for the feedback @balloob. I think this is turning out quite well now, check_ha_config_file returns an OrderedDict with an errors attribute containing a namedtuple with the error messages and config snippets.

In this form it can even become a drop in replacement for loading config, validated and errors removed. (something says there might still be some border cases when we have no SCHEMA defined in components/platforms, but can't think of an example now, maybe automation...)

kellerza · 2018-03-05T22:17:42Z

This should address flaky testconfig tests from #10966

balloob · 2018-03-06T06:46:15Z

homeassistant/config.py

    return None


+def check_ha_config_file(config_dir):


Can we keep this code inside the check config script for now?

balloob · 2018-03-06T06:47:18Z

homeassistant/config.py

+    CheckConfigError = namedtuple(  # pylint: disable=invalid-name
+        'CheckConfigError', "message domain config")
+
+    class ConfigResult(OrderedDict):


We should not define this inside function

Will also move it to check_config.py.

@pvizeli not sure I'm following you?

I think he means using the new attrs library which has recently been added as a global requirement (home-assistant/architecture#4)

Ok, thanks. And @attr.s will call super() for me?

kellerza · 2018-03-06T20:01:38Z

Ready for review (Coveralls seems to be down)

balloob

Oh this is so much better 😍 🐬

* Initial async_check_ha_config_file * check_ha_config_file * Various fixes * feedback - return the config * move_to_check_config

kellerza requested a review from a team as a code owner February 28, 2018 21:38

homeassistant added cla-signed core labels Feb 28, 2018

OttoWinter reviewed Feb 28, 2018

View reviewed changes

balloob reviewed Feb 28, 2018

View reviewed changes

homeassistant/config.py Outdated

Copy link
Copy Markdown

Member

balloob Feb 28, 2018

Choose a reason for hiding this comment

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

stale print

kellerza reacted with thumbs up emoji

balloob reviewed Feb 28, 2018

View reviewed changes

kellerza added 2 commits March 2, 2018 22:47

Initial async_check_ha_config_file

78717e2

check_ha_config_file

43f32a3

kellerza force-pushed the check_config2 branch from 797e111 to 43f32a3 Compare March 2, 2018 20:49

homeassistant added the cla-signed label Mar 2, 2018

kellerza commented Mar 2, 2018

View reviewed changes

Various fixes

3440466

kellerza changed the title ~~WIP: check_config script evolution~~ check_config script evolution Mar 2, 2018

balloob reviewed Mar 2, 2018

View reviewed changes

feedback - return the config

62df22d

homeassistant added the cla-signed label Mar 3, 2018

balloob reviewed Mar 6, 2018

View reviewed changes

move_to_check_config

d01322b

homeassistant added the cla-signed label Mar 6, 2018

balloob approved these changes Mar 9, 2018

View reviewed changes

balloob added this to the 0.65 milestone Mar 9, 2018

balloob merged commit 6734c96 into home-assistant:dev Mar 9, 2018

kellerza deleted the check_config2 branch March 9, 2018 06:15

balloob pushed a commit that referenced this pull request Mar 9, 2018

check_config script evolution (#12792)

16d72d2

* Initial async_check_ha_config_file * check_ha_config_file * Various fixes * feedback - return the config * move_to_check_config

balloob mentioned this pull request Mar 9, 2018

0.65 #12995

Merged

This was referenced Mar 9, 2018

Python_script and config errors in 0.65 #13015

Closed

Use load_yaml_config_file in config component #13015 #13023

Closed

home-assistant locked and limited conversation to collaborators Jul 26, 2018

Uh oh!

Conversation

kellerza commented Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Todo:

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balloob commented Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kellerza commented Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

balloob commented Mar 1, 2018

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!

kellerza commented Mar 3, 2018

Uh oh!

kellerza commented Mar 5, 2018

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!

OttoWinter Mar 6, 2018 • 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!

kellerza commented Mar 6, 2018

Uh oh!

balloob left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

kellerza commented Feb 28, 2018 •

edited

Loading

balloob commented Feb 28, 2018 •

edited

Loading

kellerza commented Mar 1, 2018 •

edited

Loading

OttoWinter Mar 6, 2018 •

edited

Loading