-
Notifications
You must be signed in to change notification settings - Fork 881
feat(enterprise): add ready for handshake support to pgcoord #12935
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(enterprise): add ready for handshake support to pgcoord #12935
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
c40f0ab
to
2dc196c
Compare
ce96564
to
d7f72a8
Compare
coderd/database/queries/tailnet.sql
Outdated
@@ -207,6 +207,12 @@ FROM tailnet_tunnels | |||
INNER JOIN tailnet_peers ON tailnet_tunnels.src_id = tailnet_peers.id | |||
WHERE tailnet_tunnels.dst_id = $1; | |||
|
|||
-- name: PublishReadyForHandshake :exec | |||
SELECT pg_notify( |
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.
Why is this a query instead of pubsub.Publish()
?
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.
It made it easy to mock the call, and in the no-permission test make sure it wasn't being called
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.
not necessary anymore with your changes tho 👍
"github.com/coder/coder/v2/testutil" | ||
) | ||
|
||
func TestPGCoordinatorDual_ReadyForHandshake_OK(t *testing.T) { |
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.
nice
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.
LGTM!
I made some inline comments about a few leftovers that can get cleaned up, but I don't need to review again.
require.NoError(t, err) | ||
defer coord1.Close() | ||
|
||
agpltest.ReadyForHandshakeTest(ctx, t, coord1) |
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.
Very nice structure & code reuse
ba97700
to
1f0282d
Compare
1f0282d
to
b2fc817
Compare
No description provided.