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

Google Storage permissions support #860

Closed
wants to merge 1 commit into
base: trunk
from

Conversation

Projects
None yet
4 participants
@crunk1
Contributor

crunk1 commented Sep 21, 2016

  • Added ex_(get|set|delete)_permissions to libcloud.storage.drivers.google_storage.GoogleStorageDriver
  • Added a JSON connection to the GoogleStorageDriver.
  • The JSON connection is only used in the new methods I added. Added tests for changes.

Tests:
python setup.py test
python3.4 setup.py test

@crunk1

This comment has been minimized.

Show comment
Hide comment
@crunk1
Contributor

crunk1 commented Sep 21, 2016

@crunk1 crunk1 changed the title from Google Storage permissions and libcloud.common.types.OrderedEnum to Google Storage permissions and libcloud.common.types.IntEnum Sep 22, 2016

@rickard-von-essen

This comment has been minimized.

Show comment
Hide comment
@rickard-von-essen

rickard-von-essen Sep 22, 2016

Don't think I should have been pinged here?

rickard-von-essen commented Sep 22, 2016

Don't think I should have been pinged here?

@crunk1

This comment has been minimized.

Show comment
Hide comment
@crunk1

crunk1 Sep 22, 2016

Contributor

Ah, yes. Sorry, about that, Rickard. I was looking over a previous bug patch we had discussed.

Contributor

crunk1 commented Sep 22, 2016

Ah, yes. Sorry, about that, Rickard. I was looking over a previous bug patch we had discussed.

@crunk1

This comment has been minimized.

Show comment
Hide comment
@crunk1
Contributor

crunk1 commented Sep 22, 2016

@tonybaloney

This comment has been minimized.

Show comment
Hide comment
@tonybaloney

tonybaloney Sep 26, 2016

Contributor

@crunk1 can you explain the motivations behind the PR please?

Contributor

tonybaloney commented Sep 26, 2016

@crunk1 can you explain the motivations behind the PR please?

@crunk1

This comment has been minimized.

Show comment
Hide comment
@crunk1

crunk1 Sep 26, 2016

Contributor

Google Cloud Storage doesn't have a method exposed to query for, or set, permissions on containers or objects. This PR adds the functionality to do so. Internally at Google, I have built some libraries on top of libcloud and I need a way to get and set permissions on GCS entities.
IntEnum was a helper class that I made for my changes, but I purposefully made it more generalized and put it in common/types.py with the idea to share it.

Contributor

crunk1 commented Sep 26, 2016

Google Cloud Storage doesn't have a method exposed to query for, or set, permissions on containers or objects. This PR adds the functionality to do so. Internally at Google, I have built some libraries on top of libcloud and I need a way to get and set permissions on GCS entities.
IntEnum was a helper class that I made for my changes, but I purposefully made it more generalized and put it in common/types.py with the idea to share it.

@crunk1

This comment has been minimized.

Show comment
Hide comment
@crunk1
Contributor

crunk1 commented Sep 27, 2016

@erjohnso

This comment has been minimized.

Show comment
Hide comment
@erjohnso

erjohnso Sep 28, 2016

Member

The IntEnum addition looks like it could be generically useful across more of the code base. I'm pretty sure I've used dicts and sorted key lists for things like this. But I wonder if it would be better to decouple this and introduce IntEnum more broadly (across more of libcloud) to help us all move to this model? My concern with keeping it joined with the GCS changes, we'd all forget about it and the generic solution would not get adopted elsewhere at which point, this becomes a more complex solution than needed purely for the GCS changes.

wdyt?

/cc @supertom

Member

erjohnso commented Sep 28, 2016

The IntEnum addition looks like it could be generically useful across more of the code base. I'm pretty sure I've used dicts and sorted key lists for things like this. But I wonder if it would be better to decouple this and introduce IntEnum more broadly (across more of libcloud) to help us all move to this model? My concern with keeping it joined with the GCS changes, we'd all forget about it and the generic solution would not get adopted elsewhere at which point, this becomes a more complex solution than needed purely for the GCS changes.

wdyt?

/cc @supertom

@crunk1

This comment has been minimized.

Show comment
Hide comment
@crunk1

crunk1 Sep 29, 2016

Contributor

I think it is a little beyond the scope of this PR, but I agree with your opinion. Could I put it in a subsequent PR?

Contributor

crunk1 commented Sep 29, 2016

I think it is a little beyond the scope of this PR, but I agree with your opinion. Could I put it in a subsequent PR?

@erjohnso

This comment has been minimized.

Show comment
Hide comment
@erjohnso

erjohnso Sep 29, 2016

Member

Yes please! I think teasing that out separately and doing something more basic for GCS now would be easier.

Member

erjohnso commented Sep 29, 2016

Yes please! I think teasing that out separately and doing something more basic for GCS now would be easier.

@crunk1 crunk1 changed the title from Google Storage permissions and libcloud.common.types.IntEnum to Google Storage permissions support Sep 29, 2016

- Google Storage permissions support.Added ex_(get|set|delete)_permis…
…sions to

libcloud.storage.drivers.google_storage.GoogleStorageDriver
- Added a JSON connection to the GoogleStorageDriver.
- The JSON connection is only used in the new methods I added. Added tests for changes.

Tests:
python setup.py test
python3.4 setup.py test
@crunk1

This comment has been minimized.

Show comment
Hide comment
@crunk1

crunk1 Sep 29, 2016

Contributor

PTAL, IntEnum logic and other cleanup removed from this PR.

Contributor

crunk1 commented Sep 29, 2016

PTAL, IntEnum logic and other cleanup removed from this PR.

@asfgit asfgit closed this in f30b3e3 Sep 30, 2016

@erjohnso

This comment has been minimized.

Show comment
Hide comment
@erjohnso

erjohnso Sep 30, 2016

Member

Thanks @crunk1! 👯

Member

erjohnso commented Sep 30, 2016

Thanks @crunk1! 👯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment