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

chore: incoming webhooks and tokens migration #5670

Merged
merged 3 commits into from
Dec 18, 2023

Conversation

nunogois
Copy link
Member

https://linear.app/unleash/issue/2-1699/db-create-migration-for-a-new-incoming-webhooks-table

Adds a migration that creates 2 tables:

  • incoming_webhooks
  • incoming_webhook_tokens

Copy link

vercel bot commented Dec 18, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
unleash-docs ⬜️ Ignored (Inspect) Visit Preview Dec 18, 2023 0:37am
unleash-monorepo-frontend ⬜️ Ignored (Inspect) Visit Preview Dec 18, 2023 0:37am

Copy link
Member

@chriswk chriswk left a comment

Choose a reason for hiding this comment

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

I'm not sure we want to cascade here.

Unless the thought is that incoming webhooks are strongly bound to the user that created them?

I see both pros and contras here.
Pros:

  • explicit control over what webhooks are created by whom
  • automatic cleanup when users are removed from the system

Cons:

  • automatic cleanup when users are removed. This is a double-edged sword, and the one that worries me the most

@nunogois
Copy link
Member Author

I'm not sure we want to cascade here.

Unless the thought is that incoming webhooks are strongly bound to the user that created them?

I see both pros and contras here. Pros:

  • explicit control over what webhooks are created by whom
  • automatic cleanup when users are removed from the system

Cons:

  • automatic cleanup when users are removed. This is a double-edged sword, and the one that worries me the most

I was under the assumption that we are always soft-deleting and not hard-deleting users, but I guess that may be a possibility after all. If that's the case I guess we may also want to drop the reference to the users table.

Addressed in 6c5899e

@chriswk
Copy link
Member

chriswk commented Dec 18, 2023

Ah, I forgot about the soft delete thing. May I also suggest an index on the created_by_user_id?

@chriswk
Copy link
Member

chriswk commented Dec 18, 2023

Never mind, I see they are in place already.

Copy link
Member

@chriswk chriswk left a comment

Choose a reason for hiding this comment

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

Nice :)

@chriswk
Copy link
Member

chriswk commented Dec 18, 2023

You don't need use strict; btw, so feel free to remove it :)

Copy link
Contributor

@gastonfournier gastonfournier left a comment

Choose a reason for hiding this comment

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

LGTM!

@nunogois nunogois merged commit 5f975bb into main Dec 18, 2023
7 checks passed
@nunogois nunogois deleted the chore-incoming-webhooks-and-tokens-migration branch December 18, 2023 13:22
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.

3 participants