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 s3overrides option format #24

Merged
merged 1 commit into from
Oct 3, 2016
Merged

Conversation

dsix-work
Copy link
Contributor

The goal of this change is to allow developers to specify any and all options permissible to the AWS.S3 constructor. This is useful when working with other s3-compatible services since we may wish to override a number of options.

This new options format should not break any existing configurations. The downside of configuring this way is that Object.assign() only works on the first level of the objects, so deeper items like params.Bucket could be masked.

Copy link
Contributor

@acinader acinader left a comment

Choose a reason for hiding this comment

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

You say:

The downside of configuring this way is that Object.assign() only works on the first level of the objects, so deeper items like params.Bucket could be masked.

Can you help me understand what you mean here? I tried to mock up a test of what you're referring too, but I'm a little to dense this morning to get it.

@@ -34,6 +34,8 @@ function S3Adapter() {
s3Options.secretAccessKey = options.secretKey;
}

Object.assign(s3Options,options.s3overrides);
Copy link
Contributor

Choose a reason for hiding this comment

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

add space after ,

@@ -48,7 +49,15 @@ const optionsFromArguments = function optionsFromArguments(args) {
options.globalCacheControl = otherOptions.globalCacheControl;
}
} else {
options = stringOrOptions || {};
if (args.length == 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@codecov-io
Copy link

codecov-io commented Oct 3, 2016

Current coverage is 69.84% (diff: 100%)

Merging #24 into master will decrease coverage by 25.03%

@@             master        #24   diff @@
==========================================
  Files             2          2          
  Lines           117        126     +9   
  Methods           9          9          
  Messages          0          0          
  Branches         23         25     +2   
==========================================
- Hits            111         88    -23   
- Misses            6         38    +32   
  Partials          0          0          

Powered by Codecov. Last update bb933cc...7fbe63f

@dsix-work
Copy link
Contributor Author

@acinader Here's an example of the lack of depth to Object.assign:

x = {a: 1, obj: {a: 1, b: 2}}
y = {a: 2, b: 3}
z = {a: 4, b: 6, obj: {a: 2}}
> Object.assign({}, x, y)
{ a: 2, obj: { a: 1, b: 2 }, b: 3 }
> Object.assign({}, x, z)
{ a: 4, obj: { a: 2 }, b: 6 }

In the second call, we loose the value obj.b since z.obj replaces x.obj entirely.

In the use here, it would mean that this would work:
s3 = new S3Adapter({bucket: "this-bucket"}, {sslEnabled: true})
But this would not work, since params.Bucket would be undefined:
s3 = new S3Adapter({bucket: "this-bucket"}, {params: {ACL: "public-read"}})
You would instead want to configure it like this:
s3 = new S3Adapter({}, {params: {ACL: "public-read", Bucket: "this-bucket"}})

@acinader
Copy link
Contributor

acinader commented Oct 3, 2016

@viawest-davidsix ok, i think i got it.

Since parse-server already uses lodash, we could solve the limitation you raise with acinader@6b6f258

but i'm not sure that's actually an improvement....:).

So if you agree, you can squash this and I'll merge it.

As an side, I haven't gotten around to implementing: #20

but if/when I do (unless you beat me to it ;), I now think that the supported constructors should be:

new S3Adapter(Bucket)
new S3Adapter(s3Options)
new S3Adapter(s3Options, adapterOptions)

Any thoughts welcome.

@dsix-work
Copy link
Contributor Author

I think the limitation is OK, there are few enough reasons to set params. I agree that implementing #20 would be another re-working of the options. I wonder if it might make sense to be more opinionated about configuration generally at that point?

@acinader acinader merged commit e564407 into parse-community:master Oct 3, 2016
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