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: send correct user-agent header for websocket methods #930

Merged
merged 1 commit into from
Aug 8, 2019

Conversation

dpopp07
Copy link
Contributor

@dpopp07 dpopp07 commented Aug 8, 2019

I recently noticed that the analytics headers for the WebSocket methods do not follow the new format. Making this quick change to correct them

@codecov-io
Copy link

codecov-io commented Aug 8, 2019

Codecov Report

Merging #930 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #930      +/-   ##
==========================================
+ Coverage   54.34%   54.39%   +0.05%     
==========================================
  Files          18       18              
  Lines        4501     4502       +1     
  Branches      905      905              
==========================================
+ Hits         2446     2449       +3     
+ Misses       2053     2051       -2     
  Partials        2        2
Impacted Files Coverage Δ
speech-to-text/v1.ts 31.81% <100%> (ø) ⬆️
text-to-speech/v1.ts 46.34% <100%> (+1.34%) ⬆️
test/resources/auth_helper.js 100% <0%> (+22.22%) ⬆️

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 5633782...6e6e784. Read the comment docs.

@@ -87,11 +86,13 @@ class TextToSpeechV1 extends GeneratedTextToSpeechV1 {
params.token_manager = this.tokenManager;
}

// include analytics headers
const sdkHeaders = getSdkHeaders('text_to_speech', 'v1', 'synthesizeUsingWebSocket');
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this break the browser users?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm I'm not sure. We were still sending the user-agent header before but not the x-ibm-cloud-analytics header so it depends on if that will cause CORS issues.

That said, these two operations are the ones we have a completely separate browser package for so I don't know if it's a big deal 🤷‍♂

@dpopp07 dpopp07 merged commit f24cac2 into master Aug 8, 2019
@dpopp07 dpopp07 deleted the add-analytics-headers-to-websockets branch August 8, 2019 20:10
dpopp07 added a commit that referenced this pull request Aug 8, 2019
watson-github-bot pushed a commit that referenced this pull request Aug 8, 2019
## [4.3.3](v4.3.2...v4.3.3) (2019-08-08)

### Bug Fixes

* send correct user-agent header for websocket methods ([#930](#930)) ([f24cac2](f24cac2))
@watson-github-bot
Copy link
Member

🎉 This PR is included in version 4.3.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

4 participants