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

Updates for compatibility with EU Frankfurt AWS Region #49

Merged
merged 15 commits into from
Jun 14, 2017

Conversation

jgavin-zz
Copy link
Contributor

@jgavin-zz jgavin-zz commented Jun 13, 2017

This PR addresses this issue: #47 around not being able to publish artifacts in the EU Frankfurt region.

@laughedelic laughedelic self-requested a review June 14, 2017 14:43
Copy link
Contributor

@laughedelic laughedelic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @jgavin! Thanks for the contribution!

I've pointed out a couple of things in the review. I'm going to update the Java AWS SDK dependency and see what should be done to solve these issues.

val euCentral = Region.EU_Frankfurt.toString

//Correcting for fact that region enum resolves to null when provided US_STANDARD (us-east-1)
val correctedRegion = if(region == null) "us-east-1" else region
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is not a good workaround. A null may appear here for another reason and it should be silently replaced with this particular region value. Besides, US_Standard is not just a synonym for US_EAST_1. From the AWS Java SDK docs:

This is the default Amazon S3 Region. All requests sent to s3.amazonaws.com go to this region unless a location constraint is specified when creating a bucket. The US Standard Region automatically places data in either Amazon's east or west coast data centers depending on which one provides the lowest latency.

The region argument here is just a string, so you can pass either "us-east-1" directly or using the Regions enum or some other way. My point is that this is not an issue this plugin should solve.

region match {
case euCentral => s"""https://s3.${correctedRegion}.amazonaws.com/${bucketPath}"""
case _ => s"""https://s3-${correctedRegion}.amazonaws.com/${bucketPath}"""
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing it out, I didn't know that these patterns vary. Now looking through the docs I see that it's also different for China (having .cn suffix). So I want to find a way to get this prefix by the AWS SDK means.

Copy link
Contributor

@laughedelic laughedelic Jun 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a solution:

> import com.amazonaws.services.s3._, model._

> Region.EU_Frankfurt.toAWSRegion.getServiceEndpoint(AmazonS3.ENDPOINT_PREFIX)
s3.eu-central-1.amazonaws.com

> Region.EU_Ireland.toAWSRegion.getServiceEndpoint(AmazonS3.ENDPOINT_PREFIX)
s3-eu-west-1.amazonaws.com

> Region.CN_Beijing.toAWSRegion.getServiceEndpoint(AmazonS3.ENDPOINT_PREFIX)
s3.cn-north-1.amazonaws.com.cn

> Region.US_Standard.toAWSRegion.getServiceEndpoint(AmazonS3.ENDPOINT_PREFIX)
s3.amazonaws.com

It handles the different prefixes (./- and .cn) and also works for US_Standard (for some reason Region.US_Standard.toAWSRegion doesn't throw a NPE).
I guess, I'll also change the type of the parameter to com.amazonaws.regions.Region to avoid these problems.

@laughedelic
Copy link
Contributor

I'm going to merge it and change thing then myself before cutting a new release. Thanks for bringing this up @jgavin 👍

@laughedelic laughedelic merged commit 2ed29c3 into ohnosequences:master Jun 14, 2017
@laughedelic
Copy link
Contributor

@jgavin check out the new v0.16.0 release with the fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants