search

taoensso / faraday

Amazon DynamoDB client for Clojure
https://www.taoensso.com/faraday
Eclipse Public License 1.0
238 stars 84 forks source link

Use DefaultAWSCredentialsProviderChain when access-key and explicit credentials are not present #25

Closed joelittlejohn closed 10 years ago

joelittlejohn commented 10 years ago

Currently faraday will only default to using the DefaultAWSCredentialsProviderChain if no client options are specified (the map is empty). It's nice if faraday uses DefaultAWSCredentialsProviderChain whenever credentials can't be found, even when other options are specified.

This change means that users don't need to create their own DefaultAWSCredentialsProviderChain instance and pass it in using the :credentials key.

ptaoussanis commented 10 years ago

Hi Joe, this is a welcome change - thank you.

I'll merge your changes shortly (would like to clean up the db-client* fn a little while I'm at it).

Cheers! :-)

ptaoussanis commented 10 years ago

Okay, posted.

@joelittlejohn, @paraseba - could you please comment on whether the change here would be acceptable?:

https://github.com/ptaoussanis/faraday/commit/92363eafbcb849a57ebbbf1ef74474e87105102c https://github.com/ptaoussanis/faraday/commit/f79f68cc399c62b435100eb2e38626fe1577896e

As the db-client* fn has become more sophisticated, the "creds" arg name has become increasingly inappropriate. The fact that the "creds" arg was taking a ":credentials" option was one symptom of this.

Here I'm proposing to rename the arg to "client-opts" which seems more sensible. Lib consumers could call this "client" if they want something shorter. The change is purely aesthetic (it's non-breaking) - but I'm not using Faraday myself at the moment so just wanted to get your feedback on whether you feel this is sensible.

Thanks, cheers!

joelittlejohn commented 10 years ago

@ptaoussanis yes, definitely agree! I found exactly the same quirk (:credentials inside credentials) myself when using faraday and choosing names for the vars in my own app. client-opts is much more descriptive now.

paraseba commented 10 years ago

+1

ptaoussanis commented 10 years ago

Great, just pushed v1.2.0 with the changes - thanks both for the input. Cheers :-)

joelittlejohn commented 10 years ago

Oh just one thing I noticed in 1.2.0, I see that :creds is now suggested over :credentials. Now that the creds arg has become 'client-opts' I think :credentials is a perfectly good name for this key, the abbreviation doesn't add anything IMO and :credentials feels more natural (to me anyway).

ptaoussanis commented 10 years ago

Hey Joe,

I don't have a strong preference one way or the other, so just chose the shortest form that's unambiguous. The :credentials opt will continue to work until v2.0.0 at least (assuming there's ever need for a breaking version).

Cheers :-)