Skip to content
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

Handle reconnecting websocket cancel #5436

Merged
merged 1 commit into from
Aug 31, 2021
Merged

Conversation

rl-gitpod
Copy link
Contributor

Fixes #4858

Pass in context to allow cancel to be handled and not stay in reconnection attempt loop.

@roboquat roboquat requested review from aledbf and csweichel August 30, 2021 08:23
@rl-gitpod rl-gitpod requested review from akosyakov and removed request for aledbf and csweichel August 30, 2021 08:23
Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

/lgtm

/hold

@rl-gitpod Could you clean up commit to remove code which did not make it? then you can comment /unhold and the PR will be merged

@roboquat
Copy link
Contributor

LGTM label has been added.

Git tree hash: e0df5b99dc8ae92948ae756774a0841990a26255

@codecov
Copy link

codecov bot commented Aug 30, 2021

Codecov Report

Merging #5436 (47cefd2) into main (a35798c) will decrease coverage by 4.57%.
The diff coverage is n/a.

❗ Current head 47cefd2 differs from pull request most recent head e1c1b58. Consider uploading reports for the commit e1c1b58 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5436      +/-   ##
==========================================
- Coverage   36.18%   31.61%   -4.58%     
==========================================
  Files          18       35      +17     
  Lines        4563     5846    +1283     
==========================================
+ Hits         1651     1848     +197     
- Misses       2765     3849    +1084     
- Partials      147      149       +2     
Flag Coverage Δ
components-ee-agent-smith-app 25.95% <ø> (+0.13%) ⬆️
components-gitpod-cli-app 9.74% <ø> (?)
components-gitpod-protocol-go-lib ∅ <ø> (?)
components-local-app-app-darwin ∅ <ø> (∅)
components-local-app-app-linux ∅ <ø> (∅)
components-local-app-app-windows ∅ <ø> (∅)
components-supervisor-app 37.77% <ø> (?)
components-ws-manager-app ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
components/ws-manager/pkg/manager/create.go
components/ws-manager/pkg/manager/status.go
components/ws-manager/pkg/manager/imagespec.go
components/ws-manager/pkg/manager/manager.go
components/ws-manager/pkg/manager/probe.go
...omponents/ws-manager/pkg/manager/pod_controller.go
components/ws-manager/pkg/manager/manager_ee.go
components/ws-manager/pkg/manager/config.go
components/ws-manager/pkg/manager/monitor.go
components/ws-manager/pkg/manager/metrics.go
... and 34 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e039c1b...e1c1b58. Read the comment docs.

@rl-gitpod rl-gitpod force-pushed the rl-gitpod/lca-reconnect-4858 branch from 47cefd2 to e1c1b58 Compare August 30, 2021 09:03
@roboquat roboquat removed the lgtm label Aug 30, 2021
@rl-gitpod
Copy link
Contributor Author

rl-gitpod commented Aug 30, 2021

@akosyakov - wasn't 100% sure what you were requesting but I squashed the commits & comments, which removed the lgtm

/unhold

@akosyakov
Copy link
Member

/lgtm

@roboquat roboquat added the lgtm label Aug 30, 2021
@roboquat
Copy link
Contributor

LGTM label has been added.

Git tree hash: e0df5b99dc8ae92948ae756774a0841990a26255

@akosyakov
Copy link
Member

/approve

@akosyakov
Copy link
Member

/assign @csweichel

@akosyakov
Copy link
Member

akosyakov commented Aug 31, 2021

/werft run

👍 started the job as gitpod-build-rl-gitpod-lca-reconnect-4858.2

@csweichel
Copy link
Contributor

/approve

@roboquat
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akosyakov, csweichel

Associated issue: #4858

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@roboquat roboquat merged commit 5439643 into main Aug 31, 2021
@roboquat roboquat deleted the rl-gitpod/lca-reconnect-4858 branch August 31, 2021 06:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[local-app] trying to reconnect to stopped supervisors forever
4 participants