-
Notifications
You must be signed in to change notification settings - Fork 0
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
[HP-1539] Add AWS functionality to cirrus #96
Conversation
The style in this PR agrees with This formatting comment was generated automatically by a script in uc-cdis/wool. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good so far. See comments for minor things. Make sure you update the version in the pyproject.toml too - and then send me the Fence PR using this branch and we'll wait to merge this until we can verify everything is working in updated Fence
gen3cirrus/aws/services.py
Outdated
import backoff | ||
import boto3 | ||
from botocore.exceptions import ClientError | ||
from gen3cirrus.config import config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer if we break up the imports by newlines for 3 sections:
- built in libs
- external deps
- internal imports
gen3cirrus/aws/services.py
Outdated
Generic Amazon servicing using Boto3 | ||
""" | ||
|
||
def __init__(self, client): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe just so it's super clear we should call this boto3_client
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bump
gen3cirrus/aws/services.py
Outdated
Generic Amazon servicing using Boto3 | ||
""" | ||
|
||
def __init__(self, client): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bump
Test that we can get a presigned url from a bucket | ||
""" | ||
|
||
s3 = boto3.client("s3", aws_access_key_id="", aws_secret_access_key="") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not seeing where this is mocked, so I'm actually not sure how this is passing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it need mocks 🤔 the generate_presigned_url()
in boto3 is a local operation
assert url != None | ||
|
||
|
||
def test_aws_get_presigned_url_requester_pays(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need more tests for the negative cases where boto throws an error
@Avantol13 what do you think about https://github.com/uc-cdis/fence/pull/1173/files#r1727515677 ? |
New Features