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

ci: run clippy on features separately to find issues #2866

Merged

Conversation

bantonsson
Copy link
Contributor

Changes

This runs clippy on features separately to ensure that the code doesn't rely on other features being enabled.

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)

@bantonsson bantonsson requested a review from a team as a code owner March 26, 2025 13:45
@bantonsson bantonsson force-pushed the ban/lint-features-separately branch from 397dc97 to 4699440 Compare March 26, 2025 13:46
Copy link

codecov bot commented Mar 26, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.8%. Comparing base (da2029e) to head (99998fc).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #2866     +/-   ##
=======================================
- Coverage   80.8%   80.8%   -0.1%     
=======================================
  Files        124     124             
  Lines      23833   23836      +3     
=======================================
  Hits       19280   19280             
- Misses      4553    4556      +3     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@cijothomas
Copy link
Member

info: component 'clippy' for target 'x86_64-unknown-linux-gnu' is up to date
Updating crates.io index
Downloading crates ...
Downloaded cargo-hack v0.6.36
error: binary cargo-hack already exists in destination
Add --force to overwrite

@bantonsson Can you check this log from the CI run and see if this is to be fixed

@bantonsson bantonsson force-pushed the ban/lint-features-separately branch 2 times, most recently from 1ccf589 to ae3d68e Compare March 26, 2025 15:39
@bantonsson
Copy link
Contributor Author

@cijothomas The CI / lint script/job has now been fixed.

@cijothomas
Copy link
Member

lint is now ~6-7 mins, previously ~3 mins.
This is still less than the longest CI check (Windows), so won't increase overall time.

@bantonsson bantonsson force-pushed the ban/lint-features-separately branch 2 times, most recently from f5663a3 to 3a2e6fe Compare March 28, 2025 07:48
@bantonsson
Copy link
Contributor Author

So what is the process for getting this merged when it's been approved?

@bantonsson bantonsson force-pushed the ban/lint-features-separately branch from 3a2e6fe to c8d7a99 Compare March 28, 2025 12:51
@@ -19,7 +19,7 @@ opentelemetry_sdk = { path = "../../opentelemetry-sdk", features = ["rt-tokio"]
opentelemetry-stdout = { workspace = true, features = ["trace"] }
prost = { workspace = true }
tokio = { workspace = true, features = ["full"] }
tonic = { workspace = true, features = ["server", "codegen", "channel", "prost"] }
tonic = { workspace = true, features = ["server", "codegen", "channel", "prost", "router"] }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI, while this PR was waiting, a change missing this landed in main and wasn't caught by CI.

@cijothomas
Copy link
Member

cijothomas commented Mar 28, 2025

So what is the process for getting this merged when it's been approved?

https://github.com/open-telemetry/opentelemetry-rust/blob/main/CONTRIBUTING.md#how-to-get-prs-merged
Only maintainers have the access to "click the merge button". (same across entire Otel repos).
Nothing you need to do - some maintainer (most likely me!) will click the merge!

@cijothomas cijothomas merged commit 50f0bb8 into open-telemetry:main Mar 28, 2025
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.

2 participants