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: various compilation errors #170

Merged
merged 5 commits into from
Aug 10, 2020

Conversation

naseemkullah
Copy link
Member

@naseemkullah naseemkullah commented Aug 6, 2020

Which problem is this PR solving?

Short description of the changes

  • Use @types/mysql ConnectionConfig and PoolActualConfig to overcome compilation error with @types/mysql 2.15.15

@naseemkullah naseemkullah requested a review from a team August 6, 2020 02:22
By importing the and using the ConnectionConfig and PoolActualConfig types.

Signed-off-by: Naseem <[email protected]>
@naseemkullah naseemkullah changed the title fix: compilation issue with latest @types/mysql fix: various compilation errors Aug 6, 2020
@naseemkullah naseemkullah added the dependencies Pull requests that update a dependency file label Aug 6, 2020
@codecov
Copy link

codecov bot commented Aug 6, 2020

Codecov Report

Merging #170 into master will decrease coverage by 0.41%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #170      +/-   ##
==========================================
- Coverage   94.10%   93.69%   -0.42%     
==========================================
  Files          74       67       -7     
  Lines        3582     3268     -314     
  Branches      387      354      -33     
==========================================
- Hits         3371     3062     -309     
+ Misses        211      206       -5     
Impacted Files Coverage Δ
...in-dns/test/integrations/dnspromise-lookup.test.ts
...ins/node/opentelemetry-plugin-express/.eslintrc.js
...ect/plugins/node/opentelemetry-plugin-pg/src/pg.ts
...gator-grpc-census-binary/src/BinaryTraceContext.ts
...node/opentelemetry-plugin-mysql/test/mysql.test.ts
...pc-census-binary/test/GrpcCensusPropagator.test.ts
...ugins/node/opentelemetry-plugin-dns/src/version.ts
.../opentelemetry-plugin-express/test/express.test.ts
...ins/node/opentelemetry-plugin-redis/src/version.ts
...ins/node/opentelemetry-plugin-pg-pool/src/utils.ts
... and 107 more

@naseemkullah
Copy link
Member Author

@dyladan I went down a rabbit hole here, and it appears that many things were failing due to core and/or other packages being out of date. I am left with a remaining error with the mysql plugin tests:

         should ......:
     Uncaught TypeError: core_1.randomSpanId is not a function

Any idea what may be going on?

@vmarchaud
Copy link
Member

@naseemkullah I believe that someone implemented a interface to generate span/trace ids in the last release, it might be coming from that

@dyladan
Copy link
Member

dyladan commented Aug 6, 2020

@naseemkullah I believe that someone implemented a interface to generate span/trace ids in the last release, it might be coming from that

This seems the most likely to me

@michaelgoin
Copy link
Contributor

@naseemkullah i believe they are right. will leave a comment in the code on your PR if i can but here's a related comment on a koa PR related also to express: #144 (comment)

@@ -46,7 +46,7 @@
"@opentelemetry/test-utils": "^0.9.0",
"@opentelemetry/tracing": "0.10.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

tracing being on 0.10.2 is what causes the randomSpanId issue. api/core will update to 0.10.2 on fresh installs due to being set to ^0.10.2.

looks like in related places you bumped everything to 0.10.2/^0.10.2, so i'd prob do that here as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah right, thanks!

Naseem added 2 commits August 6, 2020 16:40
@naseemkullah
Copy link
Member Author

This is now passing and ready to merge, should unblock other PRs such as #143 and #167 @open-telemetry/javascript-approvers

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

great !

@vmarchaud
Copy link
Member

@open-telemetry/javascript-maintainers I think we can merge this one so it can unblocks other PRs (ex: #144)

@mayurkale22 mayurkale22 merged commit d4faad2 into open-telemetry:master Aug 10, 2020
@obecny obecny added the enhancement New feature or request label Sep 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants