S3 region support #2153
S3 region support #2153
Conversation
|
I've verified this by creating a bucket in us-west-1. Prior to this change, it was not possible to upload into that bucket via Girder, and after the change, it worked. |
|
This is now ready for review. |
| @@ -62,10 +62,13 @@ | |||
| span.g-access-key-id #{assetstore.get('accessKeyId')} | |||
| div | |||
| b Secret access key: | |||
| span.g-secret-key #{assetstore.get('secret')} | |||
| span.g-secret-key #{assetstore.get('secret') && '[hidden]'} | |||
There was a problem hiding this comment.
This line is an unrelated change -- I just figured the secret key should be redacted from the list display page.
There was a problem hiding this comment.
If the user doesn't type in the region correctly (or at all and they're using a bucket not in us-east-1), creating the assetstore throws no errors and it's not until upload that the user gets an No 'Access-Control-Allow-Origin' header is present on the requested resource in their console.
Additionally, I'm wondering if we should change the region field to be an option field instead of a text field, using the return value of boto.s3.regions.
| input.input-sm.form-control#g-new-s3-service(type="text", placeholder="default: s3.amazonaws.com") | ||
| .form-group | ||
| label.control-label(for="g-new-s3-region") Region | ||
| input.input-sm.form-control#g-new-s3-region(type="text", placeholder="default: us-east-1") |
There was a problem hiding this comment.
adding "default: " to the start of each placeholder is inconsistent with the rest of the form (and most of Girder).
girder/api/v1/assetstore.py
Outdated
| @@ -92,11 +92,13 @@ def find(self, limit, offset, sort, params): | |||
| 'Do not include the bucket name here.', required=False, default='') | |||
| .param('readOnly', 'If this assetstore is read-only, set this to true.', | |||
| required=False, dataType='boolean', default=False) | |||
| .param('region', 'The AWS region to which the S3 bucket belongs, if not in "us-east-1".', | |||
There was a problem hiding this comment.
should default be used here?
| :returns: boto connection parameter dictionary. | ||
| """ | ||
| region = region or 'us-east-1' |
There was a problem hiding this comment.
depending on how often we start using this string, we might just want to make it a constant (DEFAULT_S3_REGION) or something.
|
I've addressed your inline feedback.
Unless I'm misunderstanding, this is a problem in general, and doesn't have to do with what region is selected. I'd defer addressing this issue to a future PR if so.
Good idea, but I'd also want to defer that to a future PR. |
|
To clarify my first point, I think addressing #2186 as a whole would also address any region-related configuration problems. |
zachmullen
force-pushed
the
s3-region-support
branch
from
6feecae
to
e966f7b
Compare
|
Oh, I don't care much about whether the word "default" appears or not, but I would definitely be in favor of a global change where the placeholders actually contain the default value rather than just repeating the field name. |
|
Oh I didn't even realize that was the convention. When I was grepping I was
just checking for whether or not default was there. +1 to the global change.
…On Thu, Jul 27, 2017 at 6:34 PM, Zach Mullen ***@***.***> wrote:
Merged #2153 <#2153>.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#2153 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAlH5RuvoyKSihzzmY_6iBE50JuZEaYSks5sSRCQgaJpZM4OII_v>
.
--
Dan LaManna
Kitware, Inc.
|
Depends on #2132 , only the last commit here is for this topic.