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 support for ACL on objects #20

Merged
merged 1 commit into from
Nov 15, 2015
Merged

Conversation

flecno
Copy link
Contributor

@flecno flecno commented Oct 25, 2015

No description provided.


The ACL to apply to the objects.

*Default:* `public-read`
Copy link
Member

Choose a reason for hiding this comment

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

@flecno I can't see where you are setting the default value in the code. Can you please follow the conventions of the other default values in this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. It's not really explicit. The default already exists in

var acl = options.acl || 'public-read';

But I would prefer to move the default to another place too. I will look into the other defaults.

Copy link
Member

Choose a reason for hiding this comment

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

Cool. If you're going to make it a part of the plugin config then the
default should be moved out of there and into the index.js. Let me know
when you've updated it and I'll take a look.

Thanks for your contributions.
On Sun, 25 Oct 2015 at 20:46 Timo Zingel notifications@github.com wrote:

In README.md
#20 (comment)
:

@@ -85,6 +85,12 @@ The region the AWS bucket is located in.

Default: us-east-1

+### acl
+
+The ACL to apply to the objects.
+
+Default: public-read

You are right. It's not really explicit. The default already exists in

var acl = options.acl || 'public-read';

But I would prefer to move the default to another place too. I will look
into the other defaults.


Reply to this email directly or view it on GitHub
https://github.com/ember-cli-deploy/ember-cli-deploy-s3/pull/20/files#r42950148
.

@flecno
Copy link
Contributor Author

flecno commented Oct 26, 2015

@achambers here is an update. What do you say? :)

@flecno
Copy link
Contributor Author

flecno commented Nov 15, 2015

I rebased this to latest master. Can you review this please?

@ghedamat
Copy link
Contributor

@flecno LGTM, thanks!

ghedamat added a commit that referenced this pull request Nov 15, 2015
Add support for ACL on objects
@ghedamat ghedamat merged commit 99cec73 into ember-cli-deploy:master Nov 15, 2015
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.

3 participants