Use awsProfile in s3region and s3credentials #57

tsuyoshizawa · 2017-10-14T19:53:47Z

Change to AwsRegionProviderChain uses awsProfile value (#56)

AwsProfileNameLoader.DEFAULT_PROFILE_NAME defines default, so the string should use constants value.

laughedelic · 2017-10-15T15:34:45Z

Thanks for the pull-request! It looks good, but I have one doubt which I'd like to discuss before merging:

from the user perspective, when you set explicitly awsProfile to something, wouldn't you expect that your credentials/region will be set from this profile?

If you have your region set in, say, environment variable, then you set awsProfile := "my-profile", it could be surprising, that s3region is still set from the env-var (because in the default chain it goes before profile region provider).

This is why in the current credentials provider chain, profile provider goes first.

I see two solutions here:

change default s3region provider chain similarly to the s3credentials: profile provider first, then others
change awsProfile setting to Option[String] with default None:
- when user sets it explicitly, use given profile for setting credentials and region (and don't try any other alternatives)
- when user leaves it unset, use default provider chains both for credentials and region

I would prefer the second path, because it uses default chains from the Java SDK and follows the path of least surprise for the user: when you set something optional explicitly, you expect it to take the priority over any other (implicit) defaults.

What do you think about it @tsuyoshizawa?

P.S. I'm marking this pull-request for Hacktoberfest in case you're participating 😉 It doesn't influence anything, just in case you'll make 3 more pull-requests with such label until the end of October, you'll get a nice t-shirt 👕

tsuyoshizawa · 2017-10-16T02:20:43Z

change awsProfile setting to Option[String] with default None:

I see. I also prefer the second path.
I will update this PR later.

I only concern this change bring breaking compatibility, but user just wrap Some.
But we should update README.md.

tsuyoshizawa · 2017-10-16T05:52:51Z

@laughedelic I fixed. Could you check again?

I will squash my commits after you review.

laughedelic

Thanks for updating the Readme. Check my comments below. I want to make the transition for those who already uses awsProfile smoother and make a breaking change in the default s3credentials for consistency.

laughedelic · 2017-10-16T11:38:35Z

src/main/scala/SBTS3Resolver.scala

@@ -85,7 +87,7 @@ object SbtS3Resolver extends AutoPlugin {
    implicit def fromProviderToAWSRegion(provider: AwsRegionProvider): Region = regionFromString(provider.getRegion())

    // Adding setting keys
-    lazy val awsProfile     = settingKey[String]("AWS credentials profile")
+    lazy val awsProfile     = settingKey[Option[String]]("AWS credentials profile")


Regarding your concern about breaking change: take a look at the trick we did in #55 (comment) (here). You can add an implicit conversion from String to Option[String] and a deprecation message, which explains that you should write explicit Some(...). This way users that didn't use the setting, won't notice the change and those who did will get a warning.

I added an implicit conversion from String to Option[String] according to acl2Option

laughedelic · 2017-10-16T11:42:06Z

src/main/scala/SBTS3Resolver.scala

    s3credentials :=
-      new ProfileCredentialsProvider(awsProfile.value) |
+      new ProfileCredentialsProvider(awsProfile.value.orNull) |
      new EnvironmentVariableCredentialsProvider() |
      InstanceProfileCredentialsProvider.getInstance(),


I'd like to change s3credentials default similarly to the new default for s3region. I think consistency here is more important than preserving compatibility:

the default will be mentioned in the readme

the change will be mentioned in the release notes

I think it won't affect most people, because usually you either rely on the default credentials chain, or set credentials explicitly. Also if you were using awsProfile before, then you also shouldn't notice the change.

So change this, please, in the same way as the s3region.

tsuyoshizawa · 2017-10-16T13:39:08Z

I changed s3credentials like s3region.

laughedelic

A couple of more changes, please

laughedelic · 2017-10-16T13:58:28Z

src/main/scala/SBTS3Resolver.scala

+      (awsProfile.value match {
+        case Some(profile) => new ProfileCredentialsProvider(profile)
+        case _ =>
+          new ProfileCredentialsProvider() |


In the case of None, it should use DefaultCredentialsProviderChain from SDK. The same way as with the region. Sorry if I wasn't clear about it.

I changed the code to use DefaultAWSCredentialsProviderChain.

laughedelic · 2017-10-16T13:58:28Z

src/main/scala/SBTS3Resolver.scala

@@ -21,6 +23,9 @@ object SbtS3Resolver extends AutoPlugin {
    @deprecated("s3acl is now an Option. Please define it either Some(...) or None if you wish to inherit the bucket default.", "0.18.0")
    implicit def acl2Option(acl: S3ACL): Option[S3ACL] = Some(acl)



The end of message (about bucket default) is not related to this setting. Just remove the part about None: "Please define it explicitly with Some(...).". None is default and doesn't use this conversion, so user doesn't need to see this warning if he didn't touch the setting.

laughedelic

Great! Thanks for the contribution. I'm going to merge it and cut a release a bit later today.

tsuyoshizawa · 2017-10-17T12:18:38Z

Thank you for review!

laughedelic · 2017-10-17T15:26:36Z

Released as v0.19.0

Fix to pass awsProfile to AwsRegionProviderChain

1f405e6

laughedelic assigned laughedelic and tsuyoshizawa and unassigned laughedelic Oct 15, 2017

laughedelic self-requested a review October 15, 2017 15:35

laughedelic added enhancement Hacktoberfest labels Oct 15, 2017

tsuyoshizawa added 4 commits October 16, 2017 14:19

Optional for awsProfile

9f5494d

update Readme.md

bdc1cc4

fix import

1464b29

use match for ignore compile error

37d5adb

laughedelic suggested changes Oct 16, 2017

View reviewed changes

support awsProfile to not Option

ab23fb5

updated Readme.md

4fc7f7c

laughedelic suggested changes Oct 16, 2017

View reviewed changes

fix deprecated message and s3credentials

e1e8e45

tsuyoshizawa force-pushed the custom-aws-region-provider-chain branch from 78a1dbe to e1e8e45 Compare October 17, 2017 00:49

laughedelic approved these changes Oct 17, 2017

View reviewed changes

laughedelic changed the title ~~Fix to pass awsProfile to AwsRegionProviderChain~~ Use awsProfile in s3region and s3credentials Oct 17, 2017

laughedelic merged commit 19e8ef4 into ohnosequences:master Oct 17, 2017

tsuyoshizawa deleted the custom-aws-region-provider-chain branch October 17, 2017 12:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use awsProfile in s3region and s3credentials #57

Use awsProfile in s3region and s3credentials #57

tsuyoshizawa commented Oct 14, 2017 •

edited by laughedelic

Loading

laughedelic commented Oct 15, 2017 •

edited

Loading

tsuyoshizawa commented Oct 16, 2017

tsuyoshizawa commented Oct 16, 2017

laughedelic left a comment

laughedelic Oct 16, 2017 •

edited

Loading

tsuyoshizawa Oct 16, 2017

laughedelic Oct 16, 2017

tsuyoshizawa commented Oct 16, 2017

laughedelic left a comment

laughedelic Oct 16, 2017

tsuyoshizawa Oct 17, 2017

laughedelic Oct 16, 2017

laughedelic left a comment

tsuyoshizawa commented Oct 17, 2017

laughedelic commented Oct 17, 2017

		@@ -21,6 +23,9 @@ object SbtS3Resolver extends AutoPlugin {
		@deprecated("s3acl is now an Option. Please define it either Some(...) or None if you wish to inherit the bucket default.", "0.18.0")
		implicit def acl2Option(acl: S3ACL): Option[S3ACL] = Some(acl)

Use awsProfile in s3region and s3credentials #57

Use awsProfile in s3region and s3credentials #57

Conversation

tsuyoshizawa commented Oct 14, 2017 • edited by laughedelic Loading

laughedelic commented Oct 15, 2017 • edited Loading

tsuyoshizawa commented Oct 16, 2017

tsuyoshizawa commented Oct 16, 2017

laughedelic left a comment

Choose a reason for hiding this comment

laughedelic Oct 16, 2017 • edited Loading

Choose a reason for hiding this comment

tsuyoshizawa Oct 16, 2017

Choose a reason for hiding this comment

laughedelic Oct 16, 2017

Choose a reason for hiding this comment

tsuyoshizawa commented Oct 16, 2017

laughedelic left a comment

Choose a reason for hiding this comment

laughedelic Oct 16, 2017

Choose a reason for hiding this comment

tsuyoshizawa Oct 17, 2017

Choose a reason for hiding this comment

laughedelic Oct 16, 2017

Choose a reason for hiding this comment

laughedelic left a comment

Choose a reason for hiding this comment

tsuyoshizawa commented Oct 17, 2017

laughedelic commented Oct 17, 2017

tsuyoshizawa commented Oct 14, 2017 •

edited by laughedelic

Loading

laughedelic commented Oct 15, 2017 •

edited

Loading

laughedelic Oct 16, 2017 •

edited

Loading