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

Add manual_only option to addon boot config #5272

Merged
merged 4 commits into from
Aug 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion .github/PULL_REQUEST_TEMPLATE.md
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
- This PR is related to issue:
- Link to documentation pull request:
- Link to cli pull request:
- Link to client library pull request:
Copy link
Member

Choose a reason for hiding this comment

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

Yeah this makes sense, and ideally we also add a pull request template to the client library, which links back.

This is not really related to this PR, so it should be a separate.

Copy link
Contributor Author

@mdegat01 mdegat01 Aug 27, 2024

Choose a reason for hiding this comment

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

I mean if its important I can. Its pretty small though. And it is sort of related in that I want this PR to link to the Client library change. So I'd end up copying the new template into this PR description anyway

Copy link
Member

Choose a reason for hiding this comment

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

Since we squash commits on merge, this won't allow to revert the change later on. We don't revert that often, but that is the main reason why one change per PR is a thing.


## Checklist

Expand All @@ -55,9 +56,11 @@
- [ ] The code has been formatted using Ruff (`ruff format supervisor tests`)
- [ ] Tests have been added to verify that the new code works.

If API endpoints of add-on configuration are added/changed:
If API endpoints or add-on configuration are added/changed:

- [ ] Documentation added/updated for [developers.home-assistant.io][docs-repository]
- [ ] [CLI][cli-repository] updated (if necessary)
- [ ] [Client library][client-library-repository] updated (if necessary)

<!--
Thank you for contributing <3
Expand All @@ -67,3 +70,5 @@ If API endpoints of add-on configuration are added/changed:

[dev-checklist]: https://developers.home-assistant.io/docs/en/development_checklist.html
[docs-repository]: https://github.com/home-assistant/developers.home-assistant
[cli-repository]: https://github.com/home-assistant/cli
[client-library-repository]: https://github.com/home-assistant-libs/python-supervisor-client/
5 changes: 4 additions & 1 deletion supervisor/addons/addon.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
ATTR_WATCHDOG,
DNS_SUFFIX,
AddonBoot,
AddonBootConfig,
AddonStartup,
AddonState,
BusEvent,
Expand Down Expand Up @@ -311,7 +312,9 @@ def options(self, value: dict[str, Any] | None) -> None:

@property
def boot(self) -> AddonBoot:
"""Return boot config with prio local settings."""
"""Return boot config with prio local settings unless config is forced."""
if self.boot_config == AddonBootConfig.MANUAL_ONLY:
return super().boot
return self.persist.get(ATTR_BOOT, super().boot)

@boot.setter
Expand Down
10 changes: 8 additions & 2 deletions supervisor/addons/model.py
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@
SECURITY_DISABLE,
SECURITY_PROFILE,
AddonBoot,
AddonBootConfig,
AddonStage,
AddonStartup,
)
Expand Down Expand Up @@ -150,10 +151,15 @@ def options(self) -> dict[str, Any]:
return self.data[ATTR_OPTIONS]

@property
def boot(self) -> AddonBoot:
"""Return boot config with prio local settings."""
def boot_config(self) -> AddonBootConfig:
"""Return boot config."""
return self.data[ATTR_BOOT]

@property
def boot(self) -> AddonBoot:
"""Return boot config with prio local settings unless config is forced."""
return AddonBoot(self.data[ATTR_BOOT])

@property
def auto_update(self) -> bool | None:
"""Return if auto update is enable."""
Expand Down
5 changes: 4 additions & 1 deletion supervisor/addons/validate.py
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@
ROLE_ALL,
ROLE_DEFAULT,
AddonBoot,
AddonBootConfig,
AddonStage,
AddonStartup,
AddonState,
Expand Down Expand Up @@ -321,7 +322,9 @@ def _migrate(config: dict[str, Any]):
vol.Optional(ATTR_STARTUP, default=AddonStartup.APPLICATION): vol.Coerce(
AddonStartup
),
vol.Optional(ATTR_BOOT, default=AddonBoot.AUTO): vol.Coerce(AddonBoot),
vol.Optional(ATTR_BOOT, default=AddonBootConfig.AUTO): vol.Coerce(
AddonBootConfig
),
vol.Optional(ATTR_INIT, default=True): vol.Boolean(),
vol.Optional(ATTR_ADVANCED, default=False): vol.Boolean(),
vol.Optional(ATTR_STAGE, default=AddonStage.STABLE): vol.Coerce(AddonStage),
Expand Down
8 changes: 7 additions & 1 deletion supervisor/api/addons.py
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@
ATTR_WEBUI,
REQUEST_FROM,
AddonBoot,
AddonBootConfig,
)
from ..coresys import CoreSysAttributes
from ..docker.stats import DockerStats
Expand All @@ -109,7 +110,7 @@
PwnedSecret,
)
from ..validate import docker_ports
from .const import ATTR_REMOVE_CONFIG, ATTR_SIGNED
from .const import ATTR_BOOT_CONFIG, ATTR_REMOVE_CONFIG, ATTR_SIGNED
from .utils import api_process, api_validate, json_loads

