-
Notifications
You must be signed in to change notification settings - Fork 31
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
v2: added WebhooksResource #91
Conversation
@@ -0,0 +1,152 @@ | |||
package tailscale |
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.
This file copied almost verbatim from V1. I'll point out differences in PR.
} | ||
) | ||
|
||
type WebhooksResource struct { |
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.
New - organizes functions into a Resource.
v2/webhooks.go
Outdated
// Create creates a new webhook with the specifications provided in the CreateWebhookRequest. | ||
// Returns a Webhook if successful. | ||
func (c *WebhooksResource) Create(ctx context.Context, request CreateWebhookRequest) (*Webhook, error) { | ||
req, err := c.buildRequest(ctx, http.MethodPost, c.buildTailnetURL("webhooks"), requestBody(request)) |
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.
URLs are built using the new buildTailnetURL
and buildURL
functions.
v2/webhooks.go
Outdated
|
||
// Create creates a new webhook with the specifications provided in the CreateWebhookRequest. | ||
// Returns a Webhook if successful. | ||
func (c *WebhooksResource) Create(ctx context.Context, request CreateWebhookRequest) (*Webhook, error) { |
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.
Webhook
has been removed from function names since it's already named in the Resource.
@@ -0,0 +1,180 @@ | |||
package tailscale_test |
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.
Tests are almost verbatim the same as V1.
} | ||
server.ResponseBody = expectedWebhook | ||
|
||
webhook, err := client.Webhooks().Create(context.Background(), req) |
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.
Tests were updated to reflect new Resource and corresponding method names.
356bbf0
to
074c98b
Compare
Updates tailscale/corp#21867 Co-authored-by: Mario Minardi <[email protected]> Signed-off-by: Percy Wegmann <[email protected]>
074c98b
to
bbf5314
Compare
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! Left some notes for future us but nothing blocking
// url escapes each path element, so the caller doesn't need to worry about | ||
// that. | ||
// buildURL builds a url to /api/v2/... using the given pathElements. | ||
// It url escapes each path element, so the caller doesn't need to worry about that. | ||
func (c *Client) buildURL(pathElements ...any) *url.URL { |
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.
Note for ourselves that can be addressed outside of this PR: might be good to have some basic regression unit tests around this method (namely for our expectations around path escaping)
func (c *Client) buildURL(pathElements ...any) *url.URL { | ||
elem := make([]string, 1, len(pathElements)+1) | ||
elem[0] = "/api/v2" | ||
for _, pathElement := range pathElements { | ||
elem = append(elem, fmt.Sprint(pathElement)) | ||
elem = append(elem, url.PathEscape(fmt.Sprint(pathElement))) |
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.
Another note for future us: will likely want to use url.QueryEscape for any query params we allow interacting with on endpoints that accept them
Updates tailscale/corp#21867