-
Notifications
You must be signed in to change notification settings - Fork 524
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
feat(instr-tedious): add support for v16 and v17 #2178
Conversation
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.
Otherwise, LGTM. I'll re-review after the test-utils.ts change conversation.
…opentelemetry-js-contrib into dluna/update-tedious-support
@@ -78,7 +78,7 @@ export class TediousInstrumentation extends InstrumentationBase { | |||
return [ | |||
new InstrumentationNodeModuleDefinition( | |||
TediousInstrumentation.COMPONENT, | |||
['>=1.11.0 <=15'], | |||
['>=1.11.0 <18'], |
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.
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.
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.
I will rebase my PR after this one merges 👍
@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? |
@trentm Done. When I added the code suggestion, it seems GH took my private email. |
/easycla |
@kirrg001 Hrm, it is still unresolved here. |
I did it again. And the "not linked" email is also part of my Github account. Its even my primary at the moment. https://confluence.linuxfoundation.org/pages/viewpage.action?pageId=86641302 |
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. |
I can look into rebasing-away that commit and force-pushing later. |
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. |
Which problem is this PR solving?
tedious
Closes: #1656
Short description of the changes
tav.yml