Skip to content

feat(image): support per-image architecture overrides for aarch64 builds - #346

Open
binujp wants to merge 1 commit into
microsoft:mainfrom
binujp:bphilip/image-arch-support
Open

feat(image): support per-image architecture overrides for aarch64 builds#346
binujp wants to merge 1 commit into
microsoft:mainfrom
binujp:bphilip/image-arch-support

Conversation

@binujp

@binujp binujp commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Summary

Add an optional architectures field to image objects to specify which
architectures that image can build on.

All images need not build on all architectures. E.g. gen1 images do not build
for aarch64. We have to specify this metadata about which images build on which
architectures in an image-definition- and build-mechanism-independent manner.
images.toml lets us encode this metadata per image, and azldev generates the
JSON schema for consumption.

The architectures field is optional: an image with no declared Architectures
is treated as unrestricted (all recognized architectures) rather than rejected
during validation. This lets older images.toml files work with the newer
azldev version and gives us a window to switch over.

Changes

  • internal/projectconfig/image.go, project.go, loader.go: add/validate optional per-image architecture overrides
  • internal/app/azldev/cmds/image/build.go, list.go: apply overrides during build/list
  • docs/user/reference/config/images.md, internal/app/azldev/agentskill/content/image.md.tmpl: document the new config
  • schemas/azldev.schema.json, scenario snapshots: regenerated via mage docs / mage scenarioUpdate

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

Copilot AI lite review requested due to automatic review settings September 9, 2026 00:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

There are correctness and consistency issues (a failing string assertion in the new build-architecture test, plus a schema description mismatch with current runtime behavior) that should be resolved before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR introduces an explicit per-image architectures configuration for images, validates it during project config validation, and enforces it in azldev image build while surfacing it in azldev image list, with accompanying docs/schema/snapshot updates.

Changes:

  • Add architectures to ImageConfig and validate that each image declares a non-empty, supported architecture list.
  • Enforce image architecture support during azldev image build (defaulting --arch to host arch) and include architectures in azldev image list output.
  • Regenerate docs/schema/snapshots and update tests/config fixtures for the new required field.
File summaries
File Description
schemas/azldev.schema.json Schema updated to require images.*.architectures and reflects regenerated descriptions.
scenario/snapshots/TestSnapshotsContainer_config_generate-schema_stdout_1.snap Snapshot updated for new schema output.
scenario/snapshots/TestSnapshots_config_generate-schema_stdout_1.snap Snapshot updated for new schema output.
internal/projectconfig/testsuite_test.go Update test fixtures to include required image architectures.
internal/projectconfig/project.go Add validateImageArchitectures / validateArchitectureList and wire into ProjectConfig.Validate().
internal/projectconfig/loader.go Use NewProjectConfig() for default initialization before merging config files.
internal/projectconfig/loader_test.go Update config fixtures for required architectures; add an image-architectures loader test.
internal/projectconfig/image.go Add architectures field + helpers/constants and SupportsArchitecture.
internal/projectconfig/configfile_test.go Add validation coverage for image architectures; update other validation tests for required field.
internal/app/azldev/cmds/image/list.go Include architectures in list output and table summary.
internal/app/azldev/cmds/image/list_test.go Assert architectures + summary in list results; add per-image architectures test.
internal/app/azldev/cmds/image/build.go Validate target arch against image-supported architectures before building.
internal/app/azldev/cmds/image/build_internal_test.go Add unit test coverage for build-architecture validation.
internal/app/azldev/agentskill/content/image.md.tmpl Document required architectures in the image skill content.
docs/user/reference/config/images.md Document the new required architectures field and supported values.
Review details
  • Files reviewed: 15/15 changed files
  • Comments generated: 5
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread internal/app/azldev/cmds/image/build_internal_test.go
Comment thread schemas/azldev.schema.json
Comment thread docs/user/reference/config/images.md Outdated
Comment thread internal/app/azldev/cmds/image/build.go
Comment thread internal/app/azldev/cmds/image/list.go Outdated
Copilot AI review requested due to automatic review settings September 9, 2026 03:29
@binujp
binujp force-pushed the bphilip/image-arch-support branch from 3967acb to 40d0545 Compare September 9, 2026 03:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The new build-architecture validation tests contain failing assertions (and host-arch defaulting logic is currently inconsistent with its intended unsupported-host handling).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

