-
Notifications
You must be signed in to change notification settings - Fork 516
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!: standardize supported versions and set upper bound limit #2196
Conversation
@trentm @JamieDanielson I think I addressed all the comments from your reviews. Since these are a lot of changes, please have a look again. I think EasyCLA is having hard time with the commits done via the The one last thing I am not sure about is if we should choose caret Thanks again for those great comments suggestions and catches. |
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.
Some nits. I think it would be good to not have this labelled as purely a "docs" PR because it does include code changes for a few modules to add an upper limit on the instrumented versions.
plugins/node/opentelemetry-instrumentation-generic-pool/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
I actually prefer |
/easycla |
Same. |
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 have similar nits to what Trent pointed out, aside from those LGTM 👍
Changed the caret as described 👍 |
Co-authored-by: Trent Mick <trentm@gmail.com>
Co-authored-by: Trent Mick <trentm@gmail.com>
Addressed all the comments (I think). @JamieDanielson @trentm can you please have one last look so this can merge? thanks again for the great feedback |
This PR is not breaking any existing behavior, it only aligns everywhere to use a consistent versioning syntax and documentation, as well as capping supportedVersions for any existing instrumentation
>=x.y.z <w
writing so I used itInstallation
section, added package name and npm link before version range to give more context.*
for unordered list style in makrdown to use dash-
, it causes errors in markdown linter after introducing new sections.related: open-telemetry/opentelemetry-js#4693
related: open-telemetry/opentelemetry-js#4696