Skip to content
This repository was archived by the owner on Jul 13, 2023. It is now read-only.

default permission should assume aws v2 if both aws versions are in use - #1986

Closed
betesh wants to merge 1 commit into
thoughtbot:masterfrom
betesh:aws_v2
Closed

default permission should assume aws v2 if both aws versions are in use#1986
betesh wants to merge 1 commit into
thoughtbot:masterfrom
betesh:aws_v2

Conversation

@betesh

@betesh betesh commented Sep 2, 2015

Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Align the parameters of a method call if they span more than one line.

@tute

tute commented Sep 9, 2015

Copy link
Copy Markdown
Contributor

These conditionals would not exist if we had two different files, one for each branch. This PR doesn't change behavior or simplify the code, so I won't merge it for now. I am very interested in simplifying this into two files: s3-aws1.rb, and s3-aws2.rb. We can merge that PR if you'd like to work on it!

Thank you very much for your contribution.

@tute tute closed this Sep 9, 2015
@betesh

betesh commented Sep 9, 2015

Copy link
Copy Markdown
Contributor Author

This is a fix to a bug in code that has already been merged into master.

@tute I assume from your later comment #1974 (comment) that you do plan on merging this after all?

The question of separate classes was brought up already in the discussion on the PR that introduced AWS v2 support: #1903 (comment)

@tute

tute commented Sep 9, 2015

Copy link
Copy Markdown
Contributor

Thanks! All done then. Where did you read about the upcoming deprecation of v1? I couldn't find docs related to that.

@betesh

betesh commented Sep 9, 2015

Copy link
Copy Markdown
Contributor Author

@tute, I meant that eventually paperclip will want to deprecate AWS v1 support. I was assuming (correct me if I'm wrong) that once paperclip publishes a gem that supports both, we'll want to deprecate AWS v1 support a few versions later. (Reading that comment again now, I was also confused by what I meant.)

@tute

tute commented Sep 10, 2015

Copy link
Copy Markdown
Contributor

@tute, I meant that eventually paperclip will want to deprecate AWS v1 support. I was assuming (correct me if I'm wrong) that once paperclip publishes a gem that supports both, we'll want to deprecate AWS v1 support a few versions later. (Reading that comment again now, I was also confused by what I meant.)

We have a specific issue to discuss wether AWS v1 should be deprecated: #1997.

@betesh

betesh commented Sep 10, 2015

Copy link
Copy Markdown
Contributor Author

Thank you for pointing me to that ticket.

However, it has nothing to do with whether this PR should be merged. There is a bug in master. It should be fixed before we publish a gem.

@betesh betesh changed the title defualt permission should assume aws v2 if both aws versions are in use default permission should assume aws v2 if both aws versions are in use Feb 5, 2016
@betesh

betesh commented Feb 5, 2016

Copy link
Copy Markdown
Contributor Author

@tute I still don't understand why this was closed. I have rebased it onto the latest master, where it is still an issue

@tute

tute commented Feb 23, 2016

Copy link
Copy Markdown
Contributor

I just came from vacations, will look into it soon.

@tute

tute commented Mar 12, 2016

Copy link
Copy Markdown
Contributor

Hi @betesh! I can't reopen the PR, because the branch seems to be gone from your repo. Want to pusblish it so we reopen?

But before, why is this change necessary? Thank you!

@betesh

betesh commented Mar 13, 2016

Copy link
Copy Markdown
Contributor Author

AWS v1: 'public_read'
AWS v2: 'public-read'

If you have both in you Gemfile, we use v2 but we use public_read, which is inconsistent and means that the default configuration is broken.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants