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 msrv #1877

Merged
merged 2 commits into from
Jun 13, 2024
Merged

fix msrv #1877

merged 2 commits into from
Jun 13, 2024

Conversation

lalitb
Copy link
Member

@lalitb lalitb commented Jun 10, 2024

Fixes #1874
Design discussion issue (if applicable) #

Changes

The opentelemetry-http has an indirect dependency on the url crate via reqwest. Pinning the url crate to a version that supports Rust 1.65.0.

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 June 10, 2024 23:57
Copy link

codecov bot commented Jun 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.6%. Comparing base (b933bdd) to head (4685646).

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #1877   +/-   ##
=====================================
  Coverage   74.6%   74.6%           
=====================================
  Files        122     122           
  Lines      19952   19952           
=====================================
  Hits       14902   14902           
  Misses      5050    5050           

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

Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

changelog ?

@lalitb
Copy link
Member Author

lalitb commented Jun 11, 2024

changelog ?

do we need it, as it won't affect customers. They can still continue using their own version of url crate for their application usage.

@@ -45,4 +45,4 @@ tokio-stream = "0.1.1"
tracing = { version = "0.1", default-features = false }
tracing-core = { version = "0.1", default-features = false }
tracing-subscriber = { version = "0.3", default-features = false }
url = { version = "2.2", default-features = false }
url = { version = "=2.5.0", default-features = false } #pinning the version supporting rustc 1.65
Copy link
Contributor

Choose a reason for hiding this comment

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

Why need ping this version to support the given rustc?

Copy link
Member Author

@lalitb lalitb Jun 11, 2024

Choose a reason for hiding this comment

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

If not pinned, the latest version 2.5.1 will be used, which requires rust 1.67.
Another option would be to bump to msrv 1.67, but as we can still support 1.65 with minimal changes, so prefer the pinning.

@cijothomas
Copy link
Member

changelog ?

do we need it, as it won't affect customers. They can still continue using their own version of url crate for their application usage.

there is bump from 2.2 to 2.5.1 right?

@lalitb
Copy link
Member Author

lalitb commented Jun 13, 2024

there is bump from 2.2 to 2.5.1 right?

Not actually. With URL "=2.5.0", we are ensuring that the reqwest crate in otel-http doesn't use url crate version beyond "2.5.0". Customers needn't be interested to know as long as it doesn't affect their usage of otel crates with the currently supported rust version. And this doesn't anyway affect the direct usage of these crates (reqwest, url) in customer's application.

@cijothomas cijothomas merged commit 64aa506 into open-telemetry:main Jun 13, 2024
24 checks passed
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.

[Bug]: CI failure
3 participants