-
Notifications
You must be signed in to change notification settings - Fork 216
Adds BlockDevices for LaunchConfigurations #210
Adds BlockDevices for LaunchConfigurations #210
Conversation
Oops - I actually think I need to change BlockDevices to BlockDeviceMappings in a few places. |
fa69b7a
to
65e7c75
Compare
65e7c75
to
91eb1b9
Compare
👍 I definitely need this for a project I'm working on. I hope it can be reviewed and merged soon. Anything I can do to help or test? |
@statik just waiting for review by a maintainer. It seems to work when paired with hashicorp/terraform#935. I've never written a line of Go before this, so if you have a min to do a quick review it would be helpful to avoid any back-and-forth due to stupid errors when the maintainers get a chance to look at it 😄. |
@@ -298,6 +355,7 @@ func (autoscaling *AutoScaling) CreateLaunchConfiguration(options *CreateLaunchC | |||
b64.Encode(userData, []byte(options.UserData)) | |||
params["UserData"] = string(userData) | |||
} | |||
addBlockDeviceParams("", params, options.BlockDevices) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're always passing an empty string for the first arg, should we just drop the prename
arg?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing he just copied the function from another package. Its fine, we'll switch to aws-go soon enough anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct - copied from the ec2 package.
Hey there - thanks for this! One quick question inline but otherwise looking good! |
LGTM. |
Adds BlockDevices for LaunchConfigurations
Adds Block Device support for Launch Configurations.
The code is mostly copied from the EC2 package and tests.