Skip to content
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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jjlimepoint
Copy link
Contributor

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 :)

@jjlimepoint
Copy link
Contributor Author

Updated to not hard require v3, but rather allow it as an option, and remove some cruft comments that I added to the patch

@tas50
Copy link
Contributor

tas50 commented Jul 12, 2018

Looks like you can squash this down to a single commit. There's also some Travis failures going on.

@jjlimepoint jjlimepoint force-pushed the aws-v3 branch 2 times, most recently from e51a3df to e5dd141 Compare July 13, 2018 03:29
@jjlimepoint
Copy link
Contributor Author

@tas50 both facts - the travis one caught a corner case which I had not. huzzah for tests. (also squashed now)

@tas50
Copy link
Contributor

tas50 commented Aug 20, 2018

@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?

@jjlimepoint
Copy link
Contributor Author

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!

@tas50
Copy link
Contributor

tas50 commented Aug 20, 2018

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.

Signed-off-by: Jaymz Julian <[email protected]>
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"]
Copy link
Contributor

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.

Copy link
Contributor Author

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"
Copy link
Contributor

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"

Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants