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

dynamically install rest-client #36

Merged
merged 2 commits into from Dec 9, 2014

Conversation

Projects
None yet
4 participants
@joekiller
Copy link
Collaborator

commented Dec 9, 2014

Proposed hacky fix chef 12 removing rest-client. Fixes #34

@evalencia @salgo @jeffbyrnes @edwardvfluke could you try this fix?

def self.get_from_s3(bucket,url,path,aws_access_key_id,aws_secret_access_key,token)
def self.get_from_s3(bucket,url,path,aws_access_key_id,aws_secret_access_key,token)
require 'rest-client'
RestClient.proxy = ENV['http_proxy']

This comment has been minimized.

Copy link
@jeffbyrnes

jeffbyrnes Dec 9, 2014

Might be good to DRY up these two lines, which are in quite a few places, into a single S3FileLib::connection method, or some other similarly-named thing. cc @eherot

This comment has been minimized.

Copy link
@eherot

eherot Dec 9, 2014

Contributor

Seconded. The "right" way to do this would be to make the connection an object itself (by defining a new class) and then put the require in the initialize method for that class. It would also allow you to remove a lot of redundant code from the other methods.

This comment has been minimized.

Copy link
@joekiller

joekiller Dec 9, 2014

Author Collaborator

Right, good suggestion. Let me put that in.

@salgo

This comment has been minimized.

Copy link
Contributor

commented Dec 9, 2014

Yes this is much neater. Do you want me to re-submit the test kitchen work (which was in the previous PR) as a new PR?

@joekiller

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 9, 2014

@eherot @jeffbyrnes take a look at the DRY attempt. If it works I think I'm going to merge to allow @salgo to make a kitchen test pr that he worked on.

I like the suggestion of making it a whole class but is a bit more refactor than I want to do right now. Ideally I think we replace all the request stuff with Fog as we discussed in #30.

@jeffbyrnes

This comment has been minimized.

Copy link

commented Dec 9, 2014

@salgo yeah, the Test Kitchen bits would be great submitted on their own.

@jeffbyrnes

This comment has been minimized.

Copy link

commented Dec 9, 2014

@joekiller I know @eherot & I would be pleased as peaches to help with a Fog refactor.

@jeffbyrnes

This comment has been minimized.

Copy link

commented Dec 9, 2014

Also, testing out now. Have to stack a bit of our own work on top to deal with buckets w/ dots (because all of ours do), so hang tight.

@jeffbyrnes

This comment has been minimized.

Copy link

commented Dec 9, 2014

@joekiller ok, tests well with Chef 12.

@jeffbyrnes

This comment has been minimized.

Copy link

commented Dec 9, 2014

Handy bit, if you modify the description to say something like “Fixes #34”, when this is merged in, it’ll take care of the other issue.

@joekiller

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 9, 2014

Great will merge it soon and publish
On Dec 9, 2014 1:36 PM, "Jeff Byrnes" notifications@github.com wrote:

Handy bit, if you modify the description to say something like “Fixes #34
https://github.com/adamsb6/s3_file/issues/34”, when this is merged in,
it’ll take care of the other issue.


Reply to this email directly or view it on GitHub
#36 (comment).

@@ -122,4 +122,10 @@ def self.verify_md5_checksum(checksum, file)

local_md5.hexdigest == s3_md5
end

def self.client

This comment has been minimized.

Copy link
@eherot

eherot Dec 9, 2014

Contributor

👍

require 'json'

use_inline_resources

action :create do
rest_client_gem = Chef::Resource::ChefGem.new('rest-client', @run_context)
rest_client_gem.run_action :install
client = S3FileLib::client

This comment has been minimized.

Copy link
@eherot

eherot Dec 9, 2014

Contributor

nice

joekiller added a commit that referenced this pull request Dec 9, 2014

Merge pull request #36 from adamsb6/fix/34
dynamically install rest-client fix #34

@joekiller joekiller merged commit cf45245 into master Dec 9, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.