Set default table privileges in first migration #188
Conversation
| def upgrade(): | ||
| # Change default privileges for future tables created by the `migrator` | ||
| # user to automatically be accessible by the `app` user. | ||
| op.execute("ALTER DEFAULT PRIVILEGES GRANT ALL ON TABLES TO app") |
There was a problem hiding this comment.
Do we want to add more rules than just tables?
When I set this up for local development on my project, I did this for each schema, borrowing some of it from our PFML project:
REVOKE ALL ON SCHEMA PUBLIC FROM PUBLIC"GRANT USAGE ON SCHEMA {schema_name} TO {role};(I think this is automatic for the public schema)ALTER DEFAULT PRIVILEGES IN SCHEMA {schema_name} GRANT ALL ON TABLES TO {role};ALTER DEFAULT PRIVILEGES IN SCHEMA {schema_name} GRANT ALL ON SEQUENCES TO {role};ALTER DEFAULT PRIVILEGES IN SCHEMA {schema_name} GRANT ALL ON ROUTINES TO {role};
Sequences and routines are a bit less necessary, but threw them in
There was a problem hiding this comment.
Yeah those are all great. The idea I had was to do as much as possible in the infra template, so the first two items (1) REVOKE ALL ON SCHEMA public FROM public and (2) GRANT USAGE ON SCHEMA {schema_name} TO {role} are going to be done in template-infra. (2) is already done and (1) is captured by this ticket.
I'm fine adding SEQUENCES and ROUTINES to this PR.
There was a problem hiding this comment.
Hmm - just thinking of a developer in the future looking at this (or at the piece in the infra), I'd wonder why those two are split into two separate places. Could you add a comment here explaining that? And maybe add to that other ticket to update the comment to link to that bit of code so a developer can see the full picture?
There was a problem hiding this comment.
Good point, added a comment and updated the ticket.
There was a problem hiding this comment.
@chouinar no rush but lemme know if you're good with the latest changes
Ticket
Resolves #186
Changes
Context for reviewers
These migration changes don't follow the append-only practice of writing migrations, but since this is a template rather than an active project, I think that makes sense so that we can keep the project's initial setup clean.
Testing
Ran
make db-recreatethenmake db-upgradeandmake db-downgrade-allmanually locallyAlso this should be covered by CI since we have a test-migrations test