Add option to use frontend PR artifact to frontend integration - #161291
Conversation
Use dictionary mapping for HTTP error messages to eliminate code duplication. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Move GitHub error messages to module-level constants to avoid duplication across _get_pr_head_sha and _download_artifact_data functions. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Hey there @home-assistant/frontend, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
|
Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
Replace sync PyGithub with aiogithubapi and requests with aiohttp. - Use aiogithubapi.GitHubAPI for GitHub API calls - Use aiohttp_client.async_get_clientsession for HTTP downloads - Remove executor job calls for API operations (now fully async) - Keep executor jobs only for blocking I/O (file operations, zip extraction) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Use specific aiogithubapi exception types instead of checking status attribute - Handle GitHubAuthenticationException for 401 errors - Handle GitHubRatelimitException and GitHubPermissionException for 403 errors - Handle GitHubNotFoundException for 404 errors - Update tests to use aiogithubapi and aiohttp mocks - Replace PyGithub mocks with GitHubAPI.generic() mocks - Replace requests mocks with aioclient_mock (AiohttpClientMocker) - Add test for GitHub API error handling Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Why isn't it enough to checkout the frontend PR and use the |
This should be an option for HAOS instances. For example the product team can easily test changes without the need to setup a dev env. |
|
Please add this context to the PR description. |
emontnemery
left a comment
There was a problem hiding this comment.
Looks pretty good.
Some comments.
I've not reviewed the tests.
- Added validation for GitHub token requirement when development PR is set. - Refactored frontend configuration schema to include validation. - Improved test coverage for PR download scenarios, ensuring proper handling of GitHub token absence and cache usage.
…improve error handling for PR downloads
| def cache_path(self, *path: str) -> str: | ||
| """Generate path to the file within the cache directory. | ||
|
|
||
| The cache directory is used for temporary data that can be | ||
| regenerated and is not included in backups. | ||
|
|
||
| Async friendly. | ||
| """ | ||
| return self.path(".cache", *path) |
There was a problem hiding this comment.
Let's add some tests of this method, it's fine to just duplicate the tests for the path method above:
core/tests/test_core_config.py
Lines 869 to 883 in 9c07550
| requires-python = ">=3.13.2" | ||
| dependencies = [ | ||
| "aiodns==4.0.0", | ||
| "aiogithubapi==24.6.0", |
There was a problem hiding this comment.
| "aiogithubapi==24.6.0", | |
| # aiogithubapi is needed by frontend | |
| "aiogithubapi==24.6.0", |
There was a problem hiding this comment.
Please explain why it's needed by frontend and can't be set as a manifest requirement.
There was a problem hiding this comment.
As far as I understand it: we import pr_download top level in frontend and pr_download imports aiogithubapi. This happens before manifest requirements are installed and so aiogithubapi wouldn't be available in pr_download. I initially loaded pr_download lazy in the setup but @emontnemery asked me to import it globally.
There was a problem hiding this comment.
What's special about the frontend integration in this regard? Other integrations can import 3rd party libraries top level if they specify them in the integration manifest requirements.
There was a problem hiding this comment.
frontend is a stage 0 integration in bootstrap.py, so it's loaded before setup.py installs req from manifest. A regular integration is loaded after setup.py did this installations.
If it's the wrong way of doing it please tell me how I should implement it.
There was a problem hiding this comment.
When I remove aiogithubapi from pyproject, it removes it from requirements.txt. When I now start it with just requirements.txt installed it crashes. I think this is how HA works: base req are installed from requirements.txt and all in requirements_all.txt is installed on demand. But this on demand happens after frontend setups.
The crash:
Traceback (most recent call last):
File "/home/wendelin/ohf/home-assistant/core/.venv/bin/hass", line 10, in <module>
sys.exit(main())
~~~~^^
File "/home/wendelin/ohf/home-assistant/core/homeassistant/__main__.py", line 188, in main
from . import config, runner # noqa: PLC0415
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/wendelin/ohf/home-assistant/core/homeassistant/runner.py", line 27, in <module>
from . import bootstrap
File "/home/wendelin/ohf/home-assistant/core/homeassistant/bootstrap.py", line 41, in <module>
from .components import (
...<22 lines>...
)
File "/home/wendelin/ohf/home-assistant/core/homeassistant/components/config/__init__.py", line 5, in <module>
from homeassistant.components import frontend
File "/home/wendelin/ohf/home-assistant/core/homeassistant/components/frontend/__init__.py", line 40, in <module>
from .pr_download import download_pr_artifact
File "/home/wendelin/ohf/home-assistant/core/homeassistant/components/frontend/pr_download.py", line 11, in <module>
from aiogithubapi import (
...<6 lines>...
)
ModuleNotFoundError: No module named 'aiogithubapi'
I had the issue only in the CI, because on my system I have all req from tests. Erik told me then to add it to pyproject.toml to automatically add it to requirements.txt.
I don't think there is another way of adding it.
There was a problem hiding this comment.
It's not a problem with setup or on demand installation requirements. It's a problem with imports and the weird way we have set dependencies in frontend at the moment. Bootstrap pre-imports integrations that frontend has set as dependencies as a performance optimization, but then those integrations import from frontend. Normally that's totally incorrect.
I'll look at this separately, later.
There was a problem hiding this comment.
Okay so you'll come back when you have a solution? Or can we merge this and fix this import problem in a separate PR?
There was a problem hiding this comment.
We can address that separately. I'm not sure how easy it is. Let's keep it like this here for now.
There was a problem hiding this comment.
First of all, the reason why frontend has integrations in its dependencies which it doesn't depend on is to ensure non-optional HTTP/WS endpoints are available when the webserver is started. Relevant PR is #139199 which removed the backup integration from frontend's dependencies and also has a not yet implemented proposal for how we can remove some/most of the other dependencies:
Going forward, we should add a way for frontend to check which integrations will be setup and a way for it to wait for an integration to set up.
In this PR it's a different problem, we're now adding a new library requirement to frontend itself. Specifying the github API in frontend's manifest.json doesn't break core startup, that still works fine without any issue (provided that the aiogithubapi is installed).
What does NOT work fine however is calculating and validating the dependencies.
Without aiogithubapi installed, this is the output from the "Prepare dependencies" CI-job:
Traceback (most recent call last):
File "<frozen runpy>", line 198, in _run_module_as_main
File "<frozen runpy>", line 88, in _run_code
File "/home/runner/work/core/core/script/gen_requirements_all.py", line 649, in <module>
sys.exit(main(_VAL, _CI))
~~~~^^^^^^^^^^^
File "/home/runner/work/core/core/script/gen_requirements_all.py", line 581, in main
data = gather_modules()
File "/home/runner/work/core/core/script/gen_requirements_all.py", line 346, in gather_modules
gather_requirements_from_modules(errors, reqs)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^
File "/home/runner/work/core/core/script/gen_requirements_all.py", line 399, in gather_requirements_from_modules
explore_module("homeassistant.scripts", True)
~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/runner/work/core/core/script/gen_requirements_all.py", line 260, in explore_module
module = importlib.import_module(package)
File "/opt/hostedtoolcache/Python/3.13.11/x64/lib/python3.13/importlib/__init__.py", line 88, in import_module
return _bootstrap._gcd_import(name[level:], package, level)
~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "<frozen importlib._bootstrap>", line 1387, in _gcd_import
File "<frozen importlib._bootstrap>", line 1360, in _find_and_load
File "<frozen importlib._bootstrap>", line 1331, in _find_and_load_unlocked
File "<frozen importlib._bootstrap>", line 935, in _load_unlocked
File "<frozen importlib._bootstrap_external>", line 1023, in exec_module
File "<frozen importlib._bootstrap>", line 488, in _call_with_frames_removed
File "/home/runner/work/core/core/homeassistant/scripts/__init__.py", line 13, in <module>
from homeassistant import runner
File "/home/runner/work/core/core/homeassistant/runner.py", line 27, in <module>
from . import bootstrap
File "/home/runner/work/core/core/homeassistant/bootstrap.py", line 41, in <module>
from .components import (
...<22 lines>...
)
File "/home/runner/work/core/core/homeassistant/components/config/__init__.py", line 5, in <module>
from homeassistant.components import frontend
File "/home/runner/work/core/core/homeassistant/components/frontend/__init__.py", line 40, in <module>
from .pr_download import download_pr_artifact
File "/home/runner/work/core/core/homeassistant/components/frontend/pr_download.py", line 11, in <module>
from aiogithubapi import (
...<6 lines>...
)
ModuleNotFoundError: No module named 'aiogithubapi'
If running the generate requirements script with aiogithubapi available, aiogithubapi is however not promoted from requirements_all.txt to requirements.txt, and there's not a warning from hassfest.
Co-authored-by: Erik Montnemery <erik@montnemery.com>
… into frontend-pr-download
emontnemery
left a comment
There was a problem hiding this comment.
LGTM, thanks @wendevlin 👍
|
Shouldn't this be documented in the dev docs since it's aimed for development? |
|
There are unresolved comments above. |
I am not sure where to put it, because it's not to develop features, it is to easier test frontend PRs. I think it would make more sense to get into more detail in the integration docs. @MindFreeze what do you think? |
|
I would say it's still mostly a development feature as users would have to be on Github to find a PR to test. Suggest to explain it in the dev docs and add a brief mention that it's possible in https://www.home-assistant.io/integrations/frontend/ with a link to the dev docs. |
|
I added dev docs and updated the user docs to reference to the dev docs. |
emontnemery
left a comment
There was a problem hiding this comment.
Let's clarify the comment in pyproject.toml
Co-authored-by: Erik Montnemery <erik@montnemery.com>
|
I discussed this a bit with @MartinHjelmare on Discord, he has no further objections which blocks this PR so I'll go ahead and merge it. |
Proposed change
development_proption to the frontend integration to easily try out PRsThis should be an option for HAOS instances. For example the product team can easily test changes without the need to setup a dev env.
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: