search

keystonejs / keystone-storage-adapter-s3

⚠️ Archived - Legacy S3 Storage Adapter for KeystoneJS
MIT License
17 stars 55 forks source link

Support environment variables? #1

Closed JedWatson closed 8 years ago

JedWatson commented 8 years ago

Keystone currently supports detecting S3 options in process.env - see https://github.com/keystonejs/keystone/blob/master/index.js#L75-L77

I think it would be worth continuing to support these, like this: https://github.com/keystonejs/keystone-email/blob/master/lib/transports/mailgun/getSendOptions.js#L7-L12

We may remove support in Keystone for the s3 config option though..? Would probably be awkward to support and breaks separation of concerns between the packages

josephg commented 8 years ago

Yeah I was wondering about that while writing it. The test suite uses environment variables.

I think generally it should be up to the application to configure the S3 options. The question is between writing:

var storage = new keystone.Storage({
    adapter: require('keystone-s3'),
    s3: {
        key: process.env.S3_KEY,
        secret: process.env.S3_SECRET,
        bucket: process.env.S3_BUCKET,
    },
})

... or leaving that out. Tbh I can't think of any strong reason to leave it out - if you want to manually configure those properties you can either not specify environment variables or just override them in your configuration.