Skip to content
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

[DPE-3927] Collect readonly dbs #228

Merged
merged 7 commits into from
Jun 5, 2024
Merged

[DPE-3927] Collect readonly dbs #228

merged 7 commits into from
Jun 5, 2024

Conversation

dragomirp
Copy link
Contributor

@dragomirp dragomirp commented May 10, 2024

Collect read only dbs from the backend postgresql when wildcard dbs are enabled.

Split off of #210 for easier review.

actions.yaml Outdated
Comment on lines 14 to 15
update-readonly-dbs:
description: Update unrelated readonly databases in the backend Postgresql server.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Action so the used doesn't need to wait for update status.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actions must be approved by Mohamed and Mykola.
Maybe: https://github.com/PietroPasotti/resurrect ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got agreement in principle, but moving the action in a separate PR (#236) until everyone agrees on the specifics.

"host": r_hosts,
"dbname": name,
"port": r_port,
"auth_dbname": databases["*"]["auth_dbname"],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need an auth_dbname, since the new readonly dbs don't have the query injected.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it a time to discuss the idea to put auth_query into template1 and make it always available?

Copy link
Member

@marceloneppel marceloneppel Jun 4, 2024

Choose a reason for hiding this comment

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

I suspect that even with that, we may lack some permission to execute the query due to the following statement from the PostgreSQL documentation:

However, CREATE DATABASE does not copy database-level GRANT permissions attached to the source database. The new database has default database-level permissions.

@dragomirp dragomirp marked this pull request as ready for review May 10, 2024 13:41
Base automatically changed from dpe-3927-multi-databases to main June 3, 2024 11:25
@dragomirp dragomirp force-pushed the dpe-3927-readonly_dbs branch from a66c0e4 to 01d90a4 Compare June 3, 2024 11:39
Copy link

codecov bot commented Jun 4, 2024

Codecov Report

Attention: Patch coverage is 30.00000% with 21 lines in your changes missing coverage. Please review.

Project coverage is 68.45%. Comparing base (cb3dda9) to head (d520b7f).
Report is 7 commits behind head on main.

Files Patch % Lines
src/charm.py 30.00% 20 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #228      +/-   ##
==========================================
- Coverage   69.41%   68.45%   -0.97%     
==========================================
  Files           7        7              
  Lines        1102     1135      +33     
  Branches      189      201      +12     
==========================================
+ Hits          765      777      +12     
- Misses        259      279      +20     
- Partials       78       79       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@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.

"host": r_hosts,
"dbname": name,
"port": r_port,
"auth_dbname": databases["*"]["auth_dbname"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it a time to discuss the idea to put auth_query into template1 and make it always available?

@@ -2,6 +2,9 @@
{% for name, database in databases.items() -%}
{{ name }} = host={{ database.host }} {% if database.dbname %}dbname={{ database.dbname }}{% else %}auth_dbname={{ database.auth_dbname }}{% endif %} port={{ database.port }} auth_user={{ database.auth_user }}
{% endfor %}
{% for name, database in readonly_databases.items() -%}
{{ name }} = host={{ database.host }} dbname={{ database.dbname }} auth_dbname={{ database.auth_dbname }} port={{ database.port }} auth_user={{ database.auth_user }}
{% endfor %}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would be great to make sure PGB is OK having 100 lines in the config.

@dragomirp dragomirp merged commit d845730 into main Jun 5, 2024
57 of 59 checks passed
@dragomirp dragomirp deleted the dpe-3927-readonly_dbs branch June 5, 2024 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants