Skip to content

Conversation

sinclert-canonical
Copy link

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

Comment on lines 128 to 148
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()

Copy link
Author

Choose a reason for hiding this comment

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

I think having a function with two lines of code is an oversimplification and provides little value. Furthermore, 1/2 usages are within an interface that will be pruned soon (deprecated_shared_db_provides.py module).

Copy link
Contributor

Choose a reason for hiding this comment

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

function with two lines of code is an oversimplification and provides little value

would you be willing to combine it back into one method then?
I'd like to keep the public interface of mysql_shell the same (1 method) since the methods should always be called together

Copy link

@taurus-forever taurus-forever left a comment

Choose a reason for hiding this comment

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

LGTM overall, one comment to avoid data format manipulations in Python.

Copy link

@paulomach paulomach left a comment

Choose a reason for hiding this comment

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

Great work

@sinclert-canonical sinclert-canonical force-pushed the sinclert/7322/predefined-roles branch from ec166ee to 597219c Compare September 1, 2025 09:29
@sinclert-canonical sinclert-canonical force-pushed the sinclert/7322/predefined-roles branch from 597219c to ed11918 Compare September 1, 2025 10:21
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.

4 participants