singularityhub / sregistry-cli

Singularity Global Client for container management
https://singularityhub.github.io/sregistry-cli/
Mozilla Public License 2.0
14 stars 18 forks source link

sregistry.main.s3: improve bucket access detection #217

Closed tschoonj closed 5 years ago

tschoonj commented 5 years ago

Fix based on https://stackoverflow.com/questions/26871884/how-can-i-easily-determine-if-a-boto-3-s3-bucket-resource-exists (I should really start reading beyond the first answer! And the comments!)

So now bucket access will properly work whenever the credentials don't allow for listing all buckets.

Sorry, think I had my root IAM credentials active last time when testing this!

vsoch commented 5 years ago

No worries @tschoonj. Do you want to address the other issue you opened here? I don't think we should mkdir -p, but we should check that it exists and exit gracefully instead of spitting out that error. Either way, we'll again need edits to the changelog and a bump to the version :)

vsoch commented 5 years ago

If we know the database path (db_path) why not just check (separately) that the parent folder exists, and that the user has write? For example:

if not os.path.exists(os.path.dirname(db_path)):
    bot.exit("Database location %s does not exist." % db_path)

if not os.access('db_path, os.W_OK):
    bot.exit("Insufficient permission to write to %s" % db_path)
vsoch commented 5 years ago

The error triggered is linting:

sregistry/database/models.py:160:11: W0703: Catching too general exception Exception (broad-except)
tschoonj commented 5 years ago

Hi @vsoch

Is there anything else you need me to do in this PR?

Thanks!

vsoch commented 5 years ago

hey @tschoonj we are close! I just fixed the merge conflict in the CHANGELOG, but the version string will need an update. I could offer to do this for you, but I think there are a few other tweaks to make so if you push again, please update the version to 0.2.18. I'll look over / make my comments now.

tschoonj commented 5 years ago

Thanks!

vsoch commented 5 years ago

https://pypi.org/project/sregistry/0.2.18/

Woo! And using twine the readme is finally fixed. :)