Merge lp://staging/~hazmat/pyjuju/security-groups into lp://staging/pyjuju
Proposed by
Kapil Thangavelu
| Status: | Merged | ||||
|---|---|---|---|---|---|
| Approved by: | Kapil Thangavelu | ||||
| Approved revision: | 287 | ||||
| Merged at revision: | 296 | ||||
| Proposed branch: | lp://staging/~hazmat/pyjuju/security-groups | ||||
| Merge into: | lp://staging/pyjuju | ||||
| Prerequisite: | lp://staging/~hazmat/pyjuju/security-connection | ||||
| To merge this branch: | bzr merge lp://staging/~hazmat/pyjuju/security-groups | ||||
| Related bugs: |
|
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| William Reade (community) | Approve | ||
| Gustavo Niemeyer | Approve | ||
|
Review via email:
|
|||
Description of the change
Implements a security group as a stored/persistent principal with membership denoting acl access to utilize the group principal credentials on a connection.
Resubmitting because this is not showing up the kanban board.
To post a comment you must log in.
This looks good. Some minors, and +1.
(note it includes a pre-req branch still in review)
[1]
+ @inlineCallbacks
+ def initialize(self):
+ """Initialize the group.
Would be nice to have that right after __init__.
[2]
+ if self._name is None:
+ raise ValueError(
This should be a RuntimeError or similar.
[3]
+ raise StateNotFound(
s/group/Group/
[4]
+ raise ValueError(
+ "Principal: %r is already a member of group: %r" % (
(...)
+ if not found:
+ raise ValueError(
As I understand it, this logic is going to be used by code, and the final
state that we get into is the desired one, so it feels like both of these
could just be NOOPs. Even more considering there are race conditions into
play here.
[5]
+ acl.append(
+ make_ace(token, read=True))
This fits in a line.