-
-
Notifications
You must be signed in to change notification settings - Fork 140
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
feat: enable webhooks by default #1152
base: develop
Are you sure you want to change the base?
Conversation
dcdfdfb
to
98e75d9
Compare
Please omit the retry changes for now, the Postgres team doesn't have the bandwidth to fix it if it goes south. @steve-chavez would it be an issue to enable Database Webhooks by default once we package it as an extension? |
I'm a bit confused about this change. So this means that the If so, is certainly an improvement, but ideally we package So could we move it to This would be a huge improvement. |
If the above can be done without a breaking change later, I can help with it after I finish supabase/pg_net#139. |
Yes, this removes the manual step from enabling on dashboard. Enabling webhooks is also currently irreversible since we don't support disabling from dashboard. That's why I think it's ok to have it as a migration since we have to deal with the upgrade path for existing projects anyway. But I agree a separate extension is better. I believe the This webhook also creates the |
+1 for moving into an extension
Having it as an extension will also make it easier to release updates later on (like the retry feature)
Sent from Outlook for Android<https://aka.ms/AAb9ysg>
…________________________________
From: Han Qiao ***@***.***>
Sent: Friday, August 23, 2024 2:52:07 AM
To: supabase/postgres ***@***.***>
Cc: Rodrigo Mansueli ***@***.***>; Mention ***@***.***>
Subject: Re: [supabase/postgres] feat: enable webhooks by default (PR #1152)
Yes, this removes the manual step from enabling on dashboard.
Enabling webhooks is also currently irreversible since we don't support disabling from dashboard. That's why I think it's ok to have it as a migration since we have to deal with the upgrade path for existing projects anyway.
But I agree a separate extension is better. I believe the supabase_functions schema is currently locked down to supabase_admin so it's safe to switch without breaking.
This webhook also creates the supabase_functions_admin role. Do we want to create it in the extension too?
—
Reply to this email directly, view it on GitHub<#1152 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABGNTEHZKARL7HJFTPEIEUTZS3EYPAVCNFSM6AAAAABM4SH3TGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMBWGM2DEMJWGI>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Ok cool. Then this should be good to go.
Haven't thought much about this, maybe the role creation and the privileges should remain as migration. I don't recall seeing a pg extension that creates roles (since they're global objects). But is something for later. |
pgsodium does this, but yeah agree that creating global objects in an extension is a bit awkward |
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.
Isn't pg_net still seeing segfaults? Do we really want to enable it by default?
No more segfaults since https://github.com/supabase/pg_net/releases/tag/v0.9.3, those were fixed on supabase/pg_net#136. |
SQL migrated over from cli / api https://github.com/supabase/cli/blob/develop/internal/db/start/templates/schema.sql#L26
With improved retry support from @mansueli: https://github.com/supabase/infrastructure/pull/16831Solves an immediate pain point with enabling webhooks. More context: https://supabase.slack.com/archives/C07E5GFAHTM/p1724229124279079
Upgrade path for existing projects can be done via ansible rollout as the migration is idempotent.