_LOGGER: logging.Logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -217,6 +218,7 @@ async def info(self, request: web.Request) -> dict[str, Any]:
ATTR_VERSION_LATEST: addon.latest_version,
ATTR_PROTECTED: addon.protected,
ATTR_RATING: rating_security(addon),
ATTR_BOOT_CONFIG: addon.boot_config,
ATTR_BOOT: addon.boot,
ATTR_OPTIONS: addon.options,
ATTR_SCHEMA: addon.schema_ui,
Expand Down Expand Up @@ -300,6 +302,10 @@ async def options(self, request: web.Request) -> None:
if ATTR_OPTIONS in body:
addon.options = body[ATTR_OPTIONS]
if ATTR_BOOT in body:
if addon.boot_config == AddonBootConfig.MANUAL_ONLY:
raise APIError(
f"Addon {addon.slug} boot option is set to {addon.boot_config} so it cannot be changed"
)
addon.boot = body[ATTR_BOOT]
if ATTR_AUTO_UPDATE in body:
addon.auto_update = body[ATTR_AUTO_UPDATE]
Expand Down
1 change: 1 addition & 0 deletions supervisor/api/const.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
ATTR_ATTRIBUTES = "attributes"
ATTR_AVAILABLE_UPDATES = "available_updates"
ATTR_BACKGROUND = "background"
ATTR_BOOT_CONFIG = "boot_config"
ATTR_BOOT_SLOT = "boot_slot"
ATTR_BOOT_SLOTS = "boot_slots"
ATTR_BOOT_TIMESTAMP = "boot_timestamp"
Expand Down
15 changes: 15 additions & 0 deletions supervisor/const.py
Original file line number Diff line number Diff line change
Expand Up @@ -382,12 +382,27 @@
ROLE_ALL = [ROLE_DEFAULT, ROLE_HOMEASSISTANT, ROLE_BACKUP, ROLE_MANAGER, ROLE_ADMIN]


class AddonBootConfig(StrEnum):
"""Boot mode config for the add-on."""

AUTO = "auto"
MANUAL = "manual"
MANUAL_ONLY = "manual_only"


class AddonBoot(StrEnum):
"""Boot mode for the add-on."""

AUTO = "auto"
MANUAL = "manual"

@classmethod
def _missing_(cls, value: str) -> Self | None:
"""Convert 'forced' config values to their counterpart."""
if value == AddonBootConfig.MANUAL_ONLY:
return AddonBoot.MANUAL
mdegat01 marked this conversation as resolved.
Show resolved Hide resolved
return None


class AddonStartup(StrEnum):
"""Startup types of Add-on."""
Expand Down
12 changes: 12 additions & 0 deletions tests/addons/test_addon.py
Original file line number Diff line number Diff line change
Expand Up @@ -691,6 +691,7 @@ async def test_local_example_install(
mock_aarch64_arch_supported: None,
):
"""Test install of an addon."""
coresys.hardware.disk.get_disk_free_space = lambda x: 5000
assert not (
data_dir := tmp_supervisor_data / "addons" / "data" / "local_example"
).exists()
Expand Down Expand Up @@ -883,3 +884,14 @@ async def test_addon_load_succeeds_with_docker_errors(
caplog.clear()
await install_addon_ssh.load()
assert "Unknown error with test/amd64-addon-ssh:9.2.1" in caplog.text


async def test_addon_manual_only_boot(coresys: CoreSys, install_addon_example: Addon):
"""Test an addon with manual only boot mode."""
assert install_addon_example.boot_config == "manual_only"
assert install_addon_example.boot == "manual"

# Users cannot change boot mode of an addon with manual forced so changing boot isn't realistic
# However boot mode can change on update and user may have set auto before, ensure it is ignored
install_addon_example.boot = "auto"
assert install_addon_example.boot == "manual"
20 changes: 20 additions & 0 deletions tests/api/test_addons.py
Original file line number Diff line number Diff line change
Expand Up @@ -346,3 +346,23 @@ async def test_api_addon_system_managed(
body = await resp.json()
assert body["data"]["system_managed"] is False
assert body["data"]["system_managed_config_entry"] is None


async def test_addon_options_boot_mode_manual_only_invalid(
api_client: TestClient, install_addon_example: Addon
):
"""Test changing boot mode is invalid if set to manual only."""
install_addon_example.data["ingress"] = False
resp = await api_client.get("/addons/local_example/info")
assert resp.status == 200
body = await resp.json()
assert body["data"]["boot"] == "manual"
assert body["data"]["boot_config"] == "manual_only"

resp = await api_client.post("/addons/local_example/options", json={"boot": "auto"})
assert resp.status == 400
body = await resp.json()
assert (
body["message"]
== "Addon local_example boot option is set to manual_only so it cannot be changed"
)
1 change: 1 addition & 0 deletions tests/fixtures/addons/local/example/config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,4 @@ ingress_port: 0
breaking_versions:
- test
- 1.0
boot: manual_only
Loading