hubcio commented on code in PR #3749: URL: https://github.com/apache/iggy/pull/3749#discussion_r3689099620
########## .github/workflows/_test_rust.yml: ########## @@ -0,0 +1,125 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +name: _test_rust +on: + workflow_call: + inputs: + component: + type: string + required: true + description: "Rust component to test" + task: + type: string + required: true + description: "Rust task to run" + secrets: + CODECOV_TOKEN: + required: false + +permissions: + contents: read + security-events: write + pull-requests: write + +jobs: + run: + if: inputs.task != 'test' + uses: ./.github/workflows/_test.yml + with: + component: ${{ inputs.component }} + task: ${{ inputs.task }} + secrets: + CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} + + build-rust-tests: + if: inputs.task == 'test' Review Comment: this cuts the noise a lot but doesn't remove it. the previous run had 85 `*-rust-tests` rows, 82 of them skipped, across 41 callers. now that only `test-rust` reaches these jobs the SDK callers drop to zero, but each of the eleven non-`test` rust tasks still materialises a skipped `build-rust-tests` and a skipped `run-rust-tests`, so about 22 skipped rows remain plus one for the skipped `run` forward on `rust/test`. to get to zero the split has to move a level up - have `_detect.yml` emit the `test` task separately from the rest of `rust_matrix`, then this workflow holds only the build and partition jobs and never needs the `run` forward at all. not required here, but that's the shape that actually makes the check list clean. ########## .github/actions/rust/pre-merge/action.yml: ########## @@ -187,44 +195,30 @@ runs: shell: bash - name: Install dependencies for Rust tests - if: startsWith(inputs.task, 'test-') && runner.os == 'Linux' + if: startsWith(inputs.task, 'test-run-') && runner.os == 'Linux' run: | - sudo apt-get install --yes musl-tools gnome-keyring keyutils dbus-x11 libsecret-tools + sudo apt-get install --yes gnome-keyring keyutils dbus-x11 libsecret-tools rm -f $HOME/.local/share/keyrings/* shell: bash - name: Install cargo-llvm-cov - if: startsWith(inputs.task, 'test-') + if: inputs.task == 'test-build' || startsWith(inputs.task, 'test-run-') uses: taiki-e/install-action@v2 with: - tool: cargo-llvm-cov + tool: [email protected] Review Comment: 0.8.7 is the current release, and the unpinned install was already resolving to it - see the cargo-llvm-cov install log on job 89212757735. pinning is the right move, but this pins one behind what CI was actually running. unless 0.8.6 was picked for a reason, bump it. ########## .github/actions/rust/pre-merge/action.yml: ########## @@ -398,3 +509,27 @@ runs: if: inputs.task == 'build-windows-sdk' run: cargo build --locked -p iggy -p iggy-cli shell: bash + + - name: Validate Rust task + if: always() + run: | + TASK="${{ inputs.task }}" + case "$TASK" in + check|check-msrv|fmt|clippy|sort|machete|doctest|verify-publish) Review Comment: `build` is missing from this allowlist, and it's a live task. `rust-bench-dashboard` declares `tasks: ["build"]`, `_detect.yml` only routes the exact name `rust` into `rust_matrix` so it falls into `other_matrix`, and `_test.yml`'s "Run Rust task" step still gates on `startsWith(inputs.component, 'rust')` - so this composite really does get invoked with `task: build`. see run 30009212161 job 89212758174: `Other • rust-bench-dashboard/build / run` passed with every task step skipped. after this change it exits 1, `test-other` goes red and `finalize_pr` blocks. it won't show up on this PR because it doesn't touch `core/bench/dashboard/**`. the `_test_rust.yml` split doesn't affect this path - `rust-bench-dashboard` never went through `test-rust`. the check is doing its job - that task has never built anything, and nothing is lost by deleting it. the dashboard crates are already compiled by the `rust` component: `rust-bench` matches `core/bench/**` and `rust` `depends_on` it, so any `core/bench/dashboard/**` change pulls in the whole rust task set through dependency resolution, `build-aarch64-gnu` included. the only path `rust-bench-dashboard` owns alone is `scripts/dashboard/**`, two shell scripts with nothing to compile. so either add `build` here to keep today's behaviour, or drop the task from `components.yml` and let the component stay path-trigger-only like the other `rust-*` ones. ########## .github/workflows/_test_rust.yml: ########## @@ -0,0 +1,125 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +name: _test_rust +on: + workflow_call: + inputs: + component: + type: string + required: true + description: "Rust component to test" + task: + type: string + required: true + description: "Rust task to run" + secrets: + CODECOV_TOKEN: + required: false + +permissions: + contents: read + security-events: write + pull-requests: write + +jobs: + run: + if: inputs.task != 'test' + uses: ./.github/workflows/_test.yml + with: + component: ${{ inputs.component }} + task: ${{ inputs.task }} + secrets: + CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} + + build-rust-tests: + if: inputs.task == 'test' + runs-on: ubuntu-latest + timeout-minutes: 60 + steps: + - name: Checkout code + uses: actions/[email protected] Review Comment: also at line 96. `actions/[email protected]` on both new jobs while `_test.yml:51` is already on `v7.0.1` (dependabot #3772, which is in the merge base). worth bumping these two by hand. ########## core/integration/tests/server/http_tls.rs: ########## @@ -40,11 +40,21 @@ const READY_RETRY_INTERVAL: Duration = Duration::from_millis(50); const REQUEST_TIMEOUT: Duration = Duration::from_secs(30); /// Absolute path to a repo loopback cert asset. The spawned server's CWD is a -/// temp dir, so relative paths break; `CARGO_MANIFEST_DIR` is the integration -/// crate, whose sibling `../certs` holds the checked-in loopback material. +/// temp dir, so relative paths break; nextest remaps the runtime +/// `CARGO_MANIFEST_DIR` to the integration crate, whose sibling `../certs` +/// holds the checked-in loopback material. fn cert_asset(file: &str) -> PathBuf { - std::fs::canonicalize(format!("{}/../certs/{file}", env!("CARGO_MANIFEST_DIR"))) - .unwrap_or_else(|error| panic!("canonicalize repo cert asset {file}: {error}")) + let manifest_dir = std::env::var_os("CARGO_MANIFEST_DIR") Review Comment: worth knowing this one isn't covered by pre-merge. `server/mod.rs` gates `mod http_tls` behind `#[cfg(feature = "vsr")]` and `vsr` isn't in the integration crate's default features, and the archive is built without `--all-features`, so these tests only get compiled by the `clippy --all-features` leg and never executed. the runtime `CARGO_MANIFEST_DIR` read is fine - `harness::context::tests::test_context_paths` exercises the same mechanism under `--workspace-remap` and passes - but the cert path itself is unverified by CI. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
