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

Update README with guidance on version updates #1979

Closed
wants to merge 5 commits into from

Conversation

lalitb
Copy link
Member

@lalitb lalitb commented Jul 31, 2024

Changes

Adding a disclaimer that it is possible to have breaking changes with minor version updates, till we are stable. Based on discussions here - open-telemetry/opentelemetry-rust-contrib#98 (comment) and #1966 (comment)

While Versioning.md could have been the better place to add this, README.md will get better attention :)

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

@lalitb lalitb requested a review from a team July 31, 2024 20:20
@lalitb lalitb changed the title Update README with Guidance on Crate Version Compatibility and Updates Update README with Guidance on version updates Jul 31, 2024
@lalitb lalitb changed the title Update README with Guidance on version updates Update README with guidance on version updates Jul 31, 2024
Copy link

codecov bot commented Jul 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.9%. Comparing base (cfbd1c3) to head (44b05f8).

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #1979   +/-   ##
=====================================
  Coverage   74.9%   74.9%           
=====================================
  Files        122     122           
  Lines      20411   20411           
=====================================
  Hits       15296   15296           
  Misses      5115    5115           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

each of which may have different version numbers. To maintain compatibility
and ensure proper functionality, it is recommended to update all related
crates simultaneously when new versions are released. Although it would be
ideal to avoid breaking changes with minor version updates, this is currently
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes it sound like are we are required to avoid such breaking changes, but we would not be able to do it. It's completely valid under Semver compatibility rules to make breaking changes with minor version updates for 0.x.x versions.

If the intention is to warn users who might have misconception about what kind of breaking changes are allowed, maybe we could just reiterate the relevant Semver compatibility rule. Something similar to the text in this doc:

Versions are considered compatible if their left-most non-zero major/minor/patch component is the same. For example, 0.1.0 and 0.1.2 are compatible, but 0.1.0 and 0.2.0 are not.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's completely valid under Semver compatibility rules to make breaking changes with minor version updates for 0.x.x versions.

This is what I read from semver:

MAJOR version when you make incompatible API changes
MINOR version when you add functionality in a backward compatible manner
PATCH version when you make backward compatible bug fixes

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh you mean with 0 as major version.

Copy link
Member Author

Choose a reason for hiding this comment

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

The issue I observe is that in the Rust ecosystem, most of crates are in 0.x.x versions but are considered stable, with minimal breaking changes occurring with minor version updates. I just wanted to emphasize that for OpenTelemetry, this would be comparatively more frequent.

Copy link
Contributor

@utpilla utpilla Jul 31, 2024

Choose a reason for hiding this comment

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

Got it. Thanks for providing the context! Maybe we can put this text under its own section in the doc to highlight this point even more? Something like this: https://github.com/rust-random/rand?tab=readme-ov-file#versions

Copy link
Member

Choose a reason for hiding this comment

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

I am unsure what is the issue we are trying to address with this wording? Can you elaborate.
Is it to convey that we are not stable?
Is it to convey that breaking changes are possible within minor version bump, until a crate hits 1.0?
Or more?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am unsure what is the issue we are trying to address with this wording? Can you elaborate.
Is it to convey that we are not stable?
Is it to convey that breaking changes are possible within minor version bump, until a crate hits 1.0?
Or more?

Maybe I haven't put the wordings properly. The main emphasis was this

To maintain compatibility and ensure proper functionality, it is recommended to update all related
crates simultaneously when new versions are released.

i.e., to ask users to update to the latest versions of all OpenTelemetry crates together when new releases are made. Additionally, if there are any indirect dependencies on OpenTelemetry crates (e.g. such as those coming through tracing-opentelemetry), they should ensure that the versions are same (preferably latest). If not, reach out to the repo providing this dependency asking them to update.

But looks like the PR is causing more confusion, and less value. Would be good to close this :)

@lalitb
Copy link
Member Author

lalitb commented Aug 1, 2024

Closing this for now. We should be adding the guidance for the version updates because of the interlinked crates published from this repo, will come up with better wordings after discussing in community meeting.

@lalitb lalitb closed this Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants