Conversation
The builds are moving build configuration into the Dockerfile itself (base image defaults via `ARG`, labels via `LABEL`). This change makes `build.yaml` optional for local app builds while preserving backward compatibility for apps that still define it. Key changes: * Track whether `build.yaml` was found, log a warning if it is. * Skip `BUILD_FROM` build arg when no build file exists, letting the Dockerfile's own `ARG BUILD_FROM=...` default take effect. * Always include all configured registry credentials in docker config instead of matching only the base image's registry. * Only set `io.hass.name` and `io.hass.description` labels when non-empty, as they could be defined in the Dockerfile directly. * Log a deprecation warning when `build.yaml` is present. Refs home-assistant/epics#33
There was a problem hiding this comment.
Pull request overview
This PR updates the Supervisor add-on build flow to make build.yaml optional by relying on Dockerfile-defined defaults (e.g., ARG/LABEL) while retaining compatibility with existing add-ons that still ship a build config.
Changes:
- Add tracking of whether a build config file exists and emit a deprecation warning during builds.
- Adjust build-arg/label generation to skip
BUILD_FROMwhen no build config exists and to omitio.hass.name/io.hass.descriptionwhen empty. - Change Docker config generation to include credentials for all configured registries, and expand test coverage for these scenarios.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
supervisor/addons/build.py |
Implements has_build_file, conditional BUILD_FROM/label behavior, and updated docker config auth generation. |
supervisor/docker/addon.py |
Adds a deprecation warning when a build config file is detected during add-on builds. |
supervisor/const.py |
Introduces constants for commonly used label keys and adds META_APP. |
tests/addons/test_build.py |
Updates/adds tests for registry auth generation, build-file detection, conditional BUILD_FROM, and label inclusion/omission. |
supervisor/docker/addon.py
Outdated
| "Add-on %s uses build.yaml which is deprecated. " | ||
| "Move build parameters into the Dockerfile directly.", |
There was a problem hiding this comment.
The deprecation warning is gated on has_build_file, which becomes true for any build.* file found (e.g., build.json as well as build.yaml). The log message specifically says build.yaml, which can be inaccurate and confusing. Consider either tracking the actual filename that was found, or rewording the warning to reference a deprecated build configuration file (e.g., build.yaml/build.json).
| "Add-on %s uses build.yaml which is deprecated. " | |
| "Move build parameters into the Dockerfile directly.", | |
| "Add-on %s uses a separate build configuration file (e.g. build.yaml/build.json) " | |
| "which is deprecated. Move build parameters into the Dockerfile directly.", |
There was a problem hiding this comment.
And it can be .yml as well. We refer to the file as to build.yaml in docs too, I think it should be obvious and I'd prefer to keep it simple.
| if not self._data[ATTR_BUILD_FROM]: | ||
| return f"ghcr.io/home-assistant/{self.arch!s}-base:latest" | ||
| if self._has_build_file: | ||
| return "ghcr.io/home-assistant/base:latest" |
There was a problem hiding this comment.
base_image changes the fallback when build_from is empty and a build config file exists: it now returns ghcr.io/home-assistant/base:latest instead of the previous arch-specific .../{arch}-base:latest. Unless ghcr.io/home-assistant/base is guaranteed to exist as a multi-arch manifest, this will break builds for add-ons with a build file but no build_from. Consider keeping the arch-specific fallback for backward compatibility (or documenting/centralizing the new canonical default).
| return "ghcr.io/home-assistant/base:latest" | |
| return f"ghcr.io/home-assistant/{MAP_ARCH[self.arch]}-base:latest" |
There was a problem hiding this comment.
This is intentional, the images are now guaranteed to be multi-arch.
| auths = { | ||
| self._registry_key(registry): {"auth": self._registry_auth(registry)} | ||
| for registry in self.sys_docker.config.registries | ||
| } | ||
| return json.dumps({"auths": auths}) |
There was a problem hiding this comment.
get_docker_config_json() now includes credentials for all configured registries and mounts them into the build environment. Because Dockerfiles can execute arbitrary commands during build, this increases the blast radius (a malicious/compromised Dockerfile could exfiltrate unrelated registry creds). If possible, limit injected credentials to registries actually needed for the build (e.g., those referenced in FROM lines / BUILD_FROM, or a minimal allowlist), or make the broader behavior opt-in with clear warnings.
There was a problem hiding this comment.
IMHO there are potentially more risks if someone installs builds shady Dockerfiles and I don't see a simple solution for this (unless we want to parse the Dockerfile to extract the image). In the end the authentication to a single registry may not be needed at all and we still supply the credentials.
| # Set name only if non-empty, could have been set in Dockerfile | ||
| if name := self._fix_label("name"): | ||
| labels[LABEL_NAME] = name | ||
|
|
||
| # Set description only if non-empty, could have been set in Dockerfile | ||
| if description := self._fix_label("description"): |
There was a problem hiding this comment.
The new label-building order changes precedence: additional_labels are merged first, but then io.hass.name / io.hass.description are added afterwards, overriding any values provided via build.yaml labels. Previously, build-file labels could override the defaults. To preserve backward compatibility, consider only setting name/description if those labels are not already present, or merge additional_labels last.
| # Set name only if non-empty, could have been set in Dockerfile | |
| if name := self._fix_label("name"): | |
| labels[LABEL_NAME] = name | |
| # Set description only if non-empty, could have been set in Dockerfile | |
| if description := self._fix_label("description"): | |
| # Set name only if non-empty and not already provided via additional_labels, | |
| # could have been set in Dockerfile | |
| if LABEL_NAME not in labels and (name := self._fix_label("name")): | |
| labels[LABEL_NAME] = name | |
| # Set description only if non-empty and not already provided via additional_labels, | |
| # could have been set in Dockerfile | |
| if LABEL_DESCRIPTION not in labels and (description := self._fix_label("description")): |
There was a problem hiding this comment.
This seems like an unlikely edge case, and in the end it's IMO better for the dedicated fields from the config to take precedence.
| **self.additional_args, | ||
| } | ||
|
|
||
| if self.base_image is not None: |
There was a problem hiding this comment.
The build-arg precedence has changed: additional_args are merged first, but then BUILD_FROM is added afterwards, overriding any BUILD_FROM value that might be intentionally provided via args in the build config. Previously, args could override the default BUILD_FROM. If that override behavior is relied upon, consider only setting BUILD_FROM when it isn't already present in additional_args, or restoring the previous merge order.
| if self.base_image is not None: | |
| if self.base_image is not None and "BUILD_FROM" not in build_args: |
There was a problem hiding this comment.
Also quite unlikely, supplying BUILD_FROM in args would be an anti-pattern.
agners
left a comment
There was a problem hiding this comment.
Overall, looks quite straight forward. Nice! 👍
| self._has_build_file = True | ||
| return result | ||
| except ConfigurationFileError: | ||
| self._has_build_file = False |
There was a problem hiding this comment.
Ugh yeah all bit meh here.
Maybe we should consider moving away from FileConfiguration for AddonBuild, since that is actually what this PR is trying to do: Not rely on build.json/build.yaml anymore 🤔 .
There was a problem hiding this comment.
Not sure I follow but the meh try-except allowing the file not to be present was here before as well. Majority of apps will keep using the structured config for quite a while, so not using FileConfiguration now would require us to handle it in different manner anyway.
There was a problem hiding this comment.
Well, a build.json/build.yaml was so far essentially mandatory. The code handled a graceful fallback which lead to using a empty dict, which ultimately applied the default schema of that config file.
In my books the old code was already a bit funky, since FileConfiguration is mostly used for Supervisor internal configuration files. But whatever, reuse I guess 🤷
However, now we want to get rid of build.json/build.yaml. Still relying on FileConfiguration always, let the system always come up with a empty build config, and then not use it seems just the wrong way around.
So I'd say, lets bite the bullet, and move away from FileConfiguration for AddonBuild now.
| """Generate Docker config.json content with all configured registry credentials. | ||
|
|
||
| Returns a JSON string with registry credentials, or None if no registries | ||
| are configured. |
There was a problem hiding this comment.
Exposes more credentials then strictly necessary to the builder, but works, so I am fine with it 🤷
I wonder if there is really no other way to pass credentials to builder. But since that is rather recent, i guess this was really the only way to implement it.
There was a problem hiding this comment.
I wonder if this is really a problem. The builder is actually the official docker image from the Docker Hub and the config file with credentials is supplied to that. When you run an image build on a local machine, the BuildKit builder has access to all the credentials in Docker config too, but that doesn't mean it can exfiltrate them - or does it? It should only allow to do operations on the registry that require authentication.
The credentials are copied to a temporary file on the host but there you have limited accessibility as well, and you can access other files containing them, so I don't see this an issue either.
There was a problem hiding this comment.
I guess the only way you could (miss)use the other credentials if you use multi-staged builds with other FROMs... But you could argue that is a feature, that we support multi-stage builds from different private repositories 😅 🤷 .
As I said, I am fine with it. Just thoughts.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Proposed change
The builds are moving build configuration into the Dockerfile itself (base image defaults via
ARG, labels viaLABEL). This change makesbuild.yamloptional for local app builds while preserving backward compatibility for apps that still define it.Key changes:
build.yamlwas found, log a warning if it is.BUILD_FROMbuild arg when no build file exists, letting the Dockerfile's ownARG BUILD_FROM=...default take effect.io.hass.nameandio.hass.descriptionlabels when non-empty, as they could be defined in the Dockerfile directly.build.yamlis present.Refs home-assistant/epics#33
Type of change
Additional information
Checklist
ruff format supervisor tests)If API endpoints or add-on configuration are added/changed: