Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 27 additions & 9 deletions .github/workflows/linter.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ on:
env:
CARGO_TERM_COLOR: always
CARGO_REGISTRIES_CRATES_IO_PROTOCOL: sparse
RUST_VERSION_NEW: "1.88.0"
RUST_VERSION_NEW: "stable"

jobs:
check-code-style:
Expand All @@ -21,16 +21,29 @@ jobs:
runs-on: ubuntu-24.04

steps:
- name: Install rust
- name: Checkout
uses: actions/checkout@v4
with:
submodules: true

- name: Install nightly rust
if: matrix.rust_version == 'RUST_VERSION_NEW'
uses: dtolnay/rust-toolchain@nightly
with:
components: clippy, rustfmt

- name: Install stable rust
uses: dtolnay/rust-toolchain@v1
with:
toolchain: ${{ env[matrix.rust_version] }}
components: clippy, rustfmt

- name: Checkout
uses: actions/checkout@v4
with:
submodules: true
- name: Rust cache
uses: Swatinem/rust-cache@v2

- name: Cargo install
if: matrix.rust_version == 'RUST_VERSION_NEW'
run: cargo install --locked cargo-sort cargo-udeps

- name: Rust version
id: rust_version_step
Expand All @@ -40,12 +53,17 @@ jobs:
echo "CARGO_INCREMENTAL=$CARGO_INCREMENTAL"
echo "::set-output name=version::$(rustc --version | cut -d ' ' -f 2)"

- name: Rust cache
uses: Swatinem/rust-cache@v2

- name: Rustfmt check
run: cargo fmt --check

- name: Linter
run: |
cargo clippy --workspace --all-targets --no-deps --exclude=ydb-grpc -- -D warnings

- name: Cargo sort check
if: matrix.rust_version == 'RUST_VERSION_NEW'
run: cargo sort --workspace --check

- name: Unused dependencies check
if: matrix.rust_version == 'RUST_VERSION_NEW'
run: cargo +nightly udeps --all-targets
35 changes: 22 additions & 13 deletions .github/workflows/rust-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ on:
env:
CARGO_TERM_COLOR: always
CARGO_REGISTRIES_CRATES_IO_PROTOCOL: sparse
RUST_VERSION_OLD: "1.68.0"
RUST_VERSION_NEW: "1.88.0"
RUST_VERSION_OLD: "1.82" # aka MSRV
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need rust newer, than 1.68?

It's ok to update an old version, when necessary, but the update should a reasonable justification.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because some of updated dependencies now require Rust 1.82
Probably I can downgrade some dependencies to keep MSRV 1.81 but not less.

Updates of dependencies which contains fixes for critical vulnerabilities reported by cargo audit requires at least 1.81.

RUST_VERSION_NEW: "stable"
Copy link
Member

Choose a reason for hiding this comment

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

"stable" for RUST_VERSION_NEW is bad for CI because it will be updated in background without commit. The new version is used not only for compilation (which should remain stable), but for linter too and linter from new version often causing it to break on older code.

It mean that if I want to fix a bug after new rust version released - I should to fix all linter errors as pre-requisite, or ignore it.

I prefer to use fixed versions for reproducible CI.

Copy link
Contributor Author

@albenik albenik Jul 10, 2025

Choose a reason for hiding this comment

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

Get it!
Will fix
What do you think about 3-version matrix in ci?

  • minimal supported aka RUST_VERSION_OLD
  • latest stable as version number aka RUST_VERSION_NEW
  • "stable" – to catch future rust releases bugs as soon as possible. This check can be optional and not block PRs.

Copy link
Member

Choose a reason for hiding this comment

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

third unblock stable - is ok, for early handle rusts bugs - nightly may be better.


jobs:
tests:
Expand All @@ -37,29 +37,38 @@ jobs:
runs-on: ubuntu-24.04

steps:
- name: Show YDB server version
run: docker ps; docker exec ydb /ydbd -V
- name: Checkout
uses: actions/checkout@v4
with:
submodules: true

- name: Install nightly rust
if: matrix.rust_version == 'RUST_VERSION_NEW'
uses: dtolnay/rust-toolchain@nightly
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need nightly toolchain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is obsolete code. Will drop it.
It used to execute cargo-udeps but I moved it to a separate workflow file

