feat: Add connection_timeout and troubleshooting_url to agent #4937
Conversation
a1e1238 to
f02dfca
Compare
| @@ -31,8 +31,13 @@ import ( | |||
| "github.com/coder/coder/testutil" | |||
| ) | |||
|
|
|||
| func setupWorkspaceForAgent(t *testing.T) (*codersdk.Client, codersdk.Workspace, string) { | |||
| func setupWorkspaceForAgent(t *testing.T, mutate func([]*proto.Agent) []*proto.Agent) (*codersdk.Client, codersdk.Workspace, string) { | |||
There was a problem hiding this comment.
Maybe mutate ...func? You would get rid of the nil in setupWorkspaceForAgent(t, nil).
There was a problem hiding this comment.
Clean suggestion, but I think we can keep this explicit for now, makes it slightly more convenient to add new arguments, if needed and I don't see a use-case for multiple mutation functions.
There was a problem hiding this comment.
In general, the nil is considered to be an anti-pattern as it has an ambiguous meaning. This is the main reason why I raised this comment, so feel free to leave it as is.
coderd/workspaceagents_test.go
Outdated
| Auth: &proto.Agent_Token{ | ||
| Token: authToken, | ||
| }, | ||
| ConnectionTimeout: 1, |
There was a problem hiding this comment.
nit: question about the naming conversion, it seems that ConnectionTimeout is reserved only for seconds, should we name this ConnectionTimeoutSec? I hope that we won't need a 500ms timeout in the future :)
There was a problem hiding this comment.
Not a bad suggestion at all. I'm a bit vary of doing it though since this value is propagated through terraform-provider-coder, I was following the previous convention for how an int/second is represented in the code (e.g. codersdk.Healthcheck.Interval).
We do have other places in the code that utilizes *Millis/_ms, however. So there is precedent for both.
There was a problem hiding this comment.
So there is precedent for both
Good for me, thanks!
There was a problem hiding this comment.
It might be worth opening an issue to standardize these fields @mafredri... it makes me significantly more nervous about working with code with unspecified durations. The change of course doesn't need to happen in this PR though!
There was a problem hiding this comment.
I'll create the issue for standardization tomorrow, I did however push the change for this PR in a182cdf already, turned out much better. Thanks for pushing for this change both of you. 👍🏻
| @@ -53,7 +54,9 @@ type WorkspaceAgent struct { | |||
| Version string `json:"version"` | |||
| Apps []WorkspaceApp `json:"apps"` | |||
| // DERPLatency is mapped by region name (e.g. "New York City", "Seattle"). | |||
| DERPLatency map[string]DERPRegion `json:"latency,omitempty"` | |||
| DERPLatency map[string]DERPRegion `json:"latency,omitempty"` | |||
| ConnectionTimeoutSeconds int32 `json:"connection_timeout_seconds"` | |||
There was a problem hiding this comment.
@BrunoQuaresma Is this a preferable format, or does a duration string work as well for the frontend (e.g. 2m0s).
NOTE: This is not a field that the frontend needs to care about unless it's useful, the status changes automatically.
There was a problem hiding this comment.
An integer works better than a duration string.
This commit adds the connection timeout and troubleshooting url fields to coder agents. If an initial connection cannot be established within connection timeout seconds, then the agent status will be marked as `"timeout"`. The troubleshooting URL will be present, if configured in the Terraform template, it can be presented to the user when the agent state is either `"timeout"` or `"disconnected"`. Fixes #4678
f2620e5 to
7a7315a
Compare
This commit adds the connection timeout and troubleshooting url fields
to coder agents.
If an initial connection cannot be established within connection timeout
seconds, then the agent status will be marked as
"timeout".The troubleshooting URL will be present, if configured in the Terraform
template, it can be presented to the user when the agent state is either
"timeout"or"disconnected".Fixes #4678