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!: standardize supported versions and set upper bound limit #2196

Open
wants to merge 62 commits into
base: main
Choose a base branch
from

Conversation

blumamir
Copy link
Member

@blumamir blumamir commented May 11, 2024

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

  • make sure there is upper limit on all supported versions so that they do not attempt to patch a new major version
  • align the range to a common format. It seems many instrumentation where using >=x.y.z <w writing so I used it
  • README.md: added where missing, moved to always be after Installation section, added package name and npm link before version range to give more context.
  • added section to GUIDELINES.md
  • replace all the use of astrix * 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

@blumamir
Copy link
Member Author

@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 code suggestion feature of PR comments in the UI. I will squash and merge all the commits after your review so to not lose and existing comments.

The one last thing I am not sure about is if we should choose caret ^ range when the version range is just one major version. I am split on this, but tend to prefer replacing it with >=1.0.0 <2 instead of ^1.0.0. WDYT?

Thanks again for those great comments suggestions and catches.

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.

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.

GUIDELINES.md Outdated Show resolved Hide resolved
GUIDELINES.md Outdated Show resolved Hide resolved
@JamieDanielson
Copy link
Member

The one last thing I am not sure about is if we should choose caret ^ range when the version range is just one major version. I am split on this, but tend to prefer replacing it with >=1.0.0 <2 instead of ^1.0.0. WDYT?

I actually prefer >=1.0.0 <2 over ^1.0.0 and would support using that. It also makes it more consistent with the other larger ranges.

@JamieDanielson
Copy link
Member

/easycla

@trentm
Copy link
Contributor

trentm commented May 22, 2024

I actually prefer >=1.0.0 <2 over ^1.0.0

Same.

Copy link
Member

@JamieDanielson JamieDanielson left a 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 👍

@blumamir
Copy link
Member Author

The one last thing I am not sure about is if we should choose caret ^ range when the version range is just one major version. I am split on this, but tend to prefer replacing it with >=1.0.0 <2 instead of ^1.0.0. WDYT?

I actually prefer >=1.0.0 <2 over ^1.0.0 and would support using that. It also makes it more consistent with the other larger ranges.

Changed the caret as described 👍

blumamir and others added 2 commits June 19, 2024 16:32
Co-authored-by: Trent Mick <[email protected]>
Co-authored-by: Trent Mick <[email protected]>
@blumamir blumamir changed the title docs: align all supported versions to a common format fix!: standardize supported versions and set upper bound limit Jun 19, 2024
@blumamir
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment