-
Notifications
You must be signed in to change notification settings - Fork 795
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(@opentelemetry/exporter-collector-grpc) regression from #2130 when host specified without protocol #2322
fix(@opentelemetry/exporter-collector-grpc) regression from #2130 when host specified without protocol #2322
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2322 +/- ##
=======================================
Coverage 92.77% 92.77%
=======================================
Files 145 145
Lines 5221 5221
Branches 1070 1070
=======================================
Hits 4844 4844
Misses 377 377 |
browser-tests appears to be a flake |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, but I think it would be best to follow the assertion patterns seen in other tests, I left some (untested 😬) comments to adhere to that, this also would mean removing the added chai packages (furthermore i think the hoisted eslint packages are used so probably no need to add them either)
packages/opentelemetry-exporter-collector-grpc/test/util.test.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-exporter-collector-grpc/test/util.test.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-exporter-collector-grpc/test/util.test.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-exporter-collector-grpc/test/util.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, i agree with @naseemkullah suggestions though
I'll remove the chai packages -- I checked and they were already present as indirect deps but I didn't realize they were only used for tsconfig-paths and not for our stuff. wrt adding the hoisted eslint -- it makes it easier to have eslint in there so I can run |
Co-authored-by: Naseem <[email protected]>
3c2cd6f
to
c3b5739
Compare
Co-authored-by: Naseem <[email protected]>
842165b
to
2aa5f4e
Compare
PTAL |
I don't have permission to update your branch for merge |
updated branch. |
Which problem is this PR solving?
host:port
fails becausehost
is used as the URL protocol instead of as a hostname.Short description of the changes
validateAndNormalizeUrl
and corrects issues that prevented them from passing.