Migrate from mitchellh/goamz to awslabs/aws-sdk-go #2034
Conversation
This commit moves the Amazon builders of Packer away from the Hashicorp fork of the goamz library to the official AWS SDK for Go, in order that third party plugins may depend on the more complete official library more easily.
|
Looks like it fails in Go 1.2 because the AWS client uses the |
|
I would think Go 1.2 is ancient history now, but it is not my call to make. @mitchellh ? I'm looking forward to Packer using the AWS Go SDK. |
| newDevice := device | ||
| if newDevice.DeviceName == image.RootDeviceName { | ||
| newDevice.SnapshotId = snapshotId | ||
| newDevice.EBS.SnapshotID = &snapshotId |
There was a problem hiding this comment.
Is there any chance that newDevice.EBS is uninitialized, and this could panic?
There was a problem hiding this comment.
Yes I think that's likely to be an issue in one or two other places also, let me look through and push some fixes. Good catch.
| } | ||
|
|
||
| if s.instance.PrivateIpAddress != "" { | ||
| ui.Message(fmt.Sprintf("Private IP: %s", s.instance.PrivateIpAddress)) | ||
| if *s.instance.PrivateIPAddress != "" { |
There was a problem hiding this comment.
same warning here on possibly nil attributes causing panics
|
I'm a bit out of my element, but overall this looks good. Some minor issues where there could maybe be panics, but I lack context to say for sure. I support the idea of moving beyond Go 1.2, but I defer that call. Post nil guards, 👍 |
|
We can drop Go 1.2 support |
This commit adds extra nil checks for some pointers which were not necessary when using goamz
|
@catsby thanks for looking over this. I think I've addressed all the issues raised with the subsequent commits. I've gone ahead and removed Go 1.2 as a build target for Travis as well as per @sethvargo. Do you want these commits squashed/rebased etc? |
|
@jen20 you don't need to squash, but feel free to rebase 😄 |
|
@sethvargo just realised it's actually already rebased on top of |
| @@ -86,3 +100,24 @@ func (c *AccessConfig) Prepare(t *packer.ConfigTemplate) []error { | |||
|
|
|||
| return nil | |||
| } | |||
|
|
|||
| func GetInstanceMetaData(path string) (contents []byte, err error) { | |||
| url := "http://169.254.169.254/latest/meta-data/" + path | |||
There was a problem hiding this comment.
Do you think this should be a constant? I'm surprised this isn't functionality exposed by the SDK. Can you check on that?
There was a problem hiding this comment.
There is a PR for that, but not merged yet: aws/aws-sdk-go#182
There was a problem hiding this comment.
Yes I'd noticed the pull request and will update this accordingly when it's merged (at the moment this code doesn't retry etc which I've not sen to be an issue so far but you never know). For now that implementation is mostly lifted out of the goamz library, but with http.Client instead of the retrying client that library uses. @sethvargo do you want me to look more into this?
There was a problem hiding this comment.
Hey @jen20 we are probably going to triage Packer in a few days. Can you keep an eye on the upstream and, if 182 is merged, update here accordingly? If not, we will merge as is and can update in the future. How does that sound?
There was a problem hiding this comment.
Yup, will monitor it and update as necessary.
There was a problem hiding this comment.
aws/aws-sdk-go#192 appears to be the pull request which needs tracking for now.
|
@sethvargo doesn't look like there's about to be any action on either of the PRs to aws-sdk-go. If you want to merge this as is I'll update with another PR when that functionality gets added. |
|
This LGTM! I'm going to merge this then work out any conflicts. |
|
@mitchellh ah I didn't notice there were conflicts (think they're fairly recent). You want me to rebase? |
|
@jen20 No worries I fixed them and merged :) |
|
I ran the [new!] acceptance tests on the AWS stuff and it works, too. :) |
|
Ah excellent. Is there a release of packer coming soon? Will update https://github.com/packer-community/packer-windows-plugins shortly (/cc: @mefellows) |
ghost
locked and limited conversation to collaborators
This commit moves the Amazon builders of Packer away from the Hashicorp fork of the goamz library to the official AWS SDK for Go, in order that third party builders etc (such as the Windows ones at Packer-Community) may depend on the more complete official library more easily.
I made the (possibly bad!) assumption that since Terraform has switched over in the last release it would also be desirable for Packer to do so.
I appreciate that this is a rather large set of changes to drop in one commit. All the tests pass, and I've verified the behaviour is as I expect in all the use cases I can. I assume there may be another test suite somewhere which tests a more exhaustive range of templates however?