feat(coderd): update endpoint to allow filtering provisioner daemons by tags #15448
Conversation
There was a problem hiding this comment.
Pretty cool, TIL CREATE DOMAIN, seems like it can be useful to us.
Left a few comments but I think the approach is good 👍🏻
Edit: The function approach for allowing query behavior to be shared is nice, but also we don't have a lot of previous uses of it, so looking forward to what others think about it.
| @@ -0,0 +1,17 @@ | |||
| CREATE DOMAIN tagset AS jsonb; | |||
|
|
|||
| COMMENT ON DOMAIN tagset IS 'A set of tags that match provisioner daemons to provisioner jobs, which can originate from workspaces or templates. tagset is a narrowed type over jsonb. It is expected to be the JSON representation of map[string]string. That is, {"key1": "value1", "key2": "value2"}. We need the narrowed type instead of just using jsonb so that we can give sqlc a type hint, otherwise it defaults to json.RawMessage. json.RawMessage is a suboptimal type to use in the context that we need tagset for.'; | |||
There was a problem hiding this comment.
❤️ thanks for adding an explanation.
I'm curious why you needed to add an override for this type instead of doing so on a field-level like with
...
- column: "provisioner_daemons.tags"
go_type:
type: "StringMap"
...for example?
There was a problem hiding this comment.
It's been a few days since I've tested this. Happy to retest. But from memory of when I was generating the code for this, the type inference for the column works just fine. It just doesn't transitively apply to the parameters passed into the select query.
I'll have a look into this once more just to be able to give you a more concrete answer at least.
There was a problem hiding this comment.
Absolutely non-blocking; this can be done after merge
|
|
||
| queryParams.Add("tags", string(tagsJSON)) | ||
| if len(queryParams) > 0 { | ||
| baseURL = fmt.Sprintf("%s?%s", baseURL, queryParams.Encode()) |
There was a problem hiding this comment.
It'd be less pithy but more durable if you used the url.URL type here to construct the URL.
This PR provides new parameters to an endpoint that will be necessary for #15048