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

Add ability to EBS Surrogate builder to persist properties from Marketplace source image, such as product codes #455

Merged
merged 20 commits into from
Mar 4, 2024

Conversation

dwc0011
Copy link
Contributor

@dwc0011 dwc0011 commented Jan 30, 2024

Add option to swap the root device to an EBS surrogate and create a new AMI in addition to being able to register an AMI. This allows billing data to be retained when creating AMI's.

Closes #440

@dwc0011 dwc0011 requested a review from a team as a code owner January 30, 2024 14:37
@dwc0011
Copy link
Contributor Author

dwc0011 commented Feb 13, 2024

Wanted to bump this to see if we can someone has time to review this. Thanks

@lorengordon
Copy link
Contributor

@nywilken Sorry for the ping, but just wanted to check and see if we could get a review on this pr, or an idea whether we're on an acceptable track. What I really need to know is if there's no possibility of merging this, so we can refocus on trying to find some other way to manage the workflow. Thanks!

@nywilken
Copy link
Member

@nywilken Sorry for the ping, but just wanted to check and see if we could get a review on this pr, or an idea whether we're on an acceptable track. What I really need to know is if there's no possibility of merging this, so we can refocus on trying to find some other way to manage the workflow. Thanks!

@lorengordon @dwc0011 apologies I've been out sick for the last few weeks and circling back to this PR. We have plans to review and merge.

Copy link
Member

@nywilken nywilken left a comment

Choose a reason for hiding this comment

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

I took a first pass and left a few comments. I think the if statement is simple enough but I would like for the names to be a bit more explicit on the work that is being done. ImageMethod and VolumeMethod seem simple.

If left a question around the DeRegister step being needed. Please let me know.

builder/ebssurrogate/root_block_device.go Outdated Show resolved Hide resolved
builder/ebssurrogate/root_block_device.go Outdated Show resolved Hide resolved
builder/ebssurrogate/step_swap_volumes.go Outdated Show resolved Hide resolved
builder/ebssurrogate/builder.go Show resolved Hide resolved
builder/ebssurrogate/builder.go Outdated Show resolved Hide resolved
@nywilken
Copy link
Member

nywilken commented Feb 26, 2024

Hi @dwc0011 @lorengordon thanks again for pushing this change forward and for you patience with the review. Speaking about this change internally we found ourselves a bit confused on the approach. To be clear, we understand why and what is happening. But we do wonder if there is a different approach on using create or register that we can take to make this clearer to not confuse users who may already be relying on ebssurrogate.

To help push this forward @lbajolet-hashicorp and I would like to setup a call so we can chat a bit more. Would you be up for a video call this week? We are both based in EST timezone.

If you are up for it you can send an email to <packer at hashicorp dot com>

@lorengordon
Copy link
Contributor

Sure, @nywilken, we can hop on a call. I'm on Pacific Time, @dwc0011 is Eastern Time. Though, it looks like the email address was cutoff? If it's easier, I'm in the hangops slack as well, as @loren.

@lbajolet-hashicorp
Copy link
Contributor

Hi @lorengordon,

Thanks for the quick update; the email address was cutoff since Markdown interpreted it as raw html because of the braces, I've encased it in code markup, you should be able to see it now.

@dwc0011
Copy link
Contributor Author

dwc0011 commented Feb 26, 2024 via email

@lorengordon
Copy link
Contributor

Hi @lorengordon,

Thanks for the quick update; the email address was cutoff since Markdown interpreted it as raw html because of the braces, I've encased it in code markup, you should be able to see it now.

Roger that, email sent!

@lorengordon
Copy link
Contributor

@nywilken @lbajolet-hashicorp Per the offline discussion, Dennis renamed the new argument as a bool, use_create_image and updated the argument description. Please let us know if you need anything else!

Copy link
Contributor

@lbajolet-hashicorp lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

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

Hi @dwc0011, @lorengordon

Looks good to me! I left a mini nitpick on a function's name, but aside from that, we should be good to go very soon.
I'll let @nywilken do a quick final review in addition to mine before we merge, and we'll follow up with a release likely next week.

