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.

Sign up for GitHub

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

Jump to bottom

CI: fix clippy issues #1013

Merged
merged 3 commits into from
Jun 20, 2024
Merged

CI: fix clippy issues #1013

merged 3 commits into from
Jun 20, 2024

Conversation

muzarski
Copy link
Contributor

There were two clippy checks that were failing:

This PR fixes the issues and makes CI pass.

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • [ ] I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • [ ] I have provided docstrings for the public items that I want to introduce.
  • [ ] I have adjusted the documentation in ./docs/source/.
  • [ ] I added appropriate Fixes: annotations to PR description.

@muzarski muzarski requested review from Lorak-mmk and wprzytula June 18, 2024 16:46
@muzarski muzarski self-assigned this Jun 18, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 18, 2024 •
edited
Loading

cargo semver-checks found no API-breaking changes in this PR! 🎉🥳
Checked commit: c531974

@muzarski
Copy link
Contributor Author

It looks like GH actions used the old version of clippy: clippy 0.1.78 (9b00956 2024-04-29).
My version locally is: clippy 0.1.79 (129f3b9 2024-06-10).

That's weird, because the runners used for CI in cql-stress already use newest version of clippy.

@Lorak-mmk
Copy link
Collaborator

Afaik rustup is present in runner images. Maybe we should explicitly install latest stable using rustup and set it as default in CI instead of relying on what is the current default on the runners.

@muzarski
Copy link
Contributor Author

Afaik rustup is present in runner images. Maybe we should explicitly install latest stable using rustup and set it as default in CI instead of relying on what is the current default on the runners.

v2: Added a step which updates rust toolchain in CI.

@muzarski muzarski mentioned this pull request Jun 19, 2024
Closed
@muzarski muzarski requested a review from Lorak-mmk June 19, 2024 10:00
@muzarski
Copy link
Contributor Author

Afaik rustup is present in runner images. Maybe we should explicitly install latest stable using rustup and set it as default in CI instead of relying on what is the current default on the runners.

v2: Added a step which updates rust toolchain in CI.

@wprzytula @Lorak-mmk Do you think that we should do the same for other workflows that use rust toolchain? As of now, we only update it for Rust / build workflow, which does all of the clippy and format checks. However, there are other workflows that use rust compiler, e.g. Cassandra tests / build or Book / build (they do not use clippy/rustfmt, tho).

@wprzytula
Copy link
Collaborator

Do you think that we should do the same for other workflows that use rust toolchain?

I can see two approaches.

  1. Unified CI, if we always assume that we only check on two Rust versions: latest stable and MSRV. This will hopefully result in less CI failures due to such problems as toolchain version difference locally vs in CI.
  2. More than one recent stable toolchain version is used. E.g. latest and the last-before-the-latest. I think that it's a shot in a foot, because even more inconvenience will follow from such dualism.

@Lorak-mmk
Copy link
Collaborator

I agree, let's use latest stable everywhere, and msrv in msrv pipeline

muzarski added 3 commits June 19, 2024 15:06
@muzarski
CI: update rust toolchain in workflows
d6cb5f1 
We decided to unify CI, and make use of latest stable version of
rust toolchain for each of the workflows that actually uses it.
The only exception is `Rust / min_rust` job, which uses MSRV.
@muzarski
macros: clippy - allow manual unwrap_or_default
cb332fa 
Clippy complains about the code generated by
`darling` crate. This commit silences clippy by allowing
to perform manual unwrap_or_default.
@muzarski
connection: stop using legacy numeric constant
c531974 
Clippy complains about using `std::i16::MAX`, instead of
`i16::MAX`. This commit fixes the issue.
@muzarski
Copy link
Contributor Author

v2: using latest stable version of rust toolchain in CI (min_rust is exception - using MSRV)

@muzarski muzarski requested a review from wprzytula June 20, 2024 10:49
@wprzytula
Copy link
Collaborator

@Lorak-mmk Please re-review and merge.

@Lorak-mmk Lorak-mmk merged commit c8db77b into scylladb:main Jun 20, 2024
11 checks passed
wprzytula pushed a commit to wprzytula/scylla-rust-driver that referenced this pull request Jul 11, 2024
@Lorak-mmk @wprzytula
Merge pull request scylladb#1013 from muzarski/clippy_fix_06_18
8dfea68 
CI: fix clippy issues
(cherry picked from commit c8db77b)
@wprzytula wprzytula mentioned this pull request Jul 11, 2024
Merged
wprzytula pushed a commit to wprzytula/scylla-rust-driver that referenced this pull request Jul 11, 2024
@Lorak-mmk @wprzytula
Merge pull request scylladb#1013 from muzarski/clippy_fix_06_18
5a9072a 
CI: fix clippy issues
(cherry picked from commit c8db77b)
@muzarski muzarski deleted the clippy_fix_06_18 branch October 29, 2024 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@piodul piodul piodul left review comments

@wprzytula wprzytula wprzytula approved these changes

@Lorak-mmk Lorak-mmk Lorak-mmk approved these changes

Assignees

@muzarski muzarski

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@muzarski @Lorak-mmk @wprzytula @piodul