-
Notifications
You must be signed in to change notification settings - Fork 90
feat(auth): add event for grant revoked #3362
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for brilliant-pasca-3e80ec canceled.
|
🚀 Performance Test ResultsTest Configuration:
Test Metrics:
📜 Logs
|
packages/auth/src/app.ts
Outdated
// if webhookUrl is not set, webhook events are not saved or processed | ||
if (this.config.webhookUrl) { | ||
for (let i = 0; i < this.config.webhookWorkers; i++) { | ||
process.nextTick(() => this.processWebhook()) | ||
} | ||
} |
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.
definitely an edge case, but nothing is stopping them from setting a value of 0 for the webhookWorkers
right? In which case it would just quietly not have any workers (even if url is set). Maybe we handle that somehow? I guess we could check that alongside webhookUrl
, or maybe error when parsing the config?
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.
that is correct, but If events are not processed they also should not be stored. I check on saving the webhookUrl as well.
I think a separate env variable (ENABLE_WEBHOOK
) that will state if the webhook is enabled or not will be best so the decision is more conscientious. All the checks to be depending on it after that.
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.
Some degree of config parsing might still be necessary, as the scenario where ENABLE_WEBHOOK
being true and WEBHOOK_URL
being undefined or WEBHOOK_WORKERS
being zero could still occur
9d0e53c
to
5f80a7a
Compare
5f80a7a
to
1368328
Compare
Changes proposed in this pull request
grant.revoked
event.trx
withknex
in some tests where trx was not defined before being used.Context
fixes #2898
Checklist
fixes #number
user-docs
label (if necessary)