Thanks for the reroll and the chat earlier, good to see this feature in the plugin, really, thanks for your efforts!

builder/ebssurrogate/step_swap_volumes.go Outdated Show resolved Hide resolved
@dwc0011
Copy link
Contributor Author

dwc0011 commented Feb 29, 2024

Thank you for your time today, look forward to this being incorporated into packer!

Copy link
Member

@nywilken nywilken left a comment

Choose a reason for hiding this comment

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

Thanks again for the syncing and re-rolling this change. Overall it looks good. I left a couple of suggested changes to have the configuration attribute on the builder config and not within the ami_root_device config.

I'm running the acceptance test now and will report back if I run into any issues.

builder/ebssurrogate/builder_acc_test.go Outdated Show resolved Hide resolved
builder/ebssurrogate/root_block_device.go Outdated Show resolved Hide resolved
builder/ebssurrogate/builder_acc_test.go Show resolved Hide resolved
builder/ebssurrogate/builder.go Show resolved Hide resolved
builder/ebssurrogate/builder.go Outdated Show resolved Hide resolved
@lorengordon
Copy link
Contributor

Thanks again for the syncing and re-rolling this change. Overall it looks good. I left a couple of suggested changes to have the configuration attribute on the builder config and not within the ami_root_device config.

I'm running the acceptance test now and will report back if I run into any issues.

Dennis is out today, but will tackle the requested change next week. Or feel free to make the changes yourself if you want to close it sooner.

@dwc0011
Copy link
Contributor Author

dwc0011 commented Mar 4, 2024

@nywilken I have addressed all the moves and fixed the tests as well. I appreciate the feedback and pointing us in the right direction. I am new to go and packer in general and expected change requests, hopefully they are all complete and we are set!

Thank you again!

Copy link
Member

@nywilken nywilken left a comment

Choose a reason for hiding this comment

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

@dwc0011 thanks for making the updated changes and pushing this change forward. This all looks good and the added acceptance tests are all green. Cheers!

2024/03/04 11:59:30 [INFO] AWS Auth provider used: "SharedCredentialsProvider"
2024/03/04 11:59:30 Found region us-west-2
2024/03/04 11:59:30 [INFO] AWS Auth provider used: "SharedCredentialsProvider"
--- PASS: TestAccBuilder_EbssurrogateUseCreateImageTrue (198.57s)
=== RUN   TestAccBuilder_EbssurrogateUseCreateImageFalse
2024/03/04 12:01:32 [INFO] AWS Auth provider used: "SharedCredentialsProvider"
2024/03/04 12:01:32 Found region us-west-2
2024/03/04 12:01:32 [INFO] AWS Auth provider used: "SharedCredentialsProvider"
--- PASS: TestAccBuilder_EbssurrogateUseCreateImageFalse (122.11s)
=== RUN   TestAccBuilder_EbssurrogateUseCreateImageOptional
2024/03/04 12:03:03 [INFO] AWS Auth provider used: "SharedCredentialsProvider"
2024/03/04 12:03:03 Found region us-west-2
2024/03/04 12:03:03 [INFO] AWS Auth provider used: "SharedCredentialsProvider"
--- PASS: TestAccBuilder_EbssurrogateUseCreateImageOptional (91.61s)
PASS
ok      github.com/hashicorp/packer-plugin-amazon/builder/ebssurrogate  413.330s

@nywilken nywilken changed the title Ebssurrogate swap volumes Add ability to EBS Surrogate builder to persist properties from Marketplace source image, such as product codes Mar 4, 2024
@nywilken nywilken merged commit 2aebcef into hashicorp:main Mar 4, 2024
12 checks passed
@nywilken
Copy link
Member

nywilken commented Mar 4, 2024

We release every Tuesday so this will go out tomorrow. If I can get it released today I will trigger things earlier. Thanks again for making this change and for working with @lbajolet-hashicorp and I to better understand the needed.

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.

Need an improved chroot-builder
4 participants