-
Notifications
You must be signed in to change notification settings - Fork 874
feat: add dynamic parameters websocket endpoint #17165
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
Conversation
3af6a57
to
18ff0b4
Compare
00de0bd
to
a54058e
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.
This PR is really clean 👍
I left some notes. It would be ideal if we could extract most of the logic from the endpoint into it's own little pkg in coder
. Then we could create some nice tabled tests a lot easier imo.
"tailscale.com/types/opt.Bool": config.OverrideNullable(config.OverrideLiteral(bindings.KeywordBoolean)), | ||
"tailscale.com/types/opt.Bool": config.OverrideNullable(config.OverrideLiteral(bindings.KeywordBoolean)), | ||
"github.com/hashicorp/hcl/v2.Expression": config.OverrideLiteral(bindings.KeywordUnknown), |
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.
I was leaving a few comments about guts
, and honestly the syntax for overriding types is just... unfortunate right now.
I ran out of time when working on guts, and threw in type overrides in the current state just to make things possible, but it's not easy to use.
So I just made a PR that makes the changes I was proposing: #17339
One day I'll fixup guts and make this easier to use.
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.
is it alright if we merge your pr into main after merging this one?
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.
That is fine
"tailscale.com/types/opt.Bool": config.OverrideNullable(config.OverrideLiteral(bindings.KeywordBoolean)), | ||
"tailscale.com/types/opt.Bool": config.OverrideNullable(config.OverrideLiteral(bindings.KeywordBoolean)), | ||
"github.com/hashicorp/hcl/v2.Expression": config.OverrideLiteral(bindings.KeywordUnknown), |
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.
That is fine
Adds a new /dynamic-parameters websocket endpoint, some tests for it, and a small bit of generic machinery.
Don't be scared by the line count! This PR is only like 400 lines of actual code, the vast majority of the diff is from go.sum