-
Notifications
You must be signed in to change notification settings - Fork 121
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
Move to AWS v3 library #569
base: master
Are you sure you want to change the base?
Conversation
Updated to not hard require v3, but rather allow it as an option, and remove some cruft comments that I added to the patch |
Looks like you can squash this down to a single commit. There's also some Travis failures going on. |
e51a3df
to
e5dd141
Compare
@tas50 both facts - the travis one caught a corner case which I had not. huzzah for tests. (also squashed now) |
@jjlimepoint I'd really like to migrate this gem to the V3 library, but I'd like to make sure we use the individual gems instead of the monolithic gem. There's a lot of code we can shed off if we move to the appropriate v3 gems. Is that something you want to tackle or should I close this out and take a stab myself? |
I can do that tomorrow most likely - I mostly avoided it initially in a vain attempt to maintain v2 compatibility - but i'm not super inclined to care about support for v2 if no-one else is! |
I'm working to get V3 into kitchen-ec2 and we'll need to do the same thing with inspec. It requires all 3 to land at the same time. Total pain, but we have to do it sometime. |
5d52eb7
to
2400eec
Compare
Signed-off-by: Jaymz Julian <[email protected]>
chef-provisioning-aws.gemspec
Outdated
s.add_dependency "aws-sdk-ec2", [">= 1.42.0", "< 2.0"] | ||
s.add_dependency "aws-sdk-s3", [">= 1.17.0", "< 2.0"] | ||
s.add_dependency "aws-sdk-rds", [">= 1.0", "< 4.0"] | ||
s.add_dependency "aws-sdk-route53", [">= 1.0", "< 4.0"] |
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 resource deps should all be ~> 1.0 since Amazon is on 1.x right 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.
Good catch - thanks for that!
@@ -1,6 +1,5 @@ | |||
require "chef/provider/lwrp_base" | |||
require "chef/provisioning/aws_driver/aws_provider" | |||
require "aws-sdk" |
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 should be require "aws-sdk-ec2"
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.
At the time I actually removed this deliberatly, since the class doesn't actually use anything from aws-sdk-* directly at all, but rather the things provided by aws_provider. Still, not real harm in specifying it.
Signed-off-by: Jaymz Julian <[email protected]>
Signed-off-by: Jaymz Julian <[email protected]>
As the world turns, so do library versions. Move to AWS v3 - note that this has ONLY been tested on my integration test rig, which means only ec2 / custom networking / vpc, s3, and elb have been tested, however I'm pretty confident in the safety given that those required no changes beyond the gemfile.
That said, I did bump the major version since v3 aws is a major change that affects gem dependencies. It would work as v2/v3 allowed, if we wanted to do that, though I haven't done that in this PR due to test coverage issues (I wanted something testable in my environment...)
It probably depends on the PR for provisioning being removed from DK, since DK uses aws v2 (though their PR says they want to use v3 as well, and prov-aws is holding them back, so shrug)
Someone other than me should probably test this before merging :)