-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
Conversation
homeassistant/config.py
Outdated
return None | ||
from homeassistant.scripts.check_config import check_ha_config_file | ||
|
||
def _load_hass_yaml_config(): |
There was a problem hiding this comment.
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)
tests/test_config.py
Outdated
@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): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
Not attempted in this PR, but the notification when you call homeassistant/restart can do with some improving: If successful, you get: If failed, you get |
@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 |
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? |
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. |
Description:
Run check_config directly instead of executing the script in a subprocess
Checklist:
If the code does not interact with devices:
tox
run successfully. Your PR cannot be merged unless tests pass