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.

Sign up for GitHub

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

Jump to bottom

feat(api_core): support version 3 policy bindings #9869

Merged
merged 10 commits into from
Jan 9, 2020
Merged

feat(api_core): support version 3 policy bindings #9869

merged 10 commits into from
Jan 9, 2020

Conversation

jkwlui
Copy link
Member

@jkwlui jkwlui commented Nov 21, 2019 •
edited
Loading

This proposal change uses a list of (binding) dicts as the underlying data structure, while providing backwards compatibility for v1 Policy.

The Policy class is still inheriting from MutableMapping, but throws an exception if policy's version is 3 and the user try to access it as if it were a dict (via accessors, setter, iterator etc).

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 21, 2019
@jkwlui jkwlui added the needs work This is a pull request that needs a little love. label Nov 22, 2019
@jkwlui jkwlui changed the title Draft: IAM conditions proposal 3 feat(api_core): support version 3 IAM policy bindings Dec 17, 2019
@jkwlui jkwlui marked this pull request as ready for review December 17, 2019 20:37
@jkwlui jkwlui mentioned this pull request Dec 17, 2019
Merged
@jkwlui jkwlui removed the needs work This is a pull request that needs a little love. label Dec 19, 2019
@jkwlui jkwlui changed the title feat(api_core): support version 3 IAM policy bindings feat(api_core): support version 3 policy bindings Dec 21, 2019
jkwlui
jkwlui commented Dec 30, 2019
View reviewed changes
api_core/google/api_core/iam.py Outdated

@property
def bindings(self):
""":obj:`list` of :obj:`dict`: The policy's bindings list.
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure how we'd document dict keys, I couldn't find any examples in the Google style guides.

Copy link
Contributor

@plamut plamut Jan 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Mypy uses the form Dict[key_type, value_type]. Not sure if this is an official guideline, but at least in BigQuery we follow this format (at least in the docstrings that follow the Google-style format). I also found quite a few occurrences of the same in firestore.

Hope this helps!

crwilcox
crwilcox approved these changes Jan 6, 2020
View reviewed changes
api_core/google/api_core/iam.py Outdated
members (:obj:`set` of str): Specifies the identities associated to this binding.
condition (dict of str:str): Specifies a condition under which this binding will apply.

:obj:`dict` Condition:
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be nested under binding?

@jkwlui jkwlui merged commit c25d741 into master Jan 9, 2020
@plamut
Copy link
Contributor

plamut commented Jan 10, 2020 •
edited
Loading

This PR broke storage unit tests in places where a test converts a policy to a dict (example).

Looking at the source, the error is raised if version > 1, therefore a simple fix for the storage tests is to set VERSION to 1 (currently: 17).

Question: What significance does VERSION carry, if any? If it's just some sort of a marker, the fixture can be easily adjusted without compromising the tests' semantics (the PR).

@plamut plamut mentioned this pull request Jan 10, 2020
Closed
This was referenced Jan 29, 2020
Merged
Merged
Merged