Skip to content
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

Merged
merged 1 commit into from
Oct 11, 2019

Conversation

vrutkovs
Copy link
Member

@vrutkovs vrutkovs commented Oct 11, 2019

These libraries contain code to work on s3 buckets (head, list, delete)
and ec2 (deregister images and snapshots)

Cherrypicked from #839, tested in the dev pipeline



@retry(stop=retry_stop, retry=retry_boto_exception, before_sleep=retry_callback)
def deregister_ami(ami_id, region):
Copy link
Member

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.

Copy link
Member Author

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 instead

Copy link
Member

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.

Copy link
Member

@ashcrow ashcrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nits, but functionality looks good 👍



@retry(stop=retry_stop, retry=retry_boto_exception, before_sleep=retry_callback)
def deregister_ami(ami_id, region):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Missing docstring

Copy link
Member

Choose a reason for hiding this comment

The 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)

Copy link
Member Author

Choose a reason for hiding this comment

The 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

Copy link
Member

Choose a reason for hiding this comment

The 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:

"""
Removes an AMI from a specific region.
"""

or even

"""
.. seealso:: :py:func:`ec2.deregister_image`
"""

but your point is well taken @cgwalters.



@retry(stop=retry_stop, retry=retry_boto_exception, before_sleep=retry_callback)
def delete_snapshot(snap_id, region):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Missing docstring

)


def download_file(bucket, key, dest):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Missing docstring



@retry(stop=retry_stop, retry=retry_boto_exception, before_sleep=retry_callback)
def head_object(bucket, key):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Missing docstring



@retry(stop=retry_stop, retry=retry_boto_exception, before_sleep=retry_callback)
def list_objects(bucket, prefix, delimiter="/", result_key='Contents'):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Missing docstring



@retry(stop=retry_stop, retry=retry_boto_exception, before_sleep=retry_callback)
def delete_object(bucket, key):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Missing docstring

These libraries contain code to work on s3 buckets (head, list, delete)
and ec2 (deregister images and snapshots)
@ashcrow
Copy link
Member

ashcrow commented Oct 11, 2019

@vrutkovs noted this is ready for merge.

@ashcrow
Copy link
Member

ashcrow commented Oct 11, 2019

Will merge post CI run

@ashcrow ashcrow merged commit 5a960b0 into coreos:master Oct 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants