[DPE-7322] Support predefined roles #34
Conversation
sinclert-canonical
added
the
enhancement
sinclert-canonical
force-pushed
the
sinclert/7322/predefined-roles
branch
from
c0ce903 to
9a40854
Compare
sinclert-canonical
requested review from
paulomach,
taurus-forever and
carlcsaposs-canonical
sinclert-canonical
force-pushed
the
sinclert/7322/predefined-roles
branch
from
2e0bd19 to
7e4758b
Compare
| f"{app_name} app requested an invalid database name on " | ||
| f"{endpoint_name} endpoint: {exception_msg}" |
There was a problem hiding this comment.
| f"{app_name} app requested an invalid database name on " | |
| f"{endpoint_name} endpoint: {exception_msg}" | |
| f"{app_name} app requested invalid database name >20 characters on " | |
| f"{endpoint_name} endpoint" |
to keep message short (under 120 characters) so it doesn't get truncated in juju status & to give user clear actionable next step (request a shorter database name)
There was a problem hiding this comment.
I would rather have a distinction between the log message (long format), and the message displayed when doing juju status (short format). As it is proposed at the moment:
- Log: {app} requested an invalid database name on {endpoint} endpoint: DBA role longer than 32 characters
- Status: DBA role longer than 32 characters.
| f"{endpoint_name} endpoint: {exception_msg}" | ||
| ) | ||
| logger.warning(message) | ||
| super().__init__(ops.BlockedStatus(exception_msg)) |
There was a problem hiding this comment.
| super().__init__(ops.BlockedStatus(exception_msg)) | |
| super().__init__(ops.BlockedStatus(message)) |
There was a problem hiding this comment.
Answered in this comment.
| if self._databag.get("extra-user-roles"): | ||
| raise _UnsupportedExtraUserRole( | ||
| app_name=relation.app.name, endpoint_name=relation.name | ||
| app_name=self._app_name, endpoint_name=self._endpoint_name | ||
| ) |
There was a problem hiding this comment.
could you raise here if database name > 20 characters so that the _RelationThatRequestedUser object does not get instantiated?
the current pattern/design is _RelationThatRequestedUser is only successfully initialized if the request is valid
There was a problem hiding this comment.
I considered that approach too, to have the exception being raised in the __init__ method. I am on the fence whether that is a good separation of concerns or not, as it will require the _RelationThatRequestedUser class to know implementation details about the MySQLshell.create_application_database function.
Maybe exposing a new mysql_shell's MYSQL_DBA_ROLE_PREFIX constant with the charmed_dba_ value?
| logger.debug(f"Created {username=} with {attributes=}") | ||
| return password | ||
|
|
||
| def create_application_database(self, *, database: str, username: str) -> str: |
There was a problem hiding this comment.
| def create_application_database(self, *, database: str, username: str) -> str: | |
| def create_application_database_and_user(self, *, database: str, username: str) -> str: |
| logger.debug(f"MySQL roles found for {name_pattern=}: {len(rows)}") | ||
| return {row[0] for row in rows} | ||
|
|
||
| def _create_application_database(self, *, database: str, rolename: str) -> str: |
There was a problem hiding this comment.
| def _create_application_database(self, *, database: str, rolename: str) -> str: | |
| def _create_application_database(self, *, database: str, rolename: str): |
nit; return value unused
| if len(rolename) >= _ROLE_MAX_LENGTH: | ||
| raise ValueError("Database DBA role longer than 32 characters") | ||
|
|
||
| ________ = self._create_application_database(database=database, rolename=rolename) |
There was a problem hiding this comment.
| ________ = self._create_application_database(database=database, rolename=rolename) | |
| self._create_application_database(database=database, rolename=rolename) |
nit
| f"{app_name} app requested unsupported extra user role on {endpoint_name} endpoint" | ||
| f"{app_name} app requested unsupported extra user role on " | ||
| f"{endpoint_name} endpoint" |
There was a problem hiding this comment.
nit: why wrap? line length is 99 characters
There was a problem hiding this comment.
Just to make both exceptions have break lines in the same exact places. Easier to see where they differ (i.e. one includes the underlying exception message, while the other do not).
This PR supports the set of predefined roles proposed in this specification.
Contents have been ported from the MySQL Router VM and K8s approved PRs. However, clauses to create a DBA role for each created databases have been added (see code), which is something that got added late into the spec review process.