feat: add bottlerocket ami types #2352
Conversation
jokestax
requested review from
patrickdappollonio,
CristhianF7 and
nathan-nicholson
and removed request for
patrickdappollonio
There was a problem hiding this comment.
There are a lot of functions with in-line defined dependencies (primarily clients to AWS) that will make this almost impossible to properly test logic paths without a lot of effort and real resources.
I may take a stab at refactoring/pairing on this, but I'm not comfortable with merging with its current structure.
jokestax
changed the title
cmd/aws/create_test.go
Outdated
| type mockInstanceTypesPaginator struct { | ||
| instanceTypes []ec2Types.InstanceTypeInfo | ||
| err error | ||
| called bool | ||
| } | ||
|
|
||
| func (m *mockInstanceTypesPaginator) HasMorePages() bool { | ||
| return !m.called | ||
| } | ||
|
|
||
| func (m *mockInstanceTypesPaginator) NextPage(ctx context.Context, opts ...func(*ec2.Options)) (*ec2.DescribeInstanceTypesOutput, error) { | ||
| if m.err != nil { | ||
| return nil, m.err | ||
| } | ||
| m.called = true | ||
| return &ec2.DescribeInstanceTypesOutput{ | ||
| InstanceTypes: m.instanceTypes, | ||
| }, nil | ||
| } | ||
|
|
||
| type mockSSMClient struct { | ||
| ssm.Client | ||
| parameterValue string | ||
| err error | ||
| } | ||
|
|
||
| func (m *mockSSMClient) GetParameter(ctx context.Context, input *ssm.GetParameterInput, opts ...func(*ssm.Options)) (*ssm.GetParameterOutput, error) { | ||
| if m.err != nil { | ||
| return nil, m.err | ||
| } | ||
| return &ssm.GetParameterOutput{ | ||
| Parameter: &ssmTypes.Parameter{ | ||
| Value: &m.parameterValue, | ||
| }, | ||
| }, nil | ||
| } | ||
|
|
||
| type mockEC2Client struct { | ||
| ec2.Client | ||
| images []ec2Types.Image | ||
| architecture string | ||
| err error | ||
| } | ||
|
|
||
| func (m *mockEC2Client) DescribeImages(ctx context.Context, input *ec2.DescribeImagesInput, opts ...func(*ec2.Options)) (*ec2.DescribeImagesOutput, error) { | ||
| if m.err != nil { | ||
| return nil, m.err | ||
| } | ||
| if m.architecture == "x86_64" { | ||
| return &ec2.DescribeImagesOutput{ | ||
| Images: []ec2Types.Image{ | ||
| { | ||
| Architecture: ec2Types.ArchitectureValues("x86_64"), | ||
| }, | ||
| }, | ||
| }, nil | ||
| } else if m.architecture == "" { | ||
| return &ec2.DescribeImagesOutput{ | ||
| Images: m.images, | ||
| }, nil | ||
| } else { | ||
| return nil, errors.New("unexpected architecture") | ||
| } | ||
| } |
There was a problem hiding this comment.
These are still using the old fashion approach where instead of passing the function, you pass the returned values, making them usable exactly once before you have to reinstantiate them again.
There was a problem hiding this comment.
I am unclear and would need your help ig 😅
There was a problem hiding this comment.
What Patrick is saying is that you can define a mock implementation of DescribeImages to the mock struct instead of using this single definition. You can see an example of this on the mockCredentialsProvider. It's a function with the same signature as the Method, but that can be dynamically defined for each test case.
There was a problem hiding this comment.
Yes, sorry, but exactly what Nathan said. I explained it in detail in one of my previous comments but I think because you edited the file either Github or you marked the issue as "resolved", so the comment is collapsed now.
cmd/aws/create_test.go
Outdated
| creds, err := getSessionCredentials(context.Background(), mockProvider) | ||
| if tt.expectedErr != nil { | ||
| require.Nil(t, creds) | ||
| require.EqualError(t, err, tt.expectedErr.Error()) |
There was a problem hiding this comment.
I mentioned in the previous review but it seems it was marked as resolved without actually resolving it. This is a very weak validation that depends on the fact that a) both haveto be error types, and b) both error string versions have to match. Not sure you want to do that for the reasons I provided before.
The same applies for the rest of the checks in the unit tests below.
Signed-off-by: nathan-nicholson <[email protected]>
Signed-off-by: nathan-nicholson <[email protected]>
Signed-off-by: nathan-nicholson <[email protected]>
Signed-off-by: nathan-nicholson <[email protected]>
Signed-off-by: nathan-nicholson <[email protected]>
Signed-off-by: nathan-nicholson <[email protected]>
|
I didn't finish completely replacing the mocks, but I'm calling it for today. Spent more time fighting table tests than I'm proud to admit. May have been easier to just do individual test cases for most of these 🤷 |
Description
Add support for Bottlerocket AMI types in the AWS cloud CLI.
Testing
Provision a cluster using the CLI with the
--ami-type <AMI_TYPE>flag.Example Command:
Supported AMI Types
AL2_x86_64AL2_ARM_64BOTTLEROCKET_ARM_64BOTTLEROCKET_x86_64BOTTLEROCKET_ARM_64_NVIDIABOTTLEROCKET_x86_64_NVIDIA