-
Notifications
You must be signed in to change notification settings - Fork 90
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: DuplicatePeer error #1848
Conversation
✅ Deploy Preview for brilliant-pasca-3e80ec canceled.
|
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.
Left a suggestion but looks good. Tested on postman as well and see the 409 duplicate peer error.
packages/backend/src/peer/errors.ts
Outdated
@@ -24,6 +26,7 @@ export const errorToMessage: { | |||
[key in PeerError]: string | |||
} = { | |||
[PeerError.DuplicateIncomingToken]: 'duplicate incoming token', | |||
[PeerError.DuplicatePeer]: 'duplicate peer', |
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.
Would it be better to provide a more specific error message rather than simply "Duplicate peer"?
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.
Updated to be more specific
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.
Thanks for making this so digestible.
Changes proposed in this pull request
Context
While building out the auto-peering service, I noticed that it didn't really make sense to allow creating a Peer with the same combination of
staticIlpAddress
andassetId
(this would mean a duplicate peering relationship). This adds a unique constraint for these two columns in the DB.Related to #1761
Checklist
fixes #number