Role mapping #496
Role mapping #496
Conversation
There was a problem hiding this comment.
Overall looks good to me @pierrotsmnrd. I will try and run the tests that are failing and see what it is.
| ) | ||
|
|
||
| op.add_column( | ||
| "namespace", sa.Column("role_mapping_metadata", sa.JSON(), nullable=True) |
There was a problem hiding this comment.
I'd prefer if this were just named metadata since I think there are reasons we'd want to update metadata other than for role_mapping_metadata e.g. what if we want to attach a description to a namespace.
There was a problem hiding this comment.
Naming a column metadata is a bit tricky, because it would imply having the name of column in the ORM named metadata as well, which is a reserved name :
sqlalchemy.exc.InvalidRequestError: Attribute name 'metadata' is reserved for the MetaData instance when using a declarative base class.
We might be able to have it named metadata in the DB and have a different name in the ORM but I'm not sure it's helpful at all.
There was a problem hiding this comment.
Ah didn't know that! Ideally I'd like to have a field in namespace, environment tables to store arbitrary json data. What do you think would be a good collumn name then? role_mapping_metadata to me this sounds restricting because it implies it is related to role_mapping. I'd like something consistent between the two tables.
There was a problem hiding this comment.
metadata_ works. meta_data as well. or anything like extra_data, external_data, custom_data
| namespace_id = Column(Integer, ForeignKey("namespace.id"), nullable=False) | ||
| namespace = relationship(Namespace, back_populates="roles_mappings") | ||
|
|
||
| # arn e.g. <namespace>/<name> like `quansight-*/*` or `quansight-devops/*` |
There was a problem hiding this comment.
Add note that must match schema.ARN_ALLOWED maybe there is also a way to enforce this regex.
There was a problem hiding this comment.
I have added a validation and added some tests for this :
d31ceaa
| @@ -186,7 +186,9 @@ def initialize(self, *args, **kwargs): | |||
| if self.conda_store.upgrade_db: | |||
| dbutil.upgrade(self.conda_store.database_url) | |||
|
|
|||
| self.authentication = self.authentication_class(parent=self, log=self.log) | |||
| self.authentication = self.authentication_class( | |||
| parent=self, log=self.log, authentication_db=self.conda_store.db | |||
| ) | ||
| raw_role_mappings = result.mappings().all() | ||
|
|
||
| db_role_mappings = {} |
There was a problem hiding this comment.
Use collection.defaultdict(set) maybe? Then no conditional needed and can just
for row in raw_role_mappings:
db_role_mappings[row["entity"]].add(row["role"])
return db_role_mappingsThere was a problem hiding this comment.
Haha yes, you're right. Some habits/patterns are hard to get rid of :) Done.
This PR aims at solving issue #491