internal/projectconfig/project.go:389

  • validateArchitectureList formats the supported architecture list without quoting the literal tokens, which is inconsistent with the rest of the repo’s error-string quoting for string values. Using %q for the slice keeps values quoted and avoids ambiguous output.

internal/app/azldev/cmds/image/build_internal_test.go:138

  • The test assertions expect backtick-quoted strings, but validateBuildArchitecture uses %#q for image/arch, so the error contains double-quoted Go string literals (e.g. image "gen1" ... "aarch64"). As written, these ErrorContains checks will fail.
	err := validateBuildArchitecture(imageConfig, ImageArchAarch64, "amd64")
	require.ErrorContains(t, err, "image `gen1` does not support architecture `aarch64`")

	err = validateBuildArchitecture(imageConfig, ImageArchDefault, "arm64")
	require.ErrorContains(t, err, "image `gen1` does not support architecture `aarch64`")
  • Files reviewed: 15/15 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread internal/app/azldev/cmds/image/build.go
Copilot AI review requested due to automatic review settings September 9, 2026 03:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The new build-architecture error path uses an invalid format verb for a string slice (%q), which will produce broken error output and makes the added unit test assertions incorrect.

Review details

Suppressed comments (2)

internal/app/azldev/cmds/image/build.go:268

  • Using %q to format a []string will produce a Go formatting error (e.g., %!q([]string=...)) in the returned message. Use %v (or join the slice) so the error is readable and tests don’t depend on undefined formatting output.
			"image %#q does not support architecture %#q; supported architectures: %q",

internal/app/azldev/cmds/image/build_internal_test.go:138

  • The test asserts backtick-quoted values, but validateBuildArchitecture formats with %#q, which yields double-quoted strings (e.g., "gen1"). Update the assertions to match the actual quoting so the test will pass consistently.
	require.ErrorContains(t, err, "image `gen1` does not support architecture `aarch64`")

	err = validateBuildArchitecture(imageConfig, ImageArchDefault, "arm64")
	require.ErrorContains(t, err, "image `gen1` does not support architecture `aarch64`")
  • Files reviewed: 15/15 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Copilot AI review requested due to automatic review settings September 9, 2026 03:44
@binujp
binujp force-pushed the bphilip/image-arch-support branch from 40d0545 to a1f1b86 Compare September 9, 2026 03:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

There’s a confirmed runtime formatting bug in the new build-arch validation error and the new unit test assertions currently won’t match the function’s actual quoting behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

internal/app/azldev/cmds/image/build_internal_test.go:141

  • These assertions expect backtick-quoted values, but validateBuildArchitecture formats values with %#q (double-quoted strings). As written, the test will fail even when the function is behaving correctly.
	err := validateBuildArchitecture(imageConfig, ImageArchAarch64, "amd64")
	require.ErrorContains(t, err, "image `gen1` does not support architecture `aarch64`")

	err = validateBuildArchitecture(imageConfig, ImageArchDefault, "arm64")
	require.ErrorContains(t, err, "image `gen1` does not support architecture `aarch64`")

	err = validateBuildArchitecture(imageConfig, ImageArchDefault, "riscv64")
	require.ErrorContains(t, err, "unsupported host architecture `riscv64`")
  • Files reviewed: 15/15 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread internal/app/azldev/cmds/image/build.go
@binujp
binujp marked this pull request as ready for review September 10, 2026 04:00
Copilot AI review requested due to automatic review settings September 10, 2026 22:54
@binujp
binujp force-pushed the bphilip/image-arch-support branch from a1f1b86 to cc72037 Compare September 10, 2026 22:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Four moderate findings remain in build validation and generated/schema artifacts.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

