Repository: home-assistant/core
Pull request: Adjust hassfest.manifest based on config.action
## Proposed change
Currently, when calling `hassfest.manifest.validate`, once we retrieve all the integration manifests that were not sorted, `hassfest` will try to run prettier on the corresponding integrations, regardless of whether the mode `config.action` is `generate` or `validate`. Currently, when running `hassfest` as a GitHub action, which runs the tool in `validate` mode, improperly sorted manifests leads to the error below because the action's container doesn't have prettier installed.

In discussion with @ludeeus , we should instead have branching logic based on the `config.action` so that we only take the steps that are needed. In the case of `validate`, which is what the GitHub action uses, there is no reason to run prettier because we aren't actually generating new files. While this was done to fix the GH action, this is a general improvement to the tool anyway



```python
Traceback (most recent call last):
  File "<frozen runpy>", line 198, in _run_module_as_main
  File "<frozen runpy>", line 88, in _run_code
  File "/usr/src/homeassistant/script/hassfest/__main__.py", line 244, in <module>
    sys.exit(main())
             ^^^^^^
  File "/usr/src/homeassistant/script/hassfest/__main__.py", line 166, in main
    plugin.validate(integrations, config)
  File "/usr/src/homeassistant/script/hassfest/manifest.py", line 393, in validate
    subprocess.run(
  File "/usr/local/lib/python3.11/subprocess.py", line 571, in run
    raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '['pre-commit', 'run', '--hook-stage', 'manual', 'prettier', '--files', PosixPath('/github/workspace/custom_components/my_integration/manifest.json')]' returned non-zero exit status 1.
Validating manifest...
```

## Type of change
<!--
  What type of change does your PR introduce to Home Assistant?
  NOTE: Please, check only 1! box!
  If your PR requires multiple boxes to be checked, you'll most likely need to
  split it into multiple PRs. This makes things easier and faster to code review.
-->

- [ ] Dependency upgrade
- [x] Bugfix (non-breaking change which fixes an issue)
- [ ] New integration (thank you!)
- [ ] New feature (which adds functionality to an existing integration)
- [ ] Deprecation (breaking change to happen in the future)
- [ ] Breaking change (fix/feature causing existing functionality to break)
- [ ] Code quality improvements to existing code or addition of tests

## Additional information
<!--
  Details are important, and help maintainers processing your PR.
  Please be sure to fill out additional details, if applicable.
-->

- This PR fixes or closes issue: fixes https://github.com/home-assistant/actions/issues/95
- This PR is related to issue: 
- Link to documentation pull request: 

## Checklist
<!--
  Put an `x` in the boxes that apply. You can also fill these out after
  creating the PR. If you're unsure about any of them, don't hesitate to ask.
  We're here to help! This is simply a reminder of what we are going to look
  for before merging your code.
-->

- [ ] The code change is tested and works locally.
- [ ] Local tests pass. **Your PR cannot be merged unless tests pass**
- [ ] There is no commented out code in this PR.
- [ ] I have followed the [development checklist][dev-checklist]
- [ ] I have followed the [perfect PR recommendations][perfect-pr]
- [ ] The code has been formatted using Black (`black --fast homeassistant tests`)
- [ ] Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

- [ ] Documentation added/updated for [www.home-assistant.io][docs-repository]

If the code communicates with devices, web services, or third-party tools:

- [ ] The [manifest file][manifest-docs] has all fields filled out correctly.  
      Updated and included derived files by running: `python3 -m script.hassfest`.
- [ ] New or updated dependencies have been added to `requirements_all.txt`.  
      Updated by running `python3 -m script.gen_requirements_all`.
- [ ] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
- [ ] Untested files have been added to `.coveragerc`.

<!--
  This project is very active and we have a high turnover of pull requests.

  Unfortunately, the number of incoming pull requests is higher than what our
  reviewers can review and merge so there is a long backlog of pull requests
  waiting for review. You can help here!
  
  By reviewing another pull request, you will help raise the code quality of
  that pull request and the final review will be faster. This way the general
  pace of pull request reviews will go up and your wait time will go down.
  
  When picking a pull request to review, try to choose one that hasn't yet
  been reviewed.

  Thanks for helping out!
-->

To help with the load of incoming pull requests:

- [ ] I have reviewed two other [open pull requests][prs] in this repository.

[prs]: https://github.com/home-assistant/core/pulls?q=is%3Aopen+is%3Apr+-author%3A%40me+-draft%3Atrue+-label%3Awaiting-for-upstream+sort%3Acreated-desc+review%3Anone+-status%3Afailure

<!--
  Thank you for contributing <3

  Below, some useful links you could explore:
-->
[dev-checklist]: https://developers.home-assistant.io/docs/development_checklist/
[manifest-docs]: https://developers.home-assistant.io/docs/creating_integration_manifest/
[quality-scale]: https://developers.home-assistant.io/docs/integration_quality_scale_index/
[docs-repository]: https://github.com/home-assistant/home-assistant.io
[perfect-pr]: https://developers.home-assistant.io/docs/review-process/#creating-the-perfect-pr


