-
Notifications
You must be signed in to change notification settings - Fork 1
[DPE-7302] Prefixes helpers #27
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
Conversation
Codecov Report❌ Patch coverage is
❌ Your project status has failed because the head coverage (48.99%) is below the target coverage (70.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## 16/edge #27 +/- ##
===========================================
+ Coverage 46.72% 48.99% +2.27%
===========================================
Files 4 4
Lines 809 849 +40
Branches 94 98 +4
===========================================
+ Hits 378 416 +38
- Misses 414 415 +1
- Partials 17 18 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| user_definition += f" IN ROLE {', '.join(str_roles)}" | ||
| return user_definition, connect_statements | ||
|
|
||
| def _adjust_user_roles( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Split off from _adjust_user_definition() so that we can get just the roles.
| def add_user_to_databases( | ||
| self, user: str, databases: List[str], extra_user_roles: Optional[List[str]] = None | ||
| ) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we don't alter the user, just add roles and connect privs.
| db_roles, db_connect_stmt = self._adjust_user_roles(user, roles, database) | ||
| roles += db_roles | ||
| connect_stmt += db_connect_stmt | ||
| with self._connect_to_database() as connection, connection.cursor() as cursor: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unrelated and out of curiosity, why not add commit control on the context manager, as a parameter of connection.cursor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likely copy paste momentum. We should overhaul and move to psycopg3 in general.
paulomach
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. unrelated comment
Helpers for prefixed databases
Tested on canonical/postgresql-operator#1238
Checklist