-
Notifications
You must be signed in to change notification settings - Fork 170
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
cosalib: create libs for s3 and ec2 operations #840
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
import boto3 | ||
from cosalib.cmdlib import ( | ||
retry_stop, | ||
retry_boto_exception, | ||
retry_callback | ||
) | ||
from tenacity import retry | ||
|
||
|
||
@retry(stop=retry_stop, retry=retry_boto_exception, before_sleep=retry_callback) | ||
def deregister_ami(ami_id, region): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Missing docstring There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But we're just wrapping the underlying API call...doesn't seem worth it to copy/paste it or have even more boilerplate docs like "Retry deregister_ami" right? Feels like we want to generate these. (On this topic of course, https://www.terraform.io/ is one of several projects implementing a "retry IaaS API calls" pattern) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree, these are just simple wrappers, I don't think its worth adding docstrings there There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This may be more of a personal preference, but having a simple doc with a function tends to be helpful. Keep in mind, this is a nit and isn't required to be updated for merge. I asked @vrutkovs if he's ready for this to merge via chat as I'm happy to hit the button 🙂. What I hope for is:
or even
but your point is well taken @cgwalters. |
||
ec2 = boto3.client('ec2', region_name=region) | ||
ec2.deregister_image(ImageId=ami_id) | ||
|
||
|
||
@retry(stop=retry_stop, retry=retry_boto_exception, before_sleep=retry_callback) | ||
def delete_snapshot(snap_id, region): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Missing docstring |
||
ec2 = boto3.client('ec2', region_name=region) | ||
ec2.delete_snapshot(SnapshotId=snap_id) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
import boto3 | ||
|
||
from botocore.exceptions import ClientError | ||
from cosalib.cmdlib import ( | ||
retry_stop, | ||
retry_boto_exception, | ||
retry_callback | ||
) | ||
from tenacity import retry | ||
|
||
|
||
S3 = boto3.client('s3') | ||
S3CONFIG = boto3.s3.transfer.TransferConfig( | ||
num_download_attempts=5 | ||
) | ||
|
||
|
||
def download_file(bucket, key, dest): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Missing docstring |
||
S3.download_file(bucket, key, dest, Config=S3CONFIG) | ||
|
||
|
||
@retry(stop=retry_stop, retry=retry_boto_exception, before_sleep=retry_callback) | ||
def head_bucket(bucket): | ||
S3.head_bucket(Bucket=bucket) | ||
|
||
|
||
@retry(stop=retry_stop, retry=retry_boto_exception, before_sleep=retry_callback) | ||
def head_object(bucket, key): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Missing docstring |
||
try: | ||
S3.head_object(Bucket=bucket, Key=key) | ||
except ClientError as e: | ||
if e.response['Error']['Code'] == '404': | ||
return False | ||
raise e | ||
return True | ||
|
||
|
||
@retry(stop=retry_stop, retry=retry_boto_exception, before_sleep=retry_callback) | ||
def list_objects(bucket, prefix, delimiter="/", result_key='Contents'): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Missing docstring |
||
kwargs = { | ||
'Bucket': bucket, | ||
'Delimiter': delimiter, | ||
'Prefix': prefix, | ||
} | ||
isTruncated = True | ||
while isTruncated: | ||
batch = S3.list_objects_v2(**kwargs) | ||
yield from batch.get(result_key) or [] | ||
kwargs['ContinuationToken'] = batch.get('NextContinuationToken') | ||
isTruncated = batch['IsTruncated'] | ||
|
||
|
||
@retry(stop=retry_stop, retry=retry_boto_exception, before_sleep=retry_callback) | ||
def delete_object(bucket, key): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Missing docstring |
||
sub_objects = list(list_objects(bucket, key)) | ||
if sub_objects != []: | ||
print("S3: deleting {sub_objects}") | ||
S3.delete_objects(Bucket=bucket, Delete=sub_objects) | ||
S3.delete_object(Bucket=bucket, Key=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.
If we're going to grow a lot of these, I think we could auto-generate them. But fine as is now.
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 don't think we'll have more than a few here. If that list grows large
ore
should be updated insteadThere 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.
OK, fair enough. And right...this is one of many "mantle vs cosa" issues.