Problem statement:
Issue with manifest key sort validation?
In #devs, someone reported that they were getting the following error when running the action:
```
Traceback (most recent call last):
  File "<frozen runpy>", line 198, in _run_module_as_main
  File "<frozen runpy>", line 88, in _run_code
  File "/usr/src/homeassistant/script/hassfest/__main__.py", line 244, in <module>
    sys.exit(main())
             ^^^^^^
  File "/usr/src/homeassistant/script/hassfest/__main__.py", line 166, in main
    plugin.validate(integrations, config)
  File "/usr/src/homeassistant/script/hassfest/manifest.py", line 393, in validate
    subprocess.run(
  File "/usr/local/lib/python3.11/subprocess.py", line 571, in run
    raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '['pre-commit', 'run', '--hook-stage', 'manual', 'prettier', '--files', PosixPath('/github/workspace/custom_components/netzooe_eservice/manifest.json')]' returned non-zero exit status 1.
Validating manifest...
```

It turns out their keys weren't sorted, but this error is obviously unhelpful and doesn't tell them that.

~~Wondering if there is an issue here with setting up pre-commit - from what I can tell, the Dockerfile installs requirements.txt but that doesn't contain pre-commit, and I think the pre-commit config would also have to be included so that prettier is installed as a pre-commit item.~~ EDIT: It looks like this is missing pre-commit. I THINK just by installing pre-commit, that this will work?

~~I may attempt to PR this but someone with more knowledge of this than me would probably be able to get it done faster 🙂~~

Hints from issue discussion:
same here

Commit message: Adjust hassfest.manifest based on config.action

Patch to review:
diff --git a/script/hassfest/manifest.py b/script/hassfest/manifest.py
index 9323b8e86c0083..0554b15728fd97 100644
--- a/script/hassfest/manifest.py
+++ b/script/hassfest/manifest.py
@@ -366,15 +366,19 @@ def _sort_manifest_keys(key: str) -> str:
     return _SORT_KEYS.get(key, key)
 
 
-def sort_manifest(integration: Integration) -> bool:
+def sort_manifest(integration: Integration, config: Config) -> bool:
     """Sort manifest."""
     keys = list(integration.manifest.keys())
     if (keys_sorted := sorted(keys, key=_sort_manifest_keys)) != keys:
         manifest = {key: integration.manifest[key] for key in keys_sorted}
         integration.manifest_path.write_text(json.dumps(manifest, indent=2))
+        if config.action == "generate":
+            text = "have been sorted"
+        else:
+            text = "are not sorted correctly"
         integration.add_error(
             "manifest",
-            "Manifest keys have been sorted: domain, name, then alphabetical order",
+            f"Manifest keys {text}: domain, name, then alphabetical order",
         )
         return True
     return False
@@ -387,9 +391,9 @@ def validate(integrations: dict[str, Integration], config: Config) -> None:
     for integration in integrations.values():
         validate_manifest(integration, core_components_dir)
         if not integration.errors:
-            if sort_manifest(integration):
+            if sort_manifest(integration, config):
                 manifests_resorted.append(integration.manifest_path)
-    if manifests_resorted:
+    if config.action == "generate" and manifests_resorted:
         subprocess.run(
             ["pre-commit", "run", "--hook-stage", "manual", "prettier", "--files"]
             + manifests_resorted,
diff --git a/script/hassfest/model.py b/script/hassfest/model.py
index e4f93c80e815f3..7df65b8221efd1 100644
--- a/script/hassfest/model.py
+++ b/script/hassfest/model.py
@@ -4,7 +4,7 @@
 from dataclasses import dataclass, field
 import json
 import pathlib
-from typing import Any
+from typing import Any, Literal
 
 
 @dataclass
@@ -26,7 +26,7 @@ class Config:
 
     specific_integrations: list[pathlib.Path] | None
     root: pathlib.Path
-    action: str
+    action: Literal["validate", "generate"]
     requirements: bool
     errors: list[Error] = field(default_factory=list)
     cache: dict[str, Any] = field(default_factory=dict)


Code review comments:
diff_hunk: @@ -366,15 +366,19 @@ def _sort_manifest_keys(key: str) -> str:
     return _SORT_KEYS.get(key, key)
 
 
-def sort_manifest(integration: Integration) -> bool:
+def sort_manifest(integration: Integration, config: Config) -> bool:
     """Sort manifest."""
     keys = list(integration.manifest.keys())
     if (keys_sorted := sorted(keys, key=_sort_manifest_keys)) != keys:
         manifest = {key: integration.manifest[key] for key in keys_sorted}
         integration.manifest_path.write_text(json.dumps(manifest, indent=2))
path: script/hassfest/manifest.py
text: @user1:
Should these lines be moved to under `if config.action == "generate":` as well?
There is not need to write this unless we are generating it?