internal/app/azldev/cmds/image/build.go:263

  • When TargetArch is populated directly rather than through Cobra's ImageArch.Set, this branch accepts any string. An unrestricted image therefore allows ImageArch("riscv64"), and createKiwiRunner forwards it to kiwi even though only the two QEMU architectures are supported; validate explicit targets against qemu.SupportedArchitectures() as well.
	arch := string(targetArch)
	if arch == "" {
		arch = qemu.GoArchToQEMUArch(hostGoArch)
		if !slices.Contains(qemu.SupportedArchitectures(), arch) {
			return fmt.Errorf("unsupported host architecture %#q", hostGoArch)

internal/projectconfig/image.go:56

  • Architectures is rejected at runtime unless every value is x86_64 or aarch64, but this schema tag omits the corresponding enum values. Schema-based editors and consumers therefore accept values that ProjectConfig.Validate rejects; add enum=x86_64,enum=aarch64 here and regenerate the schema artifacts, matching the closed-set config fields in tests.go:54-59 and specsource.go:7-10.
	Architectures []string `toml:"architectures,omitempty" json:"architectures,omitempty" jsonschema:"title=Architectures,description=Architectures supported by this image (optional; unset means unrestricted)"`
  • Files reviewed: 15/15 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread scenario/__snapshots__/TestSnapshots_config_generate-schema_stdout_1.snap Outdated
Copilot AI review requested due to automatic review settings September 11, 2026 03:51
@binujp
binujp force-pushed the bphilip/image-arch-support branch from cc72037 to dcf5836 Compare September 11, 2026 03:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

A critical script-path regression and a moderate architecture-validation issue remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

internal/app/azldev/agentskill/content/image.md.tmpl:56

  • This edits the emitted azldev-image skill, but the existing image-skill content test only asserts the kiwi config-override phrase. Add a distinctive assertion for the new architectures guidance so future content changes cannot silently drop this user-facing behavior.
- `architectures = ["x86_64", "aarch64"]` is optional; when unset or empty, the
   image is treated as unrestricted (all recognized architectures). Set it to
   restrict which architectures `image build --arch` allows for the image.

internal/projectconfig/image.go:67

  • For an unrestricted image this returns true for any string, including an architecture azldev does not recognize, even though the surrounding documentation defines unrestricted as all recognized architectures. validateBuildArchitecture currently has to duplicate a separate supported-architecture check to compensate, so other callers of this exported helper can get an incorrect result; validate arch here before applying the per-image list.
	if len(i.Architectures) == 0 {
		return true
	}

	return slices.Contains(i.Architectures, arch)
  • Files reviewed: 15/15 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment on lines +38 to +39
defaultConfig := NewProjectConfig()
resolvedCfg := &defaultConfig
Add an optional "architectures" field to image objects to specify which
archictectures that image can build on.

All images need not build on all architectures. Eg.gen1 images do not build
for aarch64. We have to specify this metadata about which images build on which
architectures in am image definition and build mechanism independent manner.
"images.toml" lets us encode this metadata per image and azldev generates the
json for schema consumption.

The architectures field is optional: an image with no declared Architectures is
treated as unrestricted (all recognized architectures) rather than rejected
during validation. This lets older images.toml files work with newer version and
gives us a window to switch over.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@binujp
binujp force-pushed the bphilip/image-arch-support branch from dcf5836 to 5630569 Compare September 11, 2026 14:13
Copilot AI review requested due to automatic review settings September 11, 2026 14:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

No unresolved issues were identified, and the reviewed changes are approval-ready.

Review details
  • Files reviewed: 16/16 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

| Publish | `publish` | [ImagePublish](#image-publish) | No | Publishing settings for this image |
| Architectures | `architectures` | string array | No | Architectures supported by this image |

The current supported architectures are `x86_64` and `aarch64`. `architectures` is

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

question(non-blocking): Is this the appropriate spot for this description of Architectures? I definitely like having it but I don't see anything else doing being described here. I wonder if there's a better place for it, but this is definitely non-blocking.

imageConfig.Architectures,
", ",
),
Tests: imageConfig.Tests,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit(non-blocking): I notice that you changed the indentation/justification for these fields. Was that intentional? If so, why?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants