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

feat(instr-tedious): add support for v16 and v17 #2178

Merged
merged 18 commits into from
May 22, 2024

Conversation

david-luna
Copy link
Contributor

Which problem is this PR solving?

  • adds support for newer versions of tedious

Closes: #1656

Short description of the changes

  • add newer versions into the instrumentation
  • add a version check in the tests to avoid testing with incopatible nodejs versions
  • update tav.yml

Copy link

codecov bot commented May 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.37%. Comparing base (dfb2dff) to head (179a830).
Report is 133 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2178      +/-   ##
==========================================
- Coverage   90.97%   90.37%   -0.61%     
==========================================
  Files         146      147       +1     
  Lines        7492     7502      +10     
  Branches     1502     1571      +69     
==========================================
- Hits         6816     6780      -36     
- Misses        676      722      +46     
Files Coverage Δ
...ode/instrumentation-tedious/src/instrumentation.ts 92.30% <ø> (-0.17%) ⬇️

... and 43 files with indirect coverage changes

@david-luna david-luna requested a review from kirrg001 May 10, 2024 11:57
Copy link
Contributor

@trentm trentm left a comment

Choose a reason for hiding this comment

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

Otherwise, LGTM. I'll re-review after the test-utils.ts change conversation.

plugins/node/instrumentation-tedious/package.json Outdated Show resolved Hide resolved
plugins/node/instrumentation-tedious/.tav.yml Outdated Show resolved Hide resolved
@@ -78,7 +78,7 @@ export class TediousInstrumentation extends InstrumentationBase {
return [
new InstrumentationNodeModuleDefinition(
TediousInstrumentation.COMPONENT,
['>=1.11.0 <=15'],
['>=1.11.0 <18'],
Copy link
Contributor

@trentm trentm May 13, 2024

Choose a reason for hiding this comment

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

nit: It would perhaps be nice to have this version string be the same as is used in the README. However, it is totally fine to leave that for now.

@blumamir was talking about a separate PR to pick a preferred form (>=${theMinVer} <=${theMaxMajorVer} or >=${theMinVer} <${theMaxMajorVer+1}`) and update many/most of the isntrumentations.

Copy link
Member

Choose a reason for hiding this comment

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

I will rebase my PR after this one merges 👍

@trentm trentm enabled auto-merge (squash) May 17, 2024 18:28
Copy link

linux-foundation-easycla bot commented May 17, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@trentm
Copy link
Contributor

trentm commented May 17, 2024

@kirrg001 Merging this is currently failing on the CLA check failing for the email address used for your commited suggestion. Are you able to sign the CLA? Or, if you already have, can you look into why the easycla comment above indicates a failure?

@kirrg001
Copy link
Contributor

@trentm Done. When I added the code suggestion, it seems GH took my private email.

@trentm
Copy link
Contributor

trentm commented May 21, 2024

/easycla

@trentm
Copy link
Contributor

trentm commented May 21, 2024

@kirrg001 Hrm, it is still unresolved here.

@kirrg001
Copy link
Contributor

I did it again. And the "not linked" email is also part of my Github account. Its even my primary at the moment.
Maybe the problem is that I already have other commits in the repo with a different email (katharina.irrgang@ibm.com) and it can't link this commit to my account, because its a different email.

https://confluence.linuxfoundation.org/pages/viewpage.action?pageId=86641302

@kirrg001
Copy link
Contributor

I guess the easiest fix is to remove the commit. Its only a tiny change. And then re-add the change with a new commit.
@david-luna

@trentm
Copy link
Contributor

trentm commented May 22, 2024

I guess the easiest fix is to remove the commit.

I can look into rebasing-away that commit and force-pushing later.

@trentm
Copy link
Contributor

trentm commented May 22, 2024

There is also a current known issue with EasyCLA and "Co-authored-by": communitybridge/easycla#4329 Not sure if that is what we are hitting here.

@trentm trentm merged commit 8c578cd into open-telemetry:main May 22, 2024
12 checks passed
@dyladan dyladan mentioned this pull request May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment