-
Notifications
You must be signed in to change notification settings - Fork 821
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(instrumentation): use caret range on import-in-the-middle #4380
fix(instrumentation): use caret range on import-in-the-middle #4380
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #4380 +/- ##
==========================================
+ Coverage 92.21% 92.24% +0.02%
==========================================
Files 333 333
Lines 9459 9459
Branches 2009 2009
==========================================
+ Hits 8723 8725 +2
+ Misses 736 734 -2 |
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 didn't find a clue why this was pinned. AFAICT, external dependencies are generally not pinned. Maybe @JamieDanielson can chime in?
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 am okay with unpinning it. IOW, basically I trust the IITM authors to follow semver.
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 am also ok unpinning
Alright, seems like no objections to unpinning - let's do that then. 🙂 |
FWIW I don't fully remember either, but I suspect it was probably intentional at the time because the initial instrumentation change was experimental and potentially delicate, so we were concerned about unexpected behavior if the exact version and behavior changed. At this point though it makes sense to follow our general guidelines, glad to see this got updated 👍 |
…elemetry#4380) * fix(instrumentation): use caret on import-in-the-middle * fix: add chanelog entry --------- Co-authored-by: Daniel Dyla <[email protected]>
Which problem is this PR solving?
import-in-the-middle
is having a lot of changes right now to keep up with changes in Node.js. This PR unpinsimport-in-the-middle
to ensure our users can also keep up without requiring a new@opentelemetry/instrumentation
release each time.This PR is intended as more of a discussion-starter. There are a few pros and cons to both sides (keeping it pinned vs. unpinning it)
Type of change