-
Notifications
You must be signed in to change notification settings - Fork 278
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
S3 utility methods for reading and writing manifests, bundles, and logs in Jenkins #367
Conversation
…verting an op file to HTML file and then calling another function to put it inside an S3 bucket.
Change made for pulling and adding few changes
…t files to be stored in the path given by user.
…conventions given to classname
Signed-off-by: Himanshu Setia <[email protected]>
Signed-off-by: Himanshu Setia <[email protected]>
You can use
|
mypy requires stubbed version of the library to check for type. This should be present in Pipfile https://pypi.org/project/boto3-stubs/ to resolve errors related to boto3 |
Yeah, will require changes to Pipfile as well: I have made changes here: a8a58c2 to resolve the errors. |
Signed-off-by: Himanshu Setia <[email protected]>
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.
This is a lot cleaner and a much more sane interface, don't you think?
Some comments/improvements + tests and this is good to go.
bundle-workflow/src/s3_utility.py
Outdated
else os.environ.get("AWS_ROLE_SESSION_NAME") | ||
) | ||
assumed_role_cred = self.__assume_role() | ||
self.__s3_client = 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.
Nit: I would save __ assumed_role_cred
and create __s3_client
and __s3_resource
on demand the first time you need it.
bundle-workflow/src/s3_utility.py
Outdated
target = Path(local_dir) / Path(file_name) | ||
return s3bucket.__file_download_helper(bucket, key, str(target)) | ||
|
||
@staticmethod |
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.
You have this to wrap ClientError
into S3DownloadFailureException
. Since I can also call bucket.download_file
externally myself, I would get a ClientError
there. I think it's surprising to get different exceptions depending on whether I call a static helper or the implementation in it, so I think you should remove this code and do the except
inside download_file
.
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.
__file_download_helper
exists to wrap the logic common to both download_folder
and download_file
. Not sure how you want to replace this?
Also, the S3DownloadFailureException wraps theClientError
. The caller can see it in the stack trace. The reason for having the wrapper is to have a high level exception class catch all S3 download failures. We can revisit this later to add things like retryability, and make this more robust.
bundle-workflow/src/s3_utility.py
Outdated
""" | ||
s3bucket = cls(bucket_name, role_arn, role_session_name) | ||
try: | ||
s3bucket.__s3_client.upload_file(source, bucket_name, 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.
You shouldn't be reaching into a private method of s3bucket
. Add a s3bucket#upload_file
method.
bundle-workflow/src/s3_utility.py
Outdated
raise S3UploadFailureException(e) | ||
|
||
|
||
class S3DownloadFailureException(Exception): |
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 think the Python convention is to call these things S3DownloadFailureError
vs. Exception
, but they all derive from the Exception
class.
I would create a S3FileError
as a base class and store the key in it. Then S3UploadError
and S3DownloadError
would switch the error message.
bundle-workflow/src/s3_utility.py
Outdated
DurationSeconds=3600, | ||
)["Credentials"] | ||
except Exception as e: | ||
print("Assume role failed due to ", str(e.__repr__())) |
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.
Wrap in a specialized STSError
.
Signed-off-by: Himanshu Setia <[email protected]>
Signed-off-by: Himanshu Setia <[email protected]>
Signed-off-by: Himanshu Setia <[email protected]>
Signed-off-by: Himanshu Setia <[email protected]>
Signed-off-by: Himanshu Setia <[email protected]>
Signed-off-by: Himanshu Setia <[email protected]>
Signed-off-by: Himanshu Setia <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #367 +/- ##
==========================================
+ Coverage 56.89% 59.04% +2.14%
==========================================
Files 37 38 +1
Lines 1051 1111 +60
==========================================
+ Hits 598 656 +58
- Misses 453 455 +2
Continue to review full report at Codecov.
|
thanks @dblock for reviewing the PR. If this looks good, can we merge it? |
Signed-off-by: Himanshu Setia <[email protected]>
mock_sts = MagicMock() | ||
mock_s3_resource = MagicMock() | ||
mock_s3_client = MagicMock() | ||
bucket_name = "unitTestBucket" |
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.
Any reason to reuse these globals instead of just doing MagicMock()
in the side effect code?
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.
Yeah, somehow the mock_s3_resource
wasn't picking the same mock object with MagicMock inside side effect. Will revisit this later and fix.
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.
LGTM!
Description
This PR aims to add utility methods for reading/writing to s3. It provides below methods
These methods will serve as base for
This PR overrides #316 .
Issues Resolved
#260
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.