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

feat(backend): tenanted webhooks #3317

Draft
wants to merge 10 commits into
base: 2893/multi-tenancy-v1
Choose a base branch
from

Conversation

njlie
Copy link
Contributor

@njlie njlie commented Feb 21, 2025

Changes proposed in this pull request

  • Uses tenant webhook settings if available during webhook sending
  • Adds tenant id to webhook events when they are created

Context

Fixes #3121.

Checklist

  • Related issues linked using fixes #number
  • Tests added/updated
  • Make sure that all checks pass
  • Bruno collection updated (if necessary)
  • Documentation issue created with user-docs label (if necessary)
  • OpenAPI specs updated (if necessary)

@github-actions github-actions bot added type: tests Testing related pkg: backend Changes in the backend package. type: source Changes business logic labels Feb 21, 2025
@golobitch golobitch force-pushed the feature/tenant-settings-initial branch 4 times, most recently from 6ee3c75 to 12d1318 Compare February 24, 2025 21:38
Base automatically changed from feature/tenant-settings-initial to 2893/multi-tenancy-v1 February 24, 2025 22:48
@njlie njlie force-pushed the nl/3121/tenanted-webhooks branch from e6b9736 to 792d0b3 Compare February 24, 2025 23:20
@njlie njlie linked an issue Feb 26, 2025 that may be closed by this pull request
2 tasks
@@ -68,7 +68,8 @@ export class Peer
},
liquidityThreshold: this.liquidityThreshold,
balance
}
},
tenantId // TODO: update when peers are tenanted
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now this is using the operator tenant ID, but will be replaced with the peer's own tenant id once #3129 is merged in.

@njlie
Copy link
Contributor Author

njlie commented Feb 26, 2025

Large portions of this code are reviewable, but at the moment there are a handful of sections that have temporary code to address the fact that peers are not tenanted - as they may generate a PeerEvent when liquidity drops low enough.

Copy link
Contributor

@BlairCurrey BlairCurrey left a comment

Choose a reason for hiding this comment

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

looks good. I assume we are waiting on the peer stuff before merging, hence the draft status.

body,
{
timeout: settings?.webhookTimeout?.value
? Number(settings?.webhookTimeout?.value)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should handle the case where Number(settings?.webhookTimeout?.value) resolves to NaN (eg value is "a").

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: backend Changes in the backend package. type: source Changes business logic type: tests Testing related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Multi-Tenant] Tenanted Webhook Events
3 participants