with:
components: clippy, rustfmt

- name: Install rust
uses: dtolnay/rust-toolchain@v1
- name: Install stable rust
uses: dtolnay/rust-toolchain@master # as described in https://github.com/dtolnay/rust-toolchain/blob/master/README.md?plain=1#L45
with:
toolchain: ${{ env[matrix.rust_version] }}
components: clippy, rustfmt

- name: Checkout
uses: actions/checkout@v4
- name: Rust cache
uses: Swatinem/rust-cache@v2
with:
submodules: true
key: ${{ env[matrix.rust_version] }}

- name: Show YDB server version
Copy link
Member

Choose a reason for hiding this comment

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

It's very convenient to see the server version at the beginning, and it's independent of the source code and Rust. Why did you move it?

Copy link
Contributor Author

@albenik albenik Jul 10, 2025

Choose a reason for hiding this comment

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

Because docker run more expensive operation than code checkout. About rust toolchain installation I am not sure by the way :)) But still: if you failed with very basic project setup it is a good idea not to execute anything more.

It's not important for me to stay at this point. If you say, that looking at YDB version as early as possible is required, I'll just rollback and make a comment in the code with such description.

Copy link
Member

@rekby rekby Jul 10, 2025

Choose a reason for hiding this comment

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

YDB run as service before. It starts before any step, the command show version only. It is fast. Faster, then install toolchain.

run: docker ps; docker exec ydb /ydbd -V

- name: Rust version
id: rust_version_step
run: |
rustc --version
cargo --version
echo "CARGO_INCREMENTAL=$CARGO_INCREMENTAL"
echo "::set-output name=version::$(rustc --version | cut -d ' ' -f 2)"

- name: Rust cache
uses: Swatinem/rust-cache@v2
echo "name=version::$(rustc --version | cut -d ' ' -f 2)" >> $GITHUB_OUTPUT

- name: Run tests
env:
Expand Down
65 changes: 65 additions & 0 deletions .github/workflows/security.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
name: Security

on:
push:
branches: [ master ]
pull_request:
branches: [ master ]

env:
CARGO_TERM_COLOR: always
CARGO_REGISTRIES_CRATES_IO_PROTOCOL: sparse
RUST_VERSION_NEW: "stable"
Copy link
Member

Choose a reason for hiding this comment

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

Almost all, upgraded with background is bad for CI, because in changes without commit and difficult to debug.


jobs:
tests:
strategy:
fail-fast: false
matrix:
rust_version: ["RUST_VERSION_NEW" ]
runs-on: ubuntu-24.04

steps:
- name: Checkout
uses: actions/checkout@v4
with:
submodules: true

- name: Install nightly rust
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need nightly toolchain?

if: matrix.rust_version == 'RUST_VERSION_NEW'
uses: dtolnay/rust-toolchain@nightly
with:
components: clippy, rustfmt

- name: Install stable rust
uses: dtolnay/rust-toolchain@master # as described in https://github.com/dtolnay/rust-toolchain/blob/master/README.md?plain=1#L45
with:
toolchain: ${{ env[matrix.rust_version] }}
components: clippy, rustfmt

- name: Rust cache
uses: Swatinem/rust-cache@v2
with:
key: ${{ env[matrix.rust_version] }}

- name: Rust version
id: rust_version_step
run: |
rustc --version
cargo --version
echo "CARGO_INCREMENTAL=$CARGO_INCREMENTAL"
echo "name=version::$(rustc --version | cut -d ' ' -f 2)" >> $GITHUB_OUTPUT
- name: Cargo install
if: matrix.rust_version == 'RUST_VERSION_NEW'
run: cargo install --locked cargo-audit cargo-pants

- name: Cargo audit
Copy link
Member

Choose a reason for hiding this comment

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

Great idea, thanks

if: matrix.rust_version == 'RUST_VERSION_NEW'
run: |
cargo audit
- name: Cargi pants
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- name: Cargi pants
- name: Cargo pants

if: matrix.rust_version == 'RUST_VERSION_NEW'
run: |
cargo pants
Copy link
Member

Choose a reason for hiding this comment

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

All files must have new line at the end.

Loading
Loading