-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Dynamically lookup the RootDeviceName of the AMI being used #779
Dynamically lookup the RootDeviceName of the AMI being used #779
Conversation
…oot devices that do not use /dev/xvda Issue #774
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.
Thank you! It is looking good, just a few minor comments...
Added length check to the results in LookupRootDeviceName Moved the root device lookup into ctl.EnsureAMI
@errordeveloper as the ami.IsAvailable function can also provide the info that I am looking up in my function, how about merging them, something like this?
We could then check if the type is instance-store and error if VolumeSize has also been set.
|
Thanks, Adam! This is looking good. |
Merged IsAvailable and RootDeviceLookup Added check for instance-store amis
Please let me know if there is anything else I need to do. |
@adamjohnson01 do you want to make sure all of your commits show up with your profile image? This usually comes down to email address being use in the commit. |
@errordeveloper, I have added the email in my commits to my profile now. Thanks! |
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.
Just one last suggestion! :)
…nd configuration into it
@errordeveloper. No problem, that is done now. |
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.
This looks great, to thanks a lot! I will have another look, but will push any changes myself before merging - just stuff that maybe quicker to do then explain. Thanks again :)
Description
The cfn/builder/nodegroup.go was using a hardcoded value for DeviceName. This meant it was not possible to manage the size of the root volume for AMIs that did not use /dev/xvda as the RootDeviceName. I have added a lookup function that will retrieve the value from the AMI being used and populate the value in the launchTemplateData.BlockDeviceMappings.
Closes #774
Checklist
make build
)make test
)make integration-test
)humans.txt
file