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

fix: evtrigs ownership #1526

Closed
wants to merge 1 commit into from
Closed

fix: evtrigs ownership #1526

wants to merge 1 commit into from

Conversation

steve-chavez
Copy link
Member

Fixes #1437.

Now that the effects of the migrations/db/init-scripts are fully tested on the following files:

This reattempts to move migrations/db/init-scripts to migrations/db/migrations and have a single directory for migrations.

Of the above tests, the only change pertains to the event triggers owners which is the problem on #1437.

This requires modifying migrations, but the changes are more about following best practices:

  • Makes migrations idempotent.
  • alter default privileges explicitly list postgres as the target role.
  • Also:
    • adds a conditional for pgjwt which is not available on the 17 version
    • Realtime publication maintains the same postgres owner

Fixes #1437.

Now that the effects of the `migrations/db/init-scripts` are fully
tested on the following files:

- [nix/tests/sql/auth.out](https://github.com/supabase/postgres/blob/develop/nix/tests/expected/auth.out)
- [nix/tests/sql/storage.out](https://github.com/supabase/postgres/blob/develop/nix/tests/expected/storage.out)
- [nix/tests/sql/roles.out](https://github.com/supabase/postgres/blob/develop/nix/tests/expected/roles.out)
- [nix/tests/sql/evtrigs.out](https://github.com/supabase/postgres/blob/develop/nix/tests/expected/evtrigs.out)
- [nix/tests/sql/extensions_schema.out](https://github.com/supabase/postgres/blob/develop/nix/tests/expected/extensions_schema.out)
- [nix/tests/sql/realtime.out](https://github.com/supabase/postgres/blob/develop/nix/tests/expected/realtime.out)

This reattempts to move `migrations/db/init-scripts` to `migrations/db/migrations` and have a single directory for migrations.

Of the above tests, the only change pertains to the event triggers owners which is the problem on #1437.

This requires modifying migrations, but the changes are more about following best practices:

+ Makes migrations idempotent.
+ [alter default privileges](https://www.postgresql.org/docs/current/sql-alterdefaultprivileges.html) explicitly list postgres as the target role.
+ Also:
  - adds a conditional for pgjwt which is not available on the 17 version
  - Realtime publication maintains the same postgres owner
Comment on lines -15 to +22
issue_pg_net_access | postgres | extensions | grant_pg_net_access | postgres
issue_pg_net_access | supabase_admin | extensions | grant_pg_net_access | supabase_admin
issue_pg_graphql_access | supabase_admin | extensions | grant_pg_graphql_access | supabase_admin
issue_graphql_placeholder | supabase_admin | extensions | set_graphql_placeholder | supabase_admin
pgrst_ddl_watch | supabase_admin | extensions | pgrst_ddl_watch | supabase_admin
pgrst_drop_watch | supabase_admin | extensions | pgrst_drop_watch | supabase_admin
graphql_watch_ddl | supabase_admin | graphql | graphql.increment_schema_version | supabase_admin
graphql_watch_drop | supabase_admin | graphql | graphql.increment_schema_version | supabase_admin
issue_pg_cron_access | supabase_admin | extensions | grant_pg_cron_access | postgres
issue_pg_cron_access | supabase_admin | extensions | grant_pg_cron_access | supabase_admin
Copy link
Member Author

@steve-chavez steve-chavez Apr 5, 2025

Choose a reason for hiding this comment

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

This is the only change in tests. Which proves #1437 is solved, without affecting other aspects of the db.

@steve-chavez steve-chavez marked this pull request as ready for review April 5, 2025 02:53
@steve-chavez steve-chavez requested review from a team as code owners April 5, 2025 02:53
Copy link
Member Author

Choose a reason for hiding this comment

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

This is actually moved, but git/github doesn't recognize it as such because there are some modifications.

@soedirgo
Copy link
Member

soedirgo commented Apr 7, 2025

Has this been tested on local infra, minimally for project creation?

@steve-chavez
Copy link
Member Author

Has this been tested on local infra, minimally for project creation?

Is that part of a CI job? If that's required, then why it's not?

@steve-chavez
Copy link
Member Author

Let's prefer #1511 for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong ownership for some built-in event triggers
2 participants