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

fix: fix ws not support throw option #2616

Merged
merged 12 commits into from Jul 28, 2022
Merged

Conversation

fatelei
Copy link
Contributor

@fatelei fatelei commented Jul 25, 2022

What is the pr for ?

Fix issue #2247

Test

With throw = false

image

With throw = true

image

@CLAassistant
Copy link

CLAassistant commented Jul 25, 2022

CLA assistant check
All committers have signed the CLA.

@codebien
Copy link
Collaborator

codebien commented Jul 25, 2022

Hi @fatelei, thanks for your contributon 🙏

Can you add some test coverage, please?

In any case, we plan to merge soon a new WebSocket API, this would be a breaking change. @mstoykov Does it make sense at this point considering the upcoming API?

@fatelei
Copy link
Contributor Author

fatelei commented Jul 25, 2022

Hi @fatelei, thanks for your contributon 🙏

Can you add some test coverage, please?

In any case, we plan to merge soon a new WebSocket API, this would be a breaking change. @mstoykov Does it make sense at this point considering the upcoming API?

unit test has been added

@codecov-commenter
Copy link

codecov-commenter commented Jul 25, 2022

Codecov Report

Merging #2616 (2ed46d6) into master (7f823f0) will decrease coverage by 0.17%.
The diff coverage is 70.16%.

❗ Current head 2ed46d6 differs from pull request most recent head 8535b00. Consider uploading reports for the commit 8535b00 to get more accurate results

@@            Coverage Diff             @@
##           master    #2616      +/-   ##
==========================================
- Coverage   75.59%   75.42%   -0.18%     
==========================================
  Files         202      205       +3     
  Lines       15992    16292     +300     
==========================================
+ Hits        12089    12288     +199     
- Misses       3151     3238      +87     
- Partials      752      766      +14     
Flag Coverage Δ
ubuntu 75.23% <70.16%> (-0.17%) ⬇️
windows 75.05% <70.16%> (-0.24%) ⬇️

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

Impacted Files Coverage Δ
cloudapi/config.go 86.66% <ø> (ø)
cmd/cloud.go 13.40% <ø> (ø)
cmd/login_cloud.go 19.76% <0.00%> (ø)
cmd/login_influxdb.go 11.66% <0.00%> (ø)
cmd/ui.go 61.35% <ø> (ø)
errext/exit_code.go 100.00% <ø> (ø)
js/common/bridge.go 100.00% <ø> (ø)
js/common/interrupt_error.go 100.00% <ø> (+18.18%) ⬆️
js/modules/k6/grpc/client.go 78.39% <0.00%> (ø)
js/timeout_error.go 56.25% <ø> (ø)
... and 61 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 521a282...8535b00. Read the comment docs.

Copy link
Collaborator

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @fatelei 🙇
I have left some comments - you will also need to fix your linters

@codebien as I mention the fuller fix for this will require for error codes to be moved around and is arguably a bigger change. Likely one too touchy and big for an outside contributor :(.

I don't think we will be removing this API any time soon so any fixes for it are welcome as long as they aren't overly complex and not actually all the useful. We will definitely prefer to push the newer API. But the old one is still used by people so we will need to support at least until we have actually released a stable version of the new one IMO.

js/modules/k6/ws/ws.go Outdated Show resolved Hide resolved
js/modules/k6/ws/ws_test.go Show resolved Hide resolved
js/modules/k6/ws/ws.go Outdated Show resolved Hide resolved
js/modules/k6/ws/ws_test.go Outdated Show resolved Hide resolved
js/modules/k6/ws/ws_test.go Outdated Show resolved Hide resolved
js/modules/k6/ws/ws_test.go Outdated Show resolved Hide resolved
js/modules/k6/ws/ws_test.go Outdated Show resolved Hide resolved
@codebien
Copy link
Collaborator

@fatelei you can run in local make lint so you don't have to wait for a CI run.

codebien
codebien previously approved these changes Jul 26, 2022
Copy link
Collaborator

@codebien codebien left a comment

Choose a reason for hiding this comment

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

LGTM

js/modules/k6/ws/ws_test.go Outdated Show resolved Hide resolved
js/modules/k6/ws/ws_test.go Outdated Show resolved Hide resolved
js/modules/k6/ws/ws_test.go Show resolved Hide resolved
Copy link
Collaborator

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for getting this through 🙇

@mstoykov mstoykov requested a review from codebien July 28, 2022 11:40
Copy link
Collaborator

@codebien codebien left a comment

Choose a reason for hiding this comment

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

Thanks @fatelei for the effort 🙏

@codebien codebien merged commit 9bf0465 into grafana:master Jul 28, 2022
@na-- na-- added this to the v0.40.0 milestone Jul 28, 2022
@mstoykov mstoykov added the breaking change for PRs that need to be mentioned in the breaking changes section of the release notes label Aug 31, 2022
@mstoykov
Copy link
Collaborator

I have marked this as "breaking change" as the default value for throw is false - so by default we will now not throw an exception while we previously would've.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change for PRs that need to be mentioned in the breaking changes section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants