Skip to content

Make S3Overrides accessible via E-Var or option. #37

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

Closed
wants to merge 1 commit into from
Closed

Make S3Overrides accessible via E-Var or option. #37

wants to merge 1 commit into from

Conversation

mrmarcsmith
Copy link
Contributor

@mrmarcsmith mrmarcsmith commented Dec 30, 2016

Background: when using parse server with a config file there is no way to specify S3Override options.
-This change doesn't affect the current method of passing s3overrides = backwards compatible.
-s3overrides would now be accessible by via parse-server config file like this:

"filesAdapter": {
   "module":"parse-server-s3-adapter",
   "options":{
          "s3overrides": { 
                 "endpoint":{
                       "protocol":"https:",
                       "host":"$FILEURL",
                       "port":443,
                       "hostname":"",
                       "pathname":"/",
                       "path":"/",
                       "href":"https:/   //"
                        },
                 "s3BucketEndpoint": false,
                 ANY OTHER S3OVERRIDES GO HERE... 
           },
          "baseUrl": "https://",
          "directAccess": true,
          "accessKey": "asdf",
          "secretKey": "asdf",
          "bucket": "mybucket",
          "region": "us-east-1"
   }
}

-OR in theory (I haven't tested) by Environmental variable like so:

S3_OVERRIDES= { 
                 "endpoint":{
                       "protocol":"https:",
                       "host":"$FILEURL",
                       "port":443,
                       "hostname":"",
                       "pathname":"/",
                       "path":"/",
                       "href":"https:/   //"
                        },
                 "s3BucketEndpoint": false,
                 ANY OTHER S3OVERRIDES GO HERE... 
           }

Background: when using parse server with a config file there is no way to specify S3Override options.
-This change doesn't affect the current method of passing s3overrides = backwards compatible.
-s3overrides would now be accessible by via parse-server config file like this:
"filesAdapter": {
"module":"parse-server-s3-adapter",
"options":{
"s3overrides": { "endpoint": {"protocol":"https:","host":"$FILEURL","port":443,"hostname":"","pathname":"/","path":"/","href":"https:///"},
"s3BucketEndpoint": false,
ANY OTHER S3OVERRIDES GO HERE... },
"baseUrl": "https://",
"directAccess": true,
"accessKey": "",
"secretKey": "",
"bucket": "",
"region": ""
}
}

-OR in theory (I haven't tested) by Environmental variable like so:

S3_OVERRIDES="{ "endpoint": {"protocol":"https:","host":"$FILEURL","port":443,"hostname":"$FILEURL","pathname":"/","path":"/","href":"https://$FILEURL/"},
"s3BucketEndpoint": false,
ANY OTHER S3OVERRIDES GO HERE... }"
@codecov-io
Copy link

Current coverage is 69.53% (diff: 100%)

Merging #37 into master will decrease coverage by 25.00%

@@             master        #37   diff @@
==========================================
  Files             2          2          
  Lines           128        128          
  Methods           9          9          
  Messages          0          0          
  Branches         25         25          
==========================================
- Hits            121         89    -32   
- Misses            7         39    +32   
  Partials          0          0          

Powered by Codecov. Last update d496264...807f27c

@acinader
Copy link
Contributor

acinader commented Jan 5, 2017

@mrmarcsmith this looks good to me.

would you be willing to augment this test: https://github.com/parse-server-modules/parse-server-s3-adapter/blob/master/spec/test.spec.js#L126

to make sure it behaves as you expect?

@mrmarcsmith mrmarcsmith mentioned this pull request Jan 25, 2017
@mrmarcsmith
Copy link
Contributor Author

@acinader I added the test. any chance we could review this again? Thanks for your time!

@acinader
Copy link
Contributor

Hi @mrmarcsmith awesome. see my comment here: #39 (comment)

@acinader
Copy link
Contributor

acinader commented Jul 1, 2017

superseded by #40

@acinader acinader closed this Jul 1, 2017
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