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

Replace unmaintained actions-rs/toolchain action. #1655

Merged
merged 12 commits into from
Apr 15, 2024

Conversation

lalitb
Copy link
Member

@lalitb lalitb commented Apr 3, 2024

Fixes #1654
Replacing actions-rs/toolchain action with more actively maintained and used dtolnay/rust-toolchain. While rustup is available on Github actions runners, I feel it would be less of maintanance efffort with using an action. But happy to change if there are other thoughts.

Also, rust latest beta compiler/toolchain has more accurate redundant import warning - rust-lang/rust#117772 - so fixed the code to remove redundant imports. Basically remove import for TryInto, TryFrom and FromIterator traits, as they are part of std::prelude.

Changes

Please provide a brief description of the changes here.

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 April 3, 2024 19:13
@lalitb
Copy link
Member Author

lalitb commented Apr 3, 2024

Ok there is error in workflow

Check failure on line 27 in .github/workflows/ci.yml

GitHub Actions
/ CI
Invalid workflow file
The workflow is not valid. .github/workflows/ci.yml (Line: 27, Col: 13): Unrecognized named-value: 'matrix'. Located at position 1 within expression: matrix.rust

Let me see how to fix this.

Copy link

codecov bot commented Apr 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.3%. Comparing base (278f5c7) to head (2e5a33a).
Report is 3 commits behind head on main.

❗ Current head 2e5a33a differs from pull request most recent head 0fd0802. Consider uploading reports for the commit 0fd0802 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1655     +/-   ##
=======================================
+ Coverage   68.9%   69.3%   +0.3%     
=======================================
  Files        136     136             
  Lines      19429   19637    +208     
=======================================
+ Hits       13396   13610    +214     
+ Misses      6033    6027      -6     

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

@lalitb
Copy link
Member Author

lalitb commented Apr 3, 2024

The tests are failing with this toolchain action, moving it to draft to fix it.

@lalitb lalitb marked this pull request as draft April 3, 2024 21:33
@lalitb lalitb marked this pull request as ready for review April 3, 2024 22:47
@lalitb
Copy link
Member Author

lalitb commented Apr 3, 2024

Ok, this is ready to review now. Some more changes are done to fix redundant import warning, coming from the latest beta build - rust-lang/rust#117772


mod throughput;

struct NoopEventVisitor;
Copy link
Member Author

Choose a reason for hiding this comment

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

NoOpLogLayer and NoopEventVisitor are not used, so removing them,

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we not need them for the future?

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 don't see them used for stress test. We already have the benchmark test for them here -

impl<S> Layer<S> for NoOpLogLayer

Copy link
Contributor

Choose a reason for hiding this comment

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

We can probably revive them if needed

Copy link

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

LGTM.

Note

.github/workflows/integration_tests.yml is modified by this PR but that test is skipped.

@lalitb lalitb added the integration tests Run integration tests label Apr 4, 2024
@lalitb
Copy link
Member Author

lalitb commented Apr 4, 2024

.github/workflows/integration_tests.yml is modified by this PR but that test is skipped.

Thanks, have forced run it now.

Copy link
Contributor

@TommyCpp TommyCpp left a comment

Choose a reason for hiding this comment

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

While rustup is available on Github actions runners, I feel it would be less of maintanance efffort with using an action

Agree with this. I prefer less change needed for CICD

.github/workflows/ci.yml Show resolved Hide resolved

mod throughput;

struct NoopEventVisitor;
Copy link
Contributor

Choose a reason for hiding this comment

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

We can probably revive them if needed

Copy link
Contributor

@TommyCpp TommyCpp left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

@cclauss
Copy link

cclauss commented Apr 5, 2024

Oh... Still two lint warnings at the bottom right of
https://github.com/open-telemetry/opentelemetry-rust/actions/runs/8559370429

@lalitb
Copy link
Member Author

lalitb commented Apr 5, 2024

Looks like something has changed in the GitHub runner images, as all existing PRs are failing with error:

thread 'build_tonic' panicked at opentelemetry-proto/tests/grpc_build.rs:116:9:
generated file has changed but it's a CI environment, please rerun this test locally and commit the changes
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@lalitb
Copy link
Member Author

lalitb commented Apr 15, 2024

Looks like something has changed in the GitHub runner images, as all existing PRs are failing with error:

thread 'build_tonic' panicked at opentelemetry-proto/tests/grpc_build.rs:116:9:
generated file has changed but it's a CI environment, please rerun this test locally and commit the changes
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

All the issues are resolved. Should be good to merge.

@cijothomas cijothomas merged commit f203b03 into open-telemetry:main Apr 15, 2024
14 of 15 checks passed
@lalitb lalitb mentioned this pull request Apr 16, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration tests Run integration tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: actions-rs/toolchain is archived
5 participants