-
Notifications
You must be signed in to change notification settings - Fork 466
kube-aws: validate that the key pair exists #362
Conversation
#346 exposes a common aws session via the cluster.Cluster struct. Will wait on that. |
Nice, thought that session stuff smelled a little funny. |
5e5ddd4
to
6704b2d
Compare
Had to move some stuff around to avoid a cyclic dependency. Now that session is in config, the cluster. Cluster struct seems pretty pointless but I left it around to minimize the scope of this PR. If you all are fine with these changes it'd be great to get them merged sooner rather than later since they'll affect my other PRs |
@cgag drop the last commit and we should be good to go |
@@ -193,6 +195,27 @@ func (c Cluster) stackConfig(opts StackTemplateOptions, compressUserData bool) ( | |||
return &stackConfig, nil | |||
} | |||
|
|||
func (c Cluster) ValidateKey() error { |
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.
Move this to cluster package.
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.
done
@@ -54,8 +53,10 @@ func runCmdValidate(cmd *cobra.Command, args []string) error { | |||
} | |||
|
|||
if err != nil { | |||
return fmt.Errorf("Error creating cluster: %v", 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.
This was just leading to output like "Error creating cluster: Error: Key does not exist". I think the "Validating stack template..." output added in the validate command makes the extra messaging unnecessary.
Moved the validateKey call to cluster.Create after an offline discussion with @colhom. |
@@ -254,3 +263,22 @@ func (c *Cluster) Destroy() error { | |||
_, err := cfSvc.DeleteStack(dreq) | |||
return err | |||
} | |||
|
|||
func (c *Cluster) validateKey() error { |
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.
Check out how validateExistingVPCState
is set up.
In the unit tests for pkg/cluster
, we "mock up" the ec2 service and test that validateExistingVPCState is behaving correctly.
The same should be done for validateKey()
.
- Extend
ec2Service
interface definition to include methods you need - Make
validateKey
takeec2Service
interface parameter - Extend
dummyEc2Service
incluster_test.go
so that it implements the new methods - Write a few unit tests in cluster_test.go for
validateKey
On that note, we have multiple keys in play here. Maybe change the name to something less general: validateKeyPair
or something to that effect?
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.
done
50c6343
to
6449c65
Compare
func (svc dummyEC2Service) DescribeVpcs(input *ec2.DescribeVpcsInput) (*ec2.DescribeVpcsOutput, error) { | ||
output := ec2.DescribeVpcsOutput{} | ||
for _, vpcId := range input.VpcIds { | ||
if vpc, ok := svc[*vpcId]; ok { | ||
for _, vpcID := range input.VpcIds { |
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 capitalization makes golint happy, but it's inconsistent with the amazon's naming. I'd prefer to not propagate their deviancy, but I can change this back if we'd rather be consistent.
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.
golint is our source of truth in these matters.
LGTM |
fixes #331