-
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
Add support for Launch Templates, custom AMI and more features for Managed Nodegroups #2544
Add support for Launch Templates, custom AMI and more features for Managed Nodegroups #2544
Conversation
b12e84b
to
f8f49f0
Compare
Some tests are breaking in ginkgo v1.14.0
45d5ca3
to
ca07ffe
Compare
Expect(err).ToNot(HaveOccurred()) | ||
Expect(template).ToNot(BeNil()) | ||
|
||
actual, err := json.Marshal(template.Resources) |
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.
Am I reading this correctly that the tests unmarshal and marshal and then compare the json? Why not match on the structs?
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.
That's right. The unmarshalling (goformation.ParseJSON(bytes)
) and marshalling is actually testing goformation's ability to preserve isomorphism, to ensure no types or values are lost or are interpreted differently.
Why not match on the structs?
Matching on the structs will require more code than matching on the expected JSON output. This is also close to an end-to-end test.
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.
makes sense!
err := stack.AddAllResources() | ||
assert.Nil(err) | ||
require.Nil(err) |
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.
😯 What's the difference?
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.
The methods in require
stop the test (calling FailNow
) when an assertion fails.
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.
ah ok so probably it should always be require
👍
Can we also add some integration tests? |
3efe52e
to
e0b6773
Compare
I was planning to but couldn't get to it. I'll address it in a separate PR. WDYT? |
c92d99c
to
006fd3b
Compare
006fd3b
to
e23dba1
Compare
Did this PR remove the network interface from the created launch template? If yes why? |
Description
Adds launch template support for managed nodegroups. Also adds support for the following features for managed nodegroups:
ami
)instancePrefix
andinstanceName
to configure the EC2 instance name for a nodegroupsecurityGroups.attachIDs
to add custom security groupsebsOptimized
,volumeType
,volumeName
,volumeEncrypted
,volumeKmsKeyID
andvolumeIOPS
maxPodsPerNode
preBootstrapCommands
andoverrideBootstrapCommand
disableIMDSv1
To use the launch templates features, specify an existing launch template:
TODO:
Closes #2551
Checklist
README.md
, or theuserdocs
directory)area/nodegroup
), target version (e.g.version/0.12.0
) and kind (e.g.kind/improvement
)