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 s3 bucket tagging support #2322

Merged
merged 6 commits into from Oct 29, 2013

Conversation

Projects
None yet
3 participants
@chrisroberts
Copy link
Contributor

chrisroberts commented Oct 25, 2013

No description provided.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 25, 2013

Coverage Status

Coverage remained the same when pulling e7a6e55 on chrisroberts:feature/s3-tags into 052768c on fog:master.

1 similar comment
@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 25, 2013

Coverage Status

Coverage remained the same when pulling e7a6e55 on chrisroberts:feature/s3-tags into 052768c on fog:master.

@geemus

This comment has been minimized.

Copy link
Member

geemus commented Oct 25, 2013

@chrisroberts - looks like a great start. Could you add some basic tests around the new functionality as well?

@chrisroberts

This comment has been minimized.

Copy link
Contributor Author

chrisroberts commented Oct 28, 2013

@geemus something like that?

@geemus

This comment has been minimized.

Copy link
Member

geemus commented Oct 28, 2013

@chrisroberts - definitely on the right track. If we could update put tags to set something on the hash (and get tags to read that) I think it will be good-to-go. Sound like a plan?

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 28, 2013

Coverage Status

Coverage remained the same when pulling fda026d on chrisroberts:feature/s3-tags into 052768c on fog:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 28, 2013

Coverage Status

Coverage increased (+1.03%) when pulling f7815fb on chrisroberts:feature/s3-tags into 052768c on fog:master.


def get_bucket_tagging(bucket_name)
response = Excon::Response.new
if bucket = self.data[:buckets][bucket_name] && bucket[:tagging]

This comment has been minimized.

@geemus

geemus Oct 28, 2013

Member

It is probably safer to namespace the tag stuff, so maybe something like self.data[:bucket_tagging][bucket_name]. otherwise I worry a bit it might butt heads with other stuff and break.

@@ -279,6 +279,22 @@
Fog::Storage[:aws].delete_bucket(@aws_bucket_name)
end

tests("bucket tagging") do

This comment has been minimized.

@geemus

geemus Oct 28, 2013

Member

Due to the way the tests run (in order throughout the file), these are failing, as they follow delete bucket tests (which delete the bucket, so at this point it doesn't exist to do tag operations to). Could you tweak/rearrange that?

@geemus

This comment has been minimized.

Copy link
Member

geemus commented Oct 28, 2013

Awesome, couple more small comments above.

@chrisroberts

This comment has been minimized.

Copy link
Contributor Author

chrisroberts commented Oct 28, 2013

@geemus Okay, is that looking a little better?

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 28, 2013

Coverage Status

Coverage decreased (-1.79%) to 63.93% when pulling 9628ee7 on chrisroberts:feature/s3-tags into 052768c on fog:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 28, 2013

Coverage Status

Coverage remained the same when pulling 7ad25c3 on chrisroberts:feature/s3-tags into 052768c on fog:master.

response = Excon::Response.new

if self.data[:buckets][bucket_name]
self.data[:bucket_tagging] ||= {}

This comment has been minimized.

@geemus

geemus Oct 28, 2013

Member

I would actually go ahead and add this to the default mock data, here: https://github.com/fog/fog/blob/master/lib/fog/aws/storage.rb#L327

That way you can always assume it is there, which also allows you to simplify the code for delete above (since you can assume the hash at least exists). Make sense?

@geemus

This comment has been minimized.

Copy link
Member

geemus commented Oct 28, 2013

Yeah, tests are looking great. One more suggestions above to cleanup the mock data storage and I think it should be good-to-go. Thanks!

@chrisroberts

This comment has been minimized.

Copy link
Contributor Author

chrisroberts commented Oct 28, 2013

@geemus Okay, cool. Missed the data init there, so moved the :bucket_tagging over there and removed the init/checks from the request mocks.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 28, 2013

Coverage Status

Coverage remained the same when pulling c3f6ae5 on chrisroberts:feature/s3-tags into 052768c on fog:master.

@geemus

This comment has been minimized.

Copy link
Member

geemus commented Oct 29, 2013

@chrisroberts awesome. Thanks so much for your patience and understanding in iterating through this. Looks great!

geemus added a commit that referenced this pull request Oct 29, 2013

Merge pull request #2322 from chrisroberts/feature/s3-tags
Add s3 bucket tagging support

@geemus geemus merged commit 37d5081 into fog:master Oct 29, 2013

1 check passed

default The Travis CI build passed
Details

@chrisroberts chrisroberts deleted the chrisroberts:feature/s3-tags branch Oct 29, 2013

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.