NotificationsYou must be signed in to change notification settings
Fork928
Star10.1k

feat: Add UI for awaiting agent connections#578

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

kylecarbs merged 14 commits intomainfromssh

Mar 29, 2022

Merged

feat: Add UI for awaiting agent connections#578

kylecarbs merged 14 commits intomainfromssh

Mar 29, 2022

Conversation

Copy link

Member

kylecarbs commentedMar 27, 2022

No description provided.

feat: Add stage to build logs

7ed283c

This adds a stage property to logs, and refactors the job logscliui.It also adds tests to the cliui for build logs!

kylecarbs self-assigned this

Mar 27, 2022

Copy link

codecovbot commentedMar 27, 2022•
edited
Loading

Codecov Report

Merging#578 (342b03d) intomain (620c889) willincrease coverage by0.54%.
The diff coverage is72.64%.

@@            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

Flag	Coverage Δ
unittest-go-	`63.40% <71.22%> (+0.44%)`	⬆️
unittest-go-macos-latest	`59.12% <51.41%> (+0.65%)`	⬆️
unittest-go-ubuntu-latest	`62.09% <72.64%> (+0.53%)`	⬆️
unittest-go-windows-2022	`58.32% <52.83%> (+0.55%)`	⬆️
unittest-js	`63.32% <ø> (ø)`

Impacted Files	Coverage Δ
cli/start.go	`66.15% <ø> (ø)`
coderd/coderd.go	`96.47% <ø> (ø)`
cli/cliui/select.go	`61.22% <59.57%> (+15.51%)`	⬆️
provisioner/terraform/provision.go	`68.82% <62.90%> (-3.47%)`	⬇️
cli/ssh.go	`43.33% <77.27%> (+38.20%)`	⬆️
coderd/provisionerdaemons.go	`59.52% <80.00%> (-2.89%)`	⬇️
cli/cliui/agent.go	`82.14% <82.14%> (ø)`
agent/conn.go	`68.96% <100.00%> (+3.58%)`	⬆️
cli/root.go	`76.84% <100.00%> (ø)`
cli/workspaceagent.go	`78.16% <100.00%> (ø)`
... and24 more

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last update620c889...342b03d. Read thecomment docs.

kylecarbs force-pushed thessh branch 3 times, most recently from0bda974 to8f0f57eCompare

March 27, 2022 01:48

feat: Add stage to build logs

048f29a

This adds a stage property to logs, and refactors the job logscliui.It also adds tests to the cliui for build logs!

kylecarbs force-pushed thenicerlogs branch from7ed283c to048f29aCompare

March 27, 2022 01:59

feat: Add config-ssh and tests for resiliency

2152758

kylecarbs force-pushed thessh branch from8f0f57e to2152758Compare

March 27, 2022 02:00

kylecarbsand others added5 commits

March 27, 2022 02:06

Merge branch 'nicerlogs' into ssh

e774dd7

Rename "Echo" test to "ImmediateExit"

172d4fe

Fix Terraform resource agent association

82f60db

Fix logs post-cancel

6bff973

Fix select on Windows

3654022

Copy link

misskniss commentedMar 28, 2022

@kylecarbs can this be closed or does it still need to be done?

Copy link

MemberAuthor

kylecarbs commentedMar 28, 2022

Needs to be done or else no SSH!

kylecarbs mentioned this pull request

Mar 28, 2022

coder projects init selector doesn't work#486

Closed

kylecarbs added3 commits

March 28, 2022 14:24

Remove terraform init logs

aef12e1

Move timer into it's own loop

15cae15

Fix race condition in provisioner jobs

fb9fbb6

Base automatically changed fromnicerlogs tomain

March 28, 2022 18:43

Merge branch 'main' into ssh

c91b3cc

kylecarbs changed the title~~feat: Add config-ssh and tests for resiliency~~feat: Add UI for awaiting agent connections

Mar 28, 2022

kylecarbs requested review fromcoadler andEmyrk

March 28, 2022 20:19

coadler approved these changes

Mar 28, 2022

View reviewed changes

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)

Copy link

Contributor

coadlerMar 28, 2022

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

Copy link

Contributor

coadlerMar 28, 2022

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 thanEOT

Copy link

MemberAuthor

kylecarbsMar 28, 2022

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

Emyrk approved these changes

Mar 28, 2022

View reviewed changes

Copy link

Member

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

cli/cliui/agent.go

		case <-timer.C:
		}
		message := "Don't panic, your workspace is booting up!"
		if resource.Agent.Status == codersdk.WorkspaceAgentDisconnected {

Copy link

Member

EmyrkMar 28, 2022

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

Copy link

MemberAuthor

kylecarbsMar 28, 2022

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.

cli/cliui/select.go

		"github.com/spf13/cobra"
		)

		func init() {

Copy link

Member

EmyrkMar 28, 2022

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

Copy link

MemberAuthor

kylecarbsMar 28, 2022

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

cli/ssh.go

		@@ -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)

Copy link

Member

EmyrkMar 28, 2022

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.

Copy link

MemberAuthor

kylecarbsMar 28, 2022

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.

cli/ssh.go

		}

		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"},

Copy link

Member

EmyrkMar 28, 2022

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?

Copy link

MemberAuthor

kylecarbsMar 28, 2022

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!

cli/cliui/agent.go

		WarnInterval time.Duration
		}

		func Agent(cmd *cobra.Command, opts AgentOptions) error {

Copy link

Member

EmyrkMar 28, 2022

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 excepterror, it's a bit unclear.

Copy link

MemberAuthor

kylecarbsMar 28, 2022

Choose a reason for hiding this comment

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

Yup, certainly should.

cmd/cliui/main.goShow resolvedHide resolved

coderd/provisionerdaemons.go

Comment on lines +403 to +405

		return &proto.UpdateJobResponse{
		Canceled: job.CanceledAt.Valid,
		}, nil

Copy link

Member

EmyrkMar 28, 2022

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

Copy link

MemberAuthor

kylecarbsMar 28, 2022

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!

provisioner/terraform/provision.go

		@@ -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)

Copy link

Member

EmyrkMar 28, 2022

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

Copy link

MemberAuthor

kylecarbsMar 28, 2022

Choose a reason for hiding this comment

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

hahahahah that it will

kylecarbs added2 commits

March 28, 2022 23:13

Merge branch 'main' into ssh

9ad326b

Fix requested changes

342b03d

kylecarbs force-pushed thessh branch froma81243c to342b03dCompare

March 29, 2022 00:06

kylecarbs merged commit82dfd6c intomain

Mar 29, 2022

kylecarbs deleted the ssh branch

March 29, 2022 00:19

Labels

None yet

4 participants

Movatterモバイル変換

feat: Add UI for awaiting agent connections#578

feat: Add UI for awaiting agent connections#578

Uh oh!

Conversation

kylecarbs commentedMar 27, 2022

Uh oh!

codecovbot commentedMar 27, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Codecov Report

Uh oh!

misskniss commentedMar 28, 2022

Uh oh!

kylecarbs commentedMar 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Emyrk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecovbot commentedMar 27, 2022•
edited
Loading