Improve lint job performance - #394
Conversation
Signed-off-by: currantw <taylor.curran@improving.com>
…olution Signed-off-by: currantw <taylor.curran@improving.com>
Signed-off-by: currantw <taylor.curran@improving.com>
Signed-off-by: currantw <taylor.curran@improving.com>
Signed-off-by: currantw <taylor.curran@improving.com>
…hash expiry HashFieldGetAndSetExpiryAsync_MultiField and HashFieldSetAndSetExpiryAsync_MultiField were calling the same TimeSpan-based overload twice. The second assertion now calls the DateTime-based overload to cover both signatures. Signed-off-by: currantw <taylor.curran@improving.com>
Signed-off-by: currantw <taylor.curran@improving.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughWorkflows/actions switch Task installation to pinned go-task/setup-task; lint workflow timeout, Rust setup/cache, and tool installs are reworked; Taskfile prettier adds --cache; two integration tests updated to call explicit-expiry overloads alongside UnsupportedFlag. ChangesCI/Build Infrastructure Modernization
Hash CommandFlags Test Updates
Sequence DiagramsequenceDiagram
participant Runner as GitHub Runner
participant SetupAction as go-task/setup-task
participant TaskCLI as task (CLI)
Runner->>SetupAction: checkout pinned action (v2.0.0 SHA)
SetupAction->>TaskCLI: provision `task` binary on runner
Runner->>TaskCLI: run subsequent `task` commands (e.g., `task lint`, `task coverage`)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/lint.yml (1)
47-47: ⚡ Quick winAdd explicit Task CLI version input to
go-task/setup-taskfor reproducible CI behavior.Line 47 uses
go-task/setup-taskpinned to action commitdc4f00a24b1b0b5e1a9265f6f4e81d65c0862414(v2.0.0), but does not specify an explicit Task CLI version viawith.version. The action defaults to3.x, allowing the installed Task version to drift over time. For better reproducibility, add thewith.versioninput with an exact version number. This concern applies to multiple workflow files in this PR (also present in.github/workflows/check-examples.yml).🔧 Suggested change
- name: Install Task uses: go-task/setup-task@dc4f00a24b1b0b5e1a9265f6f4e81d65c0862414 # v2.0.0 + with: + version: "3.x"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/lint.yml at line 47, The workflow step using the GitHub Action reference "uses: go-task/setup-task@dc4f00a24b1b0b5e1a9265f6f4e81d65c0862414" should explicitly pin the Task CLI by adding a "with.version" input (e.g., a specific semantic version like "3.11.0") so the installed Task binary is reproducible; update this step in .github/workflows/lint.yml (and the same step in .github/workflows/check-examples.yml) to include the with.version key with an exact version string.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/lint.yml:
- Line 47: The workflow step using the GitHub Action reference "uses:
go-task/setup-task@dc4f00a24b1b0b5e1a9265f6f4e81d65c0862414" should explicitly
pin the Task CLI by adding a "with.version" input (e.g., a specific semantic
version like "3.11.0") so the installed Task binary is reproducible; update this
step in .github/workflows/lint.yml (and the same step in
.github/workflows/check-examples.yml) to include the with.version key with an
exact version string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 32888c80-f2c5-43e6-be81-872ec1c0a82c
📒 Files selected for processing (5)
.github/actions/run-coverage/action.yml.github/workflows/check-examples.yml.github/workflows/lint.ymlTaskfile.ymltests/Valkey.Glide.IntegrationTests/StackExchange/CommandFlagsTests.cs
There was a problem hiding this comment.
Pull request overview
This PR aims to speed up CI linting by switching to faster tool installation methods (prebuilt binaries / official setup actions), enabling caching where useful, and tightening the lint job timeout, while also adjusting an integration test to cover the intended multi-field expiry overload.
Changes:
- Update Rust/YAML lint commands (
cargo clippy --locked,prettier --cache) to reduce repeated work. - Optimize GitHub Actions lint workflow by using
taiki-e/install-actionfor tool installs andgo-task/setup-taskfor Task installation; reduce lint timeout to 10 minutes. - Fix integration tests to exercise the correct multi-field expiry overload when asserting
CommandFlagsbehavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Valkey.Glide.IntegrationTests/StackExchange/CommandFlagsTests.cs | Adjusts NotImplementedException assertions to call the intended overloads for multi-field expiry methods. |
| Taskfile.yml | Updates Rust clippy invocation and enables Prettier caching for YAML formatting. |
| .github/workflows/lint.yml | Speeds up lint job via prebuilt tool installs, Task setup action, caching tweaks, and reduced timeout. |
| .github/workflows/check-examples.yml | Switches Task installation to go-task/setup-task. |
| .github/actions/run-coverage/action.yml | Switches Task installation to go-task/setup-task inside the composite action. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Revert --locked on cargo clippy (Cargo.lock is gitignored) - Improve Rust cache key to invalidate when Cargo.toml changes - Pin actionlint to v1.7.12 for reproducible lint results Signed-off-by: currantw <taylor.curran@improving.com>
Signed-off-by: currantw <taylor.curran@improving.com>
actionlint is not in taiki-e's supported tools manifest, so version pinning fails. Use direct curl download instead (pinned to v1.7.12). Signed-off-by: currantw <taylor.curran@improving.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/lint.yml:
- Around line 39-41: The workflow currently installs cargo-deny without a
version pin (the with: tool: cargo-deny input for
taiki-e/install-action@735e5933943122c5ac182670a935f54a949265c1); update that
input to pin cargo-deny to a specific release by changing the tool value to
include an `@version` suffix (e.g., tool: cargo-deny@<SPECIFIC_VERSION>) so CI
uses a deterministic, tested cargo-deny release.
- Line 44: Replace the unverified pipe of the remote tarball with a
download-and-verify flow: fetch actionlint_1.7.12_linux_amd64.tar.gz and its
published checksum (or SHA256 string you add as a trusted constant), verify the
tarball against the checksum (e.g., with sha256sum or openssl dgst) and only
then extract and move actionlint into /usr/local/bin; update the workflow step
that currently runs the inline curl|tar command to perform these download,
verification and conditional-extract steps so the binary is only installed if
the checksum matches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7e52a48c-2aeb-4489-b956-08bf4119d338
📒 Files selected for processing (1)
.github/workflows/lint.yml
The advisory database now contains CVSS 4.0 entries which older cargo-deny versions cannot parse. Signed-off-by: currantw <taylor.curran@improving.com>
… into currantw/134_improve_lint_performance
… into currantw/134_improve_lint_performance
Summary
Improve lint job performance by replacing slow tool installations with pre-built binary downloads and adding caching where applicable.
Issue Link
Closes #134.
Features and Behaviour Changes
cargo clippyuses--lockedto skip registry index updates.prettier --writeuses--cachefor faster local formatting iterations.Implementation
taiki-e/install-action: Replacescargo install --locked cargo-denyand the manualactionlintcurl/tar download with a single action step that downloads pre-built binaries.go-task/setup-task@v2: Replaces the curl-based Task installation script across all workflows (lint.yml,check-examples.yml,run-coverage/action.yml) with the official GitHub Action from the Task team.--lockedon clippy: Prevents Cargo from updating the registry index during lint, saving time on dependency resolution.--cacheon prettier: Enables prettier's built-in file hash cache for localformat:yamlruns so only changed files are re-checked.Limitations
⚪ None.
Testing
task lint:yamlandtask lint:actionspass locally with the updated workflow files.Related Issues
Checklist
CHANGELOG.md,README.md,DEVELOPER.md, and other documentation files are updated.mainor releasemain, squash otherwise.Summary by CodeRabbit
Chores
Tests