Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Image arch #234

Merged
merged 3 commits into from
Aug 8, 2024
Merged

Image arch #234

merged 3 commits into from
Aug 8, 2024

Conversation

lbajolet-hashicorp
Copy link
Contributor

This PR fixes issue #232 where the default value for image_architecture (introduced in v1.1.5 of the plugin) defaulted to x86_64, thereby breaking some existing arm64 build configs.
With this PR, we default again to leaving it empty, so the source image architecture gets forwarded to the final image being produced.
We also add some acceptance tests for this, so in the future we have an architecture to test those templates, and make sure we don't break it accidentally again.

Closes: #232

Copy link
Contributor

@JenGoldstrich JenGoldstrich left a comment

Choose a reason for hiding this comment

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

LGTM largely, left some minor feedback, I think it'd be good to get the acceptance tests passing before releasing here though

builder/googlecompute/builder_acc_test.go Show resolved Hide resolved
builder/googlecompute/config.go Outdated Show resolved Hide resolved
When introduced, the `image_architecture` attribute defaulted to X86_64.
This defaut was based on the assumption that the ARM64 images didn't
default to that architecture, and instead if underfined that x86_64 was
the default.

This is not true, as if this is not defined when creating the image, the
source image's architecture is inherited, so if the source image had its
architecture set to ARM64, this would carry over.
Therefore, the default value broke builds like those, as they would now
point to x86_64 instead of carrying over the previous value.

This commit then changes this default value, leaving it blank if
unspecified, thereby restoring the old behaviour.
Since we plan on using this in tests, and the API already returns it, we
include the architecture in the Image type from the common package, and
set it within the drivers.
The image_architecture attribute when introduced caused problems with
some existing configs, so to avoid introducing further bugs in the
future, we add new accetpance tests that ensure the default value is
maintained, and that the architecture is forwarded to the final image
being created.
@lbajolet-hashicorp lbajolet-hashicorp merged commit 79ce661 into main Aug 8, 2024
12 checks passed
@lbajolet-hashicorp lbajolet-hashicorp deleted the image_arch branch August 8, 2024 14:02
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Breaking change with image_architecture in 1.1.5
2 participants