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

Follow HTTP redirects after failed WS dials #251

Merged
merged 7 commits into from
Feb 23, 2024
Merged

Follow HTTP redirects after failed WS dials #251

merged 7 commits into from
Feb 23, 2024

Conversation

echlebek
Copy link
Contributor

This commit allows the opamp client to follow redirects after websocket handshake failures. Redirect following is not implemented by gorilla/websocket, but can be handled by inspecting the returned response object for 3xx status and Location header.

Closes #250

This commit allows the opamp client to follow redirects after websocket
handshake failures. Redirect following is not implemented by
gorilla/websocket, but can be handled by inspecting the returned
response object for 3xx status and Location header.
@echlebek echlebek requested a review from a team February 12, 2024 17:29
Copy link

linux-foundation-easycla bot commented Feb 12, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@tigrannajaryan
Copy link
Member

Thank you for the PR @echlebek
Please sign the CLA.

@echlebek
Copy link
Contributor Author

@tigrannajaryan Thanks for taking a look, I've signed the CLA.

client/wsclient.go Outdated Show resolved Hide resolved
client/wsclient.go Outdated Show resolved Hide resolved
client/wsclient.go Show resolved Hide resolved
client/wsclient.go Outdated Show resolved Hide resolved
client/wsclient_test.go Show resolved Hide resolved
Copy link

codecov bot commented Feb 13, 2024

Codecov Report

Attention: Patch coverage is 85.71429% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 73.74%. Comparing base (ce8a8dd) to head (f6cc5df).

Files Patch % Lines
client/wsclient.go 85.71% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #251      +/-   ##
==========================================
+ Coverage   73.45%   73.74%   +0.28%     
==========================================
  Files          25       25              
  Lines        1537     1550      +13     
==========================================
+ Hits         1129     1143      +14     
+ Misses        298      297       -1     
  Partials      110      110              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@echlebek
Copy link
Contributor Author

Coverage is now improved, with only one line missing coverage from the method. It would require a TLS server in test to cover it. Let me know if you think it's worthwhile. Thanks!

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, coverage looks good.
Just one comment remaining.

client/wsclient.go Outdated Show resolved Hide resolved
@tigrannajaryan tigrannajaryan merged commit 8b26910 into open-telemetry:main Feb 23, 2024
6 checks passed
@tigrannajaryan
Copy link
Member

Thank you @echlebek !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Follow HTTP redirects on websocket handshake failure
2 participants