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

Support building Shared Image Gallery Image without intermediate Managed Image #242

Merged
merged 4 commits into from
Jan 18, 2023

Conversation

VladRassokhin
Copy link
Contributor

As result it fixes #210
ARM64 Managed Images are not supported, only ones in Shared Image Gallery.

I've tested it on Windows 11 ARM64 in West Europe, works!
Input from contributors/mainteners is welcome.

Closes #210

@VladRassokhin VladRassokhin requested a review from a team as a code owner October 12, 2022 21:48
@VladRassokhin
Copy link
Contributor Author

I've almost not touched dtl, contributions are welcome.

Copy link
Contributor

@agrawalkshitij agrawalkshitij left a comment

Choose a reason for hiding this comment

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

This is awesome! I think overall this looks great and is going in the right direction. managed images are not being maintained in azure and all new features are being developed on SIG images

builder/azure/arm/artifact.go Show resolved Hide resolved
builder/azure/arm/step_capture_image.go Outdated Show resolved Hide resolved
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.

This is looking good so far, there are a few error messages I make suggestions on, but I did try and run tests and see a panic when I run make test on an ARM Mac running Go 1.18.5

make test
?   	github.com/hashicorp/packer-plugin-azure	[no test files]
--- FAIL: TestArtifactIDManagedImageWithSharedImageGalleryId (0.00s)
panic: interface conversion: interface {} is nil, not string [recovered]
	panic: interface conversion: interface {} is nil, not string

goroutine 10 [running]:
testing.tRunner.func1.2({0x103b633a0, 0xc00051bc80})
	/opt/homebrew/Cellar/[email protected]/1.18.5/libexec/src/testing/testing.go:1389 +0x278
testing.tRunner.func1()
	/opt/homebrew/Cellar/[email protected]/1.18.5/libexec/src/testing/testing.go:1392 +0x420
panic({0x103b633a0, 0xc00051bc80})
	/opt/homebrew/Cellar/[email protected]/1.18.5/libexec/src/runtime/panic.go:844 +0x258
github.com/hashicorp/packer-plugin-azure/builder/azure/arm.(*Artifact).String(0xc0001fe7e0)
	/Users/jgoldstrich/go/src/github.com/hashicorp/packer-plugin-azure/builder/azure/arm/artifact.go:263 +0xaa4
github.com/hashicorp/packer-plugin-azure/builder/azure/arm.TestArtifactIDManagedImageWithSharedImageGalleryId(0xc000103040)
	/Users/jgoldstrich/go/src/github.com/hashicorp/packer-plugin-azure/builder/azure/arm/artifact_test.go:229 +0x330
testing.tRunner(0xc000103040, 0x103ca0be0)
	/opt/homebrew/Cellar/[email protected]/1.18.5/libexec/src/testing/testing.go:1439 +0x190
created by testing.(*T).Run
	/opt/homebrew/Cellar/[email protected]/1.18.5/libexec/src/testing/testing.go:1486 +0x564
FAIL	github.com/hashicorp/packer-plugin-azure/builder/azure/arm	0.597s

Can you take a look?

builder/azure/arm/config.go Outdated Show resolved Hide resolved
builder/azure/arm/config.go Outdated Show resolved Hide resolved
builder/azure/arm/artifact.go Outdated Show resolved Hide resolved
@JenGoldstrich
Copy link
Contributor

Commenting this as a note for myself that we'll need to update docs/builders/arm.mdx with this PR, which specifically refers to "The Azure builder can create either a VHD or a managed image."

Also this deprecation warning should be updated as well

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.

Forgot to mark this as requesting changes to fix the panic

@VladRassokhin
Copy link
Contributor Author

I'd suggest to perform rebase before merging.

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.

Hey @VladRassokhin, just dropping in to leave one small suggestion about the additional disks TODO and update you before a holiday break. I played around with this PR manually today and built some ARM64 images with packer, very nice work and appreciate the contribution, I wanna give this another thorough review early in the new year and maybe make some small error messages changes directly to your forked branch, I'll reach out if there's anything larger we should change for this PR.

Thanks again and looking forward to getting this merged in the new year!

@JenGoldstrich
Copy link
Contributor

Happy new year @VladRassokhin! Just dropping a thought I had on the DTL builder.

I think we should actually remove these changes to DTL just to simplify getting this out, I am not sure if a lot of users are currently using the DTL builder and if folks end up needing this functionality they'll open an issue and we can port it then, that way we can simplify getting this PR merged

…ged Image

Fixes hashicorp#210 : ARM64 Managed Images are not supported, only ones in Shared Image Gallery
… will be added anyway

See builder/azure/arm/builder.go:202
@VladRassokhin
Copy link
Contributor Author

Performed some cleanup and rebase, not going to do anything else on that PR.

@JenGoldstrich
Copy link
Contributor

@VladRassokhin understood thank you so much for this contribution, this change breaks some HCP Packer integration so I'm working on fixing that now, I will do the remaining work against your fork when I am ready to merge this, but I will take care of the rest of the work to get this merged

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.

Was able to use this end to end with Terraform to deploy an ARM64 Windows VM from a SIG definition created by Packer, with HCP Packer correctly having the location set, thanks again for this contribution!

@JenGoldstrich JenGoldstrich merged commit d5b422e into hashicorp:main Jan 18, 2023
@VladRassokhin VladRassokhin deleted the fix-210 branch March 19, 2024 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Does it support ARM64?
3 participants