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

Don't accept Key and Secret as constructor arguments #20

Open
acinader opened this issue Aug 6, 2016 · 7 comments
Open

Don't accept Key and Secret as constructor arguments #20

acinader opened this issue Aug 6, 2016 · 7 comments
Assignees

Comments

@acinader
Copy link
Contributor

acinader commented Aug 6, 2016

Introduce a breaking change that would continue to allow a user to configure AWS credentials explicitly during adapter construction, but remove the discreet parameters from the constructor.

See #14 for links to AWS best practice docs and some discussion of this issue.

Currently standard configuration of AWS OR explicit credentials can be used. Accepting both makes both the documentation and argument processing complicated and error prone.

In addition, by not conforming to best practices, it promotes putting secret keys in places where they shouldn't go.

I'm proposing to:

  1. Remove the explicit constructor parameters for key and secret
  2. Remove the env vars S3_ACCESS_KEY & S3_SECRET_KEY as a configuration option for credentials (and make clear in the docs that AWS_ACCESS_KEY_ID & AWS_SECRET_ACCESS_KEY work)
  3. Rename the member of the config options from: accessKey & secretKey to accessKeyId & secretAccessKey to match the AWS SDK.
  4. Update documentation and tests to reflect change

I don't see anywhere where this change would require a doc change in parse-server, but a decision would have to made about when to either include the revision change in the parse-server package.json, or potentially removing all reference in code to this adapter and updating the documentation to show how to use it (which I think would be preferable as it would be a good model for how to use an adapter that is not explicitly referenced in parse-server.)

@flovilmart
Copy link
Contributor

@acinader would you wanna move forward with that? I'm ok with breaking changes in the adapter, as long as we provide a temporary solution, like depreciation warning, and after a certain date, this will plain not work anymore :)

@acinader
Copy link
Contributor Author

K. I've got some traveling coming up, I'll try to make this a flight project :). First step, just add deprecation notices. Check.

@acinader
Copy link
Contributor Author

bonus if i merge it with #39 and #40

@flovilmart
Copy link
Contributor

Triple the bonus if you push it before you land :p

@julianvogels
Copy link

Just stumbled upon this by getting the deprecation warning. Could you update the documentation with an example of what to do now? Instead of linking "the docs" and best practices this could help people to get the changes done quickly and move on with their lives.
Thanks for continuing to develop the adapter!

@acinader
Copy link
Contributor Author

acinader commented Jul 6, 2017

Hi @julianvogels. Can you take a look at #51 and tell me if that is clear?

@geekyfox90
Copy link

In my case I had to add AWS_DEFAULT_REGION to make it work. Please add this to docs

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 a pull request may close this issue.

4 participants