Skip to content

[DPE-7322] Support predefined roles #269

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sinclert-canonical
Copy link
Contributor

This PR supports the set of predefined roles proposed in this specification.

@sinclert-canonical sinclert-canonical added the enhancement New feature, UI change, or workload upgrade label Jul 24, 2025
@sinclert-canonical sinclert-canonical force-pushed the sinclert/7322/predefined-roles branch 4 times, most recently from 03dc3c6 to 3d1f6a2 Compare July 28, 2025 08:37
@sinclert-canonical sinclert-canonical force-pushed the sinclert/7322/predefined-roles branch from 3d1f6a2 to ec166ee Compare August 11, 2025 15:15
@sinclert-canonical sinclert-canonical marked this pull request as ready for review August 11, 2025 15:18
Copy link
Contributor

@carlcsaposs-canonical carlcsaposs-canonical left a comment

Choose a reason for hiding this comment

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

looks good except a couple nits

@@ -150,6 +170,23 @@ def add_attributes_to_mysql_router_user(
self._run_sql([f"ALTER USER `{username}` ATTRIBUTE '{attributes}'"])
logger.debug(f"Added {attributes=} to {username=}")

# TODO python3.10 min version: Use `set` instead of `typing.Set`
def get_mysql_roles(self, name_pattern: str) -> typing.Set[str]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def get_mysql_roles(self, name_pattern: str) -> typing.Set[str]:
def _get_mysql_roles(self, name_pattern: str) -> typing.Set[str]:

nit

Comment on lines -128 to +131
def create_application_database_and_user(self, *, username: str, database: str) -> str:
"""Create database and user for related database_provides application."""
def create_application_database(self, *, database: str) -> str:
Copy link
Contributor

@carlcsaposs-canonical carlcsaposs-canonical Aug 12, 2025

Choose a reason for hiding this comment

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

on the public interface (of mysql_shell), could we keep this as one method since it should always be called together?

e.g. if you want to have it split into two functions internally, suggest something like

def create_application_database_and_user():
    self._create_application_database()
    self._create_application_user()

Comment on lines +24 to +25
ROLE_DML = "charmed_dml"
ROLE_READ = "charmed_read"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ROLE_DML = "charmed_dml"
ROLE_READ = "charmed_read"
_ROLE_DML = "charmed_dml"
_ROLE_READ = "charmed_read"

nit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature, UI change, or workload upgrade Libraries: Out of sync
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants