-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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
Block Device Support for Autoscaling Launch Configurations #1271
Block Device Support for Autoscaling Launch Configurations #1271
Conversation
👍 |
This is looking really good! I think the last iteration we need before landing this is to pull it in line with the reworking of block devices that landed in master. #1045 I looked into the I think a lot of the code over in If this all sounds super scary to you, let us know and one of our contributors can try to pick it up. Thanks again for all the work on this! 😀 |
|
||
func readBlockDevicesFromLaunchConfiguration(d *schema.ResourceData, launchConfiguration *autoscaling.LaunchConfiguration, autoscalingconn *autoscaling.AutoScaling) map[string]interface{} { | ||
var blockDevices = make(map[string]interface{}) | ||
// Need to figure out how to determine the various instance types. |
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.
@phinze, I mimic'd the write functionality of the EC2 Block instances. How do I determine whether a device is root, ephemeral, or ebs on the read?
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.
@jwaldrip I'm recommending pretty much copying the schema from instances and the block device handling code. I think it should translate pretty decently to the LC APIs.
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 believe I already did that. But there is a read section that I don't exactly know how to follow.
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.
Oh! Sorry @jwaldrip I definitely responded without catching that there were new commits. 🙈
Let me take a look!
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.
Okay, this is super rough and probably won't compile, I think it could look something like this: https://gist.github.com/phinze/01f6163bf452eee289d5
Basically, checking the DeviceName
against the RootDeviceName
of the AMI, leaning on a helper living over in resource_aws_instance.go
to grab it for us.
Should be good to go. But I will let you all be the judge. |
Type: schema.TypeMap, | ||
Optional: true, | ||
Removed: "Split out into three sub-types; see Changelog and Docs", | ||
}, |
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.
No need to reproduce the deprecation warning here since we never supported it in the first place.
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.
@phinze Should I just pull out block device all together? Since it alone was never supported in the first place?
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.
Yep!
Nice! Almost there - just a couple of tweaks I mentioned. Thanks for all your work on this! 🙇 |
Nice work on this. I was playing around with this myself but you beat me to the PR. 👍 |
@jwaldrip - Would love to get this in for 0.4 (which we cut tomorrow) - let me know if you can't get to the final tweaks and I can pick'em up myself. 👍 |
working on it right now. |
Cool! Keep me posted. 🙆♀️ |
Should be good to go! |
@phinze think it will get into the release? |
Nice! Yep this will make it in. LGTM - I'll give the acc. tests a quick run and then merge. 👍 Great work on this! |
Great! would be nice to squash this into a single commit as well. |
+1 @jwaldrip can you squash? |
@phinze squashed! |
@jwaldrip terraform crashes if there is no block device information in plan. In our use case we were relying on AMIs defaults. |
Yeah I'm rooting out several issues - here's my WIP so far: 6130d97 |
@phinze I don't want to step on your toes if you are already fixing the issues. Or do you want me to jump on this as well? |
@jwaldrip i think i might see the light at the end of the tunnel 😀 give me a few more minutes here and I'll let you know. I'm curious, though, was everything working locally for you? |
I am checking again against my project. |
Yep. Panic'd ... my bad. I started rushing near the end there. How are things looking? |
Looking good! Going to push a fresh PR with my tweaks so @pmoust can give us one more double check before merge. |
Okay moving to #1364 to finish this out! 👌 |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
No description provided.