Conversation
Co-authored-by: harsha-simhadri <5590673+harsha-simhadri@users.noreply.github.com>
Co-authored-by: harsha-simhadri <5590673+harsha-simhadri@users.noreply.github.com>
Co-authored-by: harsha-simhadri <5590673+harsha-simhadri@users.noreply.github.com>
Co-authored-by: harsha-simhadri <5590673+harsha-simhadri@users.noreply.github.com>
Co-authored-by: harsha-simhadri <5590673+harsha-simhadri@users.noreply.github.com>
harsha-simhadri
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #755 +/- ##
========================================
Coverage 88.99% 89.00%
========================================
Files 428 428
Lines 78234 78417 +183
========================================
+ Hits 69626 69793 +167
- Misses 8608 8624 +16
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds an automated crate publishing workflow for releasing DiskANN crates to crates.io. The workflow can be triggered manually via workflow_dispatch with a dry-run option (default: true) for testing, or automatically when pushing version tags matching v{major}.{minor}.{patch}. The implementation aims to enable safe testing of the release process without actual publication.
Changes:
- Added
.github/workflows/publish.ymlworkflow with dual-trigger support (tags and manual dispatch) - Added
.github/PUBLISH_CRATES.mddocumentation describing the release process and testing procedures - Implemented dry-run mode for testing packaging and validation without publishing
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
.github/workflows/publish.yml |
New GitHub Actions workflow that validates versions, runs tests, and publishes workspace crates with optional dry-run mode |
.github/PUBLISH_CRATES.md |
Comprehensive documentation covering prerequisites, dry-run testing, release steps, and pre-release checklist |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| DRY_RUN_FLAG="--dry-run" | ||
| echo "🧪 DRY-RUN MODE" | ||
| fi | ||
| cargo publish --locked --workspace $DRY_RUN_FLAG |
There was a problem hiding this comment.
The cargo publish command does not support a --workspace flag. As of Rust 1.92 (and as of January 2025), cargo publish only publishes a single crate at a time. To publish multiple workspace crates, you need to either:
- Use a loop to publish each crate individually in dependency order
- Use a tool like
cargo-workspacesorcargo-release - Manually specify the publish order
The current command will fail with an error. You'll need to implement a solution that publishes crates one at a time, ensuring dependencies are published and indexed before their dependents.
| cargo publish --locked --workspace $DRY_RUN_FLAG | |
| echo "Resolving workspace crates to publish..." | |
| PKGS=$(cargo metadata --no-deps --format-version 1 | jq -r '.packages[] | select(.source == null) | .name') | |
| if [ -z "$PKGS" ]; then | |
| echo "::error::No workspace crates found to publish" | |
| exit 1 | |
| fi | |
| for pkg in $PKGS; do | |
| echo "📦 Publishing crate: $pkg (DRY_RUN_FLAG='$DRY_RUN_FLAG')" | |
| cargo publish --locked -p "$pkg" $DRY_RUN_FLAG | |
| # When actually publishing, wait a bit for crates.io index to update | |
| if [ -z "$DRY_RUN_FLAG" ]; then | |
| echo "Waiting for crates.io index to update before publishing the next crate..." | |
| sleep 30 | |
| fi | |
| done |
There was a problem hiding this comment.
This is not accurate. There is a cargo publish --workspace and it does work. It doesn't necessarily handle partial failures gracefully, but hopefully those are relatively rare.
|
|
||
| ## Overview | ||
|
|
||
| All workspace crates are published together with synchronized version numbers using `cargo publish --workspace`, which automatically resolves dependency order and waits for each crate to be indexed before publishing its dependents. The release is triggered by pushing a version tag. The Rust toolchain version is read from [`rust-toolchain.toml`](../../rust-toolchain.toml). |
There was a problem hiding this comment.
The documentation states that cargo publish --workspace "automatically resolves dependency order and waits for each crate to be indexed before publishing its dependents," but this is not accurate. The cargo publish command does not have a --workspace flag and does not automatically handle dependency ordering or index polling. This documentation needs to be updated to reflect the actual implementation approach.
| DRY_RUN_FLAG="" | ||
| if [ "${{ github.event.inputs.dry_run || 'false' }}" = "true" ]; then | ||
| DRY_RUN_FLAG="--dry-run" | ||
| echo "🧪 DRY-RUN MODE" |
There was a problem hiding this comment.
When not in dry-run mode (live publishing), there's no visual indicator in the logs. Consider adding an else clause to echo a message like "📦 LIVE MODE - Publishing to crates.io" to make it clear when actual publishing is happening. This helps prevent accidental publishes and makes logs easier to understand.
| echo "🧪 DRY-RUN MODE" | |
| echo "🧪 DRY-RUN MODE" | |
| else | |
| echo "📦 LIVE MODE - Publishing to crates.io" |
| if: github.event_name == 'push' | ||
| run: | | ||
| TAG_VERSION="${GITHUB_REF#refs/tags/v}" | ||
| CARGO_VERSION=$(grep '^version = ' Cargo.toml | head -n1 | sed 's/.*"\(.*\)".*/\1/') |
There was a problem hiding this comment.
The version extraction uses grep '^version = ' Cargo.toml | head -n1 which assumes the workspace version is the first version field in the file. However, looking at the root Cargo.toml structure, the version is under [workspace.package] section. A more robust approach would be to use grep -A 5 '^\[workspace\.package\]' Cargo.toml | grep 'version = ' or use a proper TOML parser to avoid potential mismatches if the file structure changes.
| CARGO_VERSION=$(grep '^version = ' Cargo.toml | head -n1 | sed 's/.*"\(.*\)".*/\1/') | |
| CARGO_VERSION=$(grep -A 5 '^\[workspace\.package\]' Cargo.toml | grep 'version = ' | head -n1 | sed 's/.*"\(.*\)".*/\1/') |
| ### Option 2: Local | ||
|
|
||
| ```bash | ||
| cargo publish --locked --workspace --dry-run |
There was a problem hiding this comment.
The command example shows cargo publish --locked --workspace --dry-run, but as noted, cargo publish does not support --workspace. This example needs to be updated to reflect the actual implementation.
| 3. Watch the workflow — look for successful `cargo publish --workspace --dry-run` | ||
|
|
||
| ### Option 2: Local | ||
|
|
||
| ```bash | ||
| cargo publish --locked --workspace --dry-run |
There was a problem hiding this comment.
The documentation mentions "look for successful cargo publish --workspace --dry-run" but this command format is invalid. Update this to reflect the actual command that will be used.
| 3. Watch the workflow — look for successful `cargo publish --workspace --dry-run` | |
| ### Option 2: Local | |
| ```bash | |
| cargo publish --locked --workspace --dry-run | |
| 3. Watch the workflow — look for successful `cargo publish --locked --dry-run` | |
| ### Option 2: Local | |
| ```bash | |
| cargo publish --locked --dry-run |
| - name: Verify version matches tag | ||
| if: github.event_name == 'push' | ||
| run: | | ||
| TAG_VERSION="${GITHUB_REF#refs/tags/v}" | ||
| CARGO_VERSION=$(grep '^version = ' Cargo.toml | head -n1 | sed 's/.*"\(.*\)".*/\1/') | ||
| if [ "$TAG_VERSION" != "$CARGO_VERSION" ]; then | ||
| echo "::error::Tag version ($TAG_VERSION) does not match Cargo.toml version ($CARGO_VERSION)" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Consider adding a validation step to ensure that all workspace crates use version.workspace = true and don't have hardcoded versions. This would catch configuration errors before attempting to publish. A simple check like grep -r '^version = "' */Cargo.toml | grep -v workspace could identify crates with hardcoded versions.
| exit 1 | ||
| fi | ||
|
|
||
| - name: Run tests |
There was a problem hiding this comment.
Running the full test suite on every publish attempt (including dry-run) adds significant time to the workflow. Consider making the test step conditional with if: github.event.inputs.dry_run != 'true' || github.event_name == 'push' to skip tests during dry-run testing, since tests are already validated in PR CI checks before merging. However, keeping tests for actual releases (tag pushes) is valuable as a final safety check.
| - name: Run tests | |
| - name: Run tests | |
| if: github.event.inputs.dry_run != 'true' || github.event_name == 'push' |
| ### What Dry-Run Tests | ||
|
|
||
| - Crate metadata and packaging validation | ||
| - Dependency resolution and publish ordering |
There was a problem hiding this comment.
The claim that dry-run tests "Dependency resolution and publish ordering" is misleading. Since workspace-wide publishing isn't supported by cargo publish, the actual implementation will need to determine publish order separately. A dry-run of a single crate publish won't validate that all crates will be published in the correct dependency order.
| CARGO_REGISTRY_TOKEN: ${{ secrets.CRATES_IO_TOKEN }} | ||
| run: | | ||
| DRY_RUN_FLAG="" | ||
| if [ "${{ github.event.inputs.dry_run || 'false' }}" = "true" ]; then |
There was a problem hiding this comment.
The dry-run default expression github.event.inputs.dry_run || 'false' will evaluate to 'false' when the workflow is triggered by a tag push (since github.event.inputs.dry_run will be empty/null). This is correct behavior for tag-triggered publishes. However, consider making this more explicit with a comment or using a more readable approach like setting a workflow-level environment variable that checks both the trigger type and input.
| if [ "${{ github.event.inputs.dry_run || 'false' }}" = "true" ]; then | |
| # For tag-push events, github.event.inputs.dry_run is empty, so this evaluates to a real (non-dry-run) publish. | |
| if [ "${{ github.event.inputs.dry_run }}" = "true" ]; then |
|
|
||
| - name: Publish workspace crates | ||
| env: | ||
| CARGO_REGISTRY_TOKEN: ${{ secrets.CRATES_IO_TOKEN }} |
There was a problem hiding this comment.
Vermy minor - this can be slightly safer with
env:
DRY_RUN: ${{ github.event.inputs.dry_run || 'false ' }}And then using DRY_RUN to set the DRY_RUN_FLAG internally rather than expanding the entire github.event directly in the shell script. Evaluating it in env should force it to be a boolean as an extra safety measure.
| - name: Read Rust version from rust-toolchain.toml | ||
| id: rust-version | ||
| run: | | ||
| RUST_VERSION=$(sed -n 's/^channel = "\(.*\)"/\1/p' rust-toolchain.toml) |
There was a problem hiding this comment.
Reading from rust-toolchain.toml is a nice trick - we should update ci.yml to do the same thing (though probably in a different PR).
| cargo search diskann --limit 20 | ||
| ``` | ||
|
|
||
| ### Example Pre-Release Flow |
There was a problem hiding this comment.
This recommends pushing directly to main, which we can't do due to branch protection rules.
Instead, we should use this workflow:
- Checkout main, bump the version and push.
- Make a pull-request out of the version bumped commit.
- Use the dry-run as a pre-merge check.
- Merge the PR and use the github UI to tag the release along with change notes.
|
|
||
| - name: Verify version matches tag | ||
| if: github.event_name == 'push' | ||
| run: | |
There was a problem hiding this comment.
One thing I noticed while adding another comment is that since manual running takes dry-run as a parameter - it's currently possible to publish without a dry run from branches that aren't main.
Copilot suggest putting something like this early in the workflow
- name: Prevent publish from non-main branch
if: >-
github.event_name == 'workflow_dispatch'
&& github.event.inputs.dry_run != 'true'
&& github.ref != 'refs/heads/main'
run: |
echo "Live publishing is only allowed from main. Use dry-run for other branches."
exit 1which I think is reasonable. It will fail the triggered manually on a branch that is not main and dry-run is not provided.
|
|
||
| ### What It Does NOT Test | ||
|
|
||
| - Actual publishing, registry token auth, upload reliability |
There was a problem hiding this comment.
We may want to mention what to do if the publish step fails. Basically, we either need to manually fix and publish the remaining crates, or fix the publish issue, bump the version number, and bump again (or if the fix doesn't change the versions that succeeded, then the remaining ones can be updated and pushed potentially in the same release).
If the publishing fails part way through for networking issues, then we'll need to handle the remaining ones manually.
The automated publish workflow could only be triggered by pushing version tags, making it impossible to test changes to the workflow or validate a release without actually publishing to crates.io.
Changes
Workflow trigger
workflow_dispatchwithdry_runinput (defaults totrue)Publish logic
--dry-runflag tocargo publishwhen enabled🧪 DRY-RUN MODEvs📦 LIVE MODEDocumentation
RELEASING.md: Testing section with dry-run vs live comparison.github/TESTING_RELEASES.md: Step-by-step testing guideUsage
Manual test without publishing:
Actual publish (unchanged):
git tag v0.46.0 && git push origin v0.46.0Dry-run validates packaging, dependencies, and publish order across all 15 crates without uploading to the registry.
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.