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

Add require 'aws-sdk' as needed #183

Merged
merged 1 commit into from
Jan 19, 2016

Conversation

shortdudey123
Copy link
Contributor

The aws-sdk gem namespace was being called before it was required.
Solves #181

The aws-sdk gem namespace was being called before it was required.
Solves sous-chefs#181
@tas50
Copy link
Contributor

tas50 commented Nov 25, 2015

+1

@tas50
Copy link
Contributor

tas50 commented Nov 25, 2015

Avoids the need for including the default recipe if the user installs the SDK via the chef-client cookbooks or via another method.

@miketheman
Copy link
Contributor

I don't I understand why this solves the problem.

In these lines, create_aws_interface is meant to require & initialize the client.

If the @@ec2 object is empty at the time, then this method should be invoked.

It's entirely possible that the class variable name is a misnomer, and that it should be renamed to @@aws and used globally to instantiate an AWS Client interface and that create_aws_interface should be pulled out of EC2-only land.

Will this solve a particular immediate problem? Maybe. Will it continue to prove confusing for others? More likely.

@shortdudey123
Copy link
Contributor Author

create_aws_interface can't be called if the namespace is not valid (i.e. the gem has not been required yet)

@miketheman
Copy link
Contributor

@shortdudey123 Then I must be missing something, because it's right here at the end of the default recipe: https://github.com/chef-cookbooks/aws/blob/master/recipes/default.rb#L26

@shortdudey123
Copy link
Contributor Author

This allows the LWRP to be used without including the default recipe.

Use case 1: aws-sdk is already installed via another method (i.e. another recipe)
Use case 2: the default recipe is included in bootstrapping the host, then not included again

@miketheman
Copy link
Contributor

I guess that's reasonable, but the cookbook explicitly states in it's usage that it expects the default recipe to be run prior to any library functions: https://github.com/chef-cookbooks/aws#defaultrb

I think that if the desire is to make it a better method for calling the code via LWRP without the default recipe (which, anecdotally, should be safe on every run, what's the danger of including it in a node's run list?), then this would probably be structured in a better manner to DRY up the invocations of require/rescue - it's pretty much repetition at this point.

@shortdudey123
Copy link
Contributor Author

but the cookbook explicitly states in it's usage that it expects the default recipe to be run prior to any library functions

Fair enough :)
You can close this if always including the default recipe makes more sense on your end

@lamont-granquist
Copy link
Contributor

Pulling the code from the recipe into the provider is correct.

Unless someone steps forward to actually do the work of DRY'ing up the requires in the library code, this should probably just get merged, otherwise we're blocking on nobody having enough time to clean it up right and making the perfect the enemy of the good.

👍 we should just ship this unless a better PR appears soon.

@thommay
Copy link

thommay commented Dec 1, 2015

yeah, this is obviously the right thing to do for the cookbook's ergonomics. 👍

tas50 added a commit that referenced this pull request Jan 19, 2016
@tas50 tas50 merged commit 7d61a16 into sous-chefs:master Jan 19, 2016
@shortdudey123 shortdudey123 deleted the add_needed_gem_require branch February 22, 2016 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants