-
Notifications
You must be signed in to change notification settings - Fork 775
Make app builds work without build.yaml #6694
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -19,7 +19,13 @@ | |||||||||||||||||||||||||||||
| ATTR_SQUASH, | ||||||||||||||||||||||||||||||
| ATTR_USERNAME, | ||||||||||||||||||||||||||||||
| FILE_SUFFIX_CONFIGURATION, | ||||||||||||||||||||||||||||||
| META_ADDON, | ||||||||||||||||||||||||||||||
| LABEL_ARCH, | ||||||||||||||||||||||||||||||
| LABEL_DESCRIPTION, | ||||||||||||||||||||||||||||||
| LABEL_NAME, | ||||||||||||||||||||||||||||||
| LABEL_TYPE, | ||||||||||||||||||||||||||||||
| LABEL_URL, | ||||||||||||||||||||||||||||||
| LABEL_VERSION, | ||||||||||||||||||||||||||||||
| META_APP, | ||||||||||||||||||||||||||||||
| SOCKET_DOCKER, | ||||||||||||||||||||||||||||||
| CpuArch, | ||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||
|
|
@@ -48,20 +54,29 @@ def __init__(self, coresys: CoreSys, addon: AnyAddon) -> None: | |||||||||||||||||||||||||||||
| """Initialize Supervisor add-on builder.""" | ||||||||||||||||||||||||||||||
| self.coresys: CoreSys = coresys | ||||||||||||||||||||||||||||||
| self.addon = addon | ||||||||||||||||||||||||||||||
| self._has_build_file: bool = False | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| # Search for build file later in executor | ||||||||||||||||||||||||||||||
| super().__init__(None, SCHEMA_BUILD_CONFIG) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| @property | ||||||||||||||||||||||||||||||
| def has_build_file(self) -> bool: | ||||||||||||||||||||||||||||||
| """Return True if a build configuration file was found on disk.""" | ||||||||||||||||||||||||||||||
| return self._has_build_file | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| def _get_build_file(self) -> Path: | ||||||||||||||||||||||||||||||
| """Get build file. | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| Must be run in executor. | ||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||
| return find_one_filetype( | ||||||||||||||||||||||||||||||
| result = find_one_filetype( | ||||||||||||||||||||||||||||||
| self.addon.path_location, "build", FILE_SUFFIX_CONFIGURATION | ||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||
| self._has_build_file = True | ||||||||||||||||||||||||||||||
| return result | ||||||||||||||||||||||||||||||
| except ConfigurationFileError: | ||||||||||||||||||||||||||||||
| self._has_build_file = False | ||||||||||||||||||||||||||||||
| return self.addon.path_location / "build.json" | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| async def read_data(self) -> None: | ||||||||||||||||||||||||||||||
|
|
@@ -81,10 +96,12 @@ def arch(self) -> CpuArch: | |||||||||||||||||||||||||||||
| return self.sys_arch.match([self.addon.arch]) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| @property | ||||||||||||||||||||||||||||||
| def base_image(self) -> str: | ||||||||||||||||||||||||||||||
| """Return base image for this add-on.""" | ||||||||||||||||||||||||||||||
| def base_image(self) -> str | None: | ||||||||||||||||||||||||||||||
| """Return base image for this add-on, or None to use Dockerfile default.""" | ||||||||||||||||||||||||||||||
| 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" | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is intentional, the images are now guaranteed to be multi-arch.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Copilot
AI
Apr 1, 2026
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Copilot
AI
Apr 1, 2026
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Copilot
AI
Apr 1, 2026
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also quite unlikely, supplying BUILD_FROM in args would be an anti-pattern.
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.
Ugh yeah all bit meh here.
Maybe we should consider moving away from
FileConfigurationforAddonBuild, since that is actually what this PR is trying to do: Not rely onbuild.json/build.yamlanymore 🤔 .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.
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
FileConfigurationnow would require us to handle it in different manner anyway.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.
Well, a
build.json/build.yamlwas 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
FileConfigurationis 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 onFileConfigurationalways, 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
FileConfigurationforAddonBuildnow.