-
Notifications
You must be signed in to change notification settings - Fork 13
Conversation
iaas/amazonec2/amazonec2.go
Outdated
driverOpts.Values["swarm-strategy"] = "spread" | ||
driverOpts.Values["swarm-discovery"] = "" | ||
driverOpts.Values["swarm-master"] = false | ||
driver.SetConfigFromFlags(&driverOpts) |
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.
Error return value of driver.SetConfigFromFlags
is not checked
Codecov Report
@@ Coverage Diff @@
## master #111 +/- ##
=========================================
- Coverage 53.55% 51.9% -1.65%
=========================================
Files 5 6 +1
Lines 450 524 +74
=========================================
+ Hits 241 272 +31
- Misses 185 228 +43
Partials 24 24
Continue to review full report at Codecov.
|
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.
README.md on examples/amazonec2
, I believe it's a place to explain how to use
It would be important to create for the other examples too
driverOpts.Values[f.String()] = false | ||
} | ||
} | ||
// TODO: receive this configs to remove hard coded values |
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.
why have this todo ?
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.
It's not the ideal scenario to have this configurations hard coded, instead, we should receive them as parameters, or (my preferred option) get the default values just like the docker-machine binary uses when creating a machine via cli, but honestly, I wasn't able to replicate this behavior.
driverOpts.Values[f.String()] = false | ||
} | ||
} | ||
// TODO: receive this configs to remove hard coded values |
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.
you can implement functional option pattern so this values can be override easier. An exemple https://github.com/nuveo/storing/blob/master/awss3/awss3.go
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 don't think I need to override this values, the better approach is just replicate the same default values that docker-machine uses. Right now, they are hard coded, the ideal would be retrieve them somehow, maybe something like the driver.GetCreateFlags()
method, but retrieving the swarm values.
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.
LGTM
This PR adds support for usage of EC2 from AWS to gofn, simply instantiating a new provider with secret and access keys.
Example to verify execution:
https://github.com/gofn/gofn/tree/improvements/aws_ec2/examples/amazonec2
ref #54