ENH: S3 access using instance role #2621
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2621 +/- ##
==========================================
+ Coverage 67.6% 68.07% +0.47%
==========================================
Files 339 340 +1
Lines 42820 44700 +1880
Branches 5290 5796 +506
==========================================
+ Hits 28948 30430 +1482
- Misses 13189 13512 +323
- Partials 683 758 +75
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thanks for the PR @anibalsolon - it looks like we're not testing DataSink's s3 behavior currently in our CI 😰
Is that something you'd be interested in adding with this PR?
|
@mgxd isn't that because fakes3 is not installed? https://github.com/nipy/nipype/blob/master/nipype/interfaces/tests/test_io.py#L319-L321 However, I am not sure about the appropriate way to test the IAM Role + S3. FakeS3 does not implement it. |
nipype/interfaces/io.py
Outdated
| return bucket | ||
|
|
||
|
|
||
| def _get_head_bucket(self, s3_resource, bucket_name): |
There was a problem hiding this comment.
This should probably be a @staticmethod or a helper function outside the class, as it does not use self at all.
nipype/interfaces/io.py
Outdated
| dst_sp = dst.split('/') | ||
| dst_sp[0] = dst_sp[0].lower() | ||
| dst = '/'.join(dst_sp) | ||
| if dst[:len(s3_str)].lower().startswith(s3_str): |
There was a problem hiding this comment.
Since you're already taking the substring, why not:
if dst[:len(s3_str)].lower() == s3_str:
nipype/interfaces/io.py
Outdated
| @@ -123,6 +123,35 @@ def add_traits(base, names, trait_type=None): | |||
| return base | |||
|
|
|||
|
|
|||
| def get_head_bucket(s3_resource, bucket_name): | |||
There was a problem hiding this comment.
Minor quibble: I would put a _ prefix on the function still, just to mark it as a private function that external code shouldn't rely on.
|
Thanks. LGTM. @mgxd I'll let you merge if you have no more comments. |
Fixes #2620.
Changes proposed in this pull request