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

#825: Allow passing explicit connection to ACL.{reload,save} #853

Merged
merged 7 commits into from
May 6, 2015
Merged

#825: Allow passing explicit connection to ACL.{reload,save} #853

merged 7 commits into from
May 6, 2015

Conversation

tseaver
Copy link
Contributor

@tseaver tseaver commented May 1, 2015

See #825

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 0ed115b on tseaver:825-storage_acl-explicit_connection into * on GoogleCloudPlatform:master*.

"""Reload the ACL data from Cloud Storage.
@property
def connection(self):
"""Compute the connection for API requests for this ACL.

This comment was marked as spam.

@tseaver
Copy link
Contributor Author

tseaver commented May 4, 2015

@dhermes PTAL

"""
# Allow override for testing
path = getattr(self, '_reload_path', None)

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 324dea7 on tseaver:825-storage_acl-explicit_connection into 8b5c5b9 on GoogleCloudPlatform:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling d4bc450 on tseaver:825-storage_acl-explicit_connection into 8b5c5b9 on GoogleCloudPlatform:master.

@tseaver
Copy link
Contributor Author

tseaver commented May 6, 2015

@dhermes d4bc450 drops the "pure virtual" property implementation of reload_path and save_path, replacing them with a class attr default of None, allowing tests to assign to the ACL instances. Any remaining issues?

@dhermes
Copy link
Contributor

dhermes commented May 6, 2015

LGTM.

@tseaver I'd like to have a pow-wow about the current state of our ACL support and try to figure out what we want (or better yet, what someone wants that's actually used ACLs for an application). Things are a bit out of place in the current state (e.g. roles is a set, but storage has a concept of concentric roles permissions for the very purpose of avoiding an ACL having more than one role).

tseaver added a commit that referenced this pull request May 6, 2015
#825: Allow passing explicit connection to `ACL.{reload,save}`
@tseaver tseaver merged commit 90b4f72 into googleapis:master May 6, 2015
@tseaver tseaver deleted the 825-storage_acl-explicit_connection branch May 6, 2015 20:49
@tseaver
Copy link
Contributor Author

tseaver commented May 6, 2015

It is annyoing that those docs can't keep role vs. permisison straight, even on the same page.

If READER, WRITER, and OWNER are the only possible permissions / roles, then we could avoid having an ACE (a single assertion/entity, of which an ACL is a collection) have only one of them.

@tseaver
Copy link
Contributor Author

tseaver commented May 6, 2015

We should also work out how to support the predefined ACLs feature

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the Cloud Storage API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants