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

Perform check_config service in current process #13017

Merged
merged 2 commits into from
Mar 19, 2018

Conversation

kellerza
Copy link
Member

@kellerza kellerza commented Mar 9, 2018

Description:

Run check_config directly instead of executing the script in a subprocess

Checklist:

  • The code change is tested and works locally.

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

@kellerza kellerza requested a review from a team as a code owner March 9, 2018 20:22
@homeassistant homeassistant added cla-signed core small-pr PRs with less than 30 lines. labels Mar 9, 2018
return None
from homeassistant.scripts.check_config import check_ha_config_file

def _load_hass_yaml_config():
Copy link
Member

Choose a reason for hiding this comment

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

There is no need fo this, just add the 2 methods to async_add_job:

res = await hass.async_add_job(check_ha_config_file, hass.config.config_dir)

@mock.patch('asyncio.create_subprocess_exec')
def test_check_ha_config_file_correct(self, mock_create):
@mock.patch('os.path.isfile', return_value=True)
def test_check_ha_config_file_correct(self, mock_file):
Copy link
Member

Choose a reason for hiding this comment

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

We already have tests for check_ha_config_file so we should stub that out here and just have it return a dummy error log.

Copy link
Member

Choose a reason for hiding this comment

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

We should try to do 0 I/O inside our tests. I/O in tests make them unreliable and slow (especially on Travis).

@kellerza
Copy link
Member Author

Not attempted in this PR, but the notification when you call homeassistant/restart can do with some improving:

If successful, you get: Failed to call service homeassistant/restarts. and it restarts.

If failed, you get Service homeassistant/restart called. and nothing else --> check the log and you get a list of the errors

@kellerza kellerza changed the title WIP: Perform check_config service in current process Perform check_config service in current process Mar 18, 2018
@balloob balloob merged commit 4270bc7 into home-assistant:dev Mar 19, 2018
@kellerza kellerza deleted the ckservice branch March 20, 2018 07:04
@balloob balloob mentioned this pull request Mar 30, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Jul 26, 2018
@balloob
Copy link
Member

balloob commented Apr 28, 2019

@kellerza do you remember why we did this?

I want to reverse this, as the core should not have a dependency on a script. Nor is this script something that can currently run inside the HASS process, because it mocks out a bunch of things that can have unintended side effects (like mocking out !secret)

@kellerza
Copy link
Member Author

Someone performed a hass check_config from the gui “restart” request, which was terribly slow and this started this (although forking does not interfere with the current process)

Technically we don't need to mock anything for a quick config check and I have already split the check_config script in 2 parts. One for a quick check- almost no mocks(target is none) and then a more advanced version that mocks secrets and some parts of the loader to print info and secret usage

If you want I can complete this work and split check_config to homeassistant.helpers.check_config.py?

@balloob
Copy link
Member

balloob commented Apr 28, 2019

That would be great. I was looking at it last night, and indeed a lot of code can be re-used.

Still not sure how you would do the secrets part though? Maybe we can leave that for the very last PR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-signed core small-pr PRs with less than 30 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants