-
Notifications
You must be signed in to change notification settings - Fork 582
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
feat: Add UI for awaiting agent connections #578
Conversation
This adds a stage property to logs, and refactors the job logs cliui. It also adds tests to the cliui for build logs!
Codecov Report
@@ Coverage Diff @@
## main #578 +/- ##
==========================================
+ Coverage 63.67% 64.22% +0.54%
==========================================
Files 197 198 +1
Lines 11543 11622 +79
Branches 85 85
==========================================
+ Hits 7350 7464 +114
+ Misses 3413 3362 -51
- Partials 780 796 +16
Continue to review full report at Codecov.
|
0bda974
to
8f0f57e
Compare
This adds a stage property to logs, and refactors the job logs cliui. It also adds tests to the cliui for build logs!
@kylecarbs can this be closed or does it still need to be done? |
Needs to be done or else no SSH! |
coderd/coderdtest/coderdtest.go
Outdated
@@ -258,6 +258,7 @@ func AwaitWorkspaceAgents(t *testing.T, client *codersdk.Client, build uuid.UUID | |||
if resource.Agent == nil { | |||
continue | |||
} | |||
// fmt.Printf("resources: %+v\n", resource.Agent) |
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.
Looks like a debug line
examples/gcp-windows/main.tf
Outdated
sensitive = true | ||
} | ||
variable "service_account" { | ||
description = <<EOT |
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.
IMO I mostly see these with <<EOF
rather than EOT
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 agree. Zero-clue why I did EOT
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.
Some nits and comments.
case <-timer.C: | ||
} | ||
message := "Don't panic, your workspace is booting up!" | ||
if resource.Agent.Status == codersdk.WorkspaceAgentDisconnected { |
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 will fail --race
because there is another for loop in the main thread touching this.
We should use atomic or just a mutex to protect that
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.
Good point. I didn't even have a test for this, and now I shall add one.
"github.com/spf13/cobra" | ||
) | ||
|
||
func init() { |
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 don't know this survey stuff. Not looking to understand this atm lol
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.
All of the libraries are questionable in their own rights tbh
@@ -24,6 +25,12 @@ func workspaceSSH() *cobra.Command { | |||
if err != nil { | |||
return err | |||
} | |||
if workspace.LatestBuild.Job.CompletedAt == nil { | |||
err = cliui.WorkspaceBuild(cmd, client, workspace.LatestBuild.ID, workspace.CreatedAt) |
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.
Should we log some message that their workspace was offline and is being restarted? I feel like this being a silent side effect could be confusing.
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.
Yeah, I need to revamp the SSH command.
} | ||
|
||
conn, err := client.DialWorkspaceAgent(cmd.Context(), resource.ID, nil, nil) | ||
conn, err := client.DialWorkspaceAgent(cmd.Context(), resource.ID, []webrtc.ICEServer{{ | ||
URLs: []string{"stun:stun.l.google.com:19302"}, |
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.
For customers with more restrictive firewalls, can they use TURN?
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 won't be hardcoded for long. Just a temporary hack!
WarnInterval time.Duration | ||
} | ||
|
||
func Agent(cmd *cobra.Command, opts AgentOptions) 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.
Can we comment what this function is doing? With lack of returns except error
, it's a bit unclear.
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.
Yup, certainly should.
return &proto.UpdateJobResponse{ | ||
Canceled: job.CanceledAt.Valid, | ||
}, nil |
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 is a big function. I see it's mostly boiler plate fields, but still
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.
It's ~140 lines, not that bad!
@@ -209,7 +208,7 @@ func (t *terraform) Provision(stream proto.DRPCProvisioner_ProvisionStream) erro | |||
case <-stream.Context().Done(): | |||
return | |||
case <-shutdown.Done(): | |||
_ = cmd.Process.Signal(os.Kill) | |||
_ = cmd.Process.Signal(os.Interrupt) |
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.
nice of you to be less aggressive 😄. I'm sure that PID will appreciate it
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.
hahahahah that it will
No description provided.