Skip to content

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

Merged
merged 10 commits into from
Apr 10, 2025

Conversation

aslilac
Copy link
Member

@aslilac aslilac commented Mar 28, 2025

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

@aslilac aslilac changed the base branch from main to lilac/files-cache April 1, 2025 21:55
@aslilac aslilac force-pushed the lilac/dynamic-parameters-endpoint branch from 3af6a57 to 18ff0b4 Compare April 1, 2025 21:58
Base automatically changed from lilac/files-cache to main April 2, 2025 22:42
@aslilac aslilac requested a review from Emyrk April 2, 2025 23:39
@aslilac aslilac marked this pull request as ready for review April 3, 2025 23:50
@aslilac aslilac force-pushed the lilac/dynamic-parameters-endpoint branch from 00de0bd to a54058e Compare April 7, 2025 22:32
Copy link
Member

@Emyrk Emyrk left a 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.

Comment on lines -91 to +94
"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),
Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is fine

@aslilac aslilac requested a review from Emyrk April 10, 2025 19:39
Comment on lines -91 to +94
"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),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is fine

@aslilac aslilac merged commit 859dd2f into main Apr 10, 2025
39 checks passed
@aslilac aslilac deleted the lilac/dynamic-parameters-endpoint branch April 10, 2025 20:08
@github-actions github-actions bot locked and limited conversation to collaborators Apr 10, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants