Skip to content
This repository was archived by the owner on Apr 25, 2026. It is now read-only.

chore: add GA to publish tangle-subxt#1043

Merged
drewstone merged 2 commits into
mainfrom
1xstj/ci-subxt
Jul 31, 2025
Merged

chore: add GA to publish tangle-subxt#1043
drewstone merged 2 commits into
mainfrom
1xstj/ci-subxt

Conversation

@1xstj
Copy link
Copy Markdown
Contributor

@1xstj 1xstj commented Jul 31, 2025

Summary of changes
Changes introduced in this pull request:

Reference issue to close (if applicable)

Closes

@1xstj 1xstj marked this pull request as ready for review July 31, 2025 15:33
@1xstj 1xstj requested review from drewstone and shekohex as code owners July 31, 2025 15:33
@claude
Copy link
Copy Markdown

claude Bot commented Jul 31, 2025

Claude finished @1xstj's task —— View job


PR Review Analysis

Todo List:

  • Read repository's CLAUDE.md for repo-specific guidelines
  • Analyze the new GitHub Actions workflow file (.github/workflows/publish-subxt.yml)
  • Check repository structure and related configuration files for context
  • Review workflow for best practices, security, and performance issues
  • Post comprehensive review feedback via GitHub comment

Comprehensive Code Review

I've thoroughly analyzed the new GitHub Actions workflow for publishing tangle-subxt to crates.io. Here's my detailed feedback:

Positive Aspects

  1. Smart Version Detection: The workflow cleverly checks if the version already exists on crates.io before attempting to publish, preventing duplicate publication attempts.

  2. Conditional Execution: Proper use of conditional steps with if: steps.version-check.outputs.should_publish == 'true' prevents unnecessary operations.

  3. Appropriate Triggers:

    • Manual trigger via workflow_dispatch for flexibility
    • Automatic trigger on pushes to main branch with relevant path filters
  4. Rust Toolchain Setup: Uses the reliable dtolnay/rust-toolchain@stable action with proper caching via swatinem/rust-cache@v2.

🔧 Areas for Improvement

Security Considerations

  1. Line 58: Direct token usage in command line could potentially expose the token in logs:

    cargo publish --token $CARGO_REGISTRY_TOKEN

    Recommendation: Use cargo publish without the explicit token flag, as cargo will automatically use the CARGO_REGISTRY_TOKEN environment variable.

  2. Lines 66-67: Git configuration uses ${GITHUB_ACTOR} which could be manipulated in certain scenarios. Consider using a more controlled approach.

Best Practices & Reliability

  1. Lines 43-44: The version check logic relies on cargo search output parsing which could be brittle:

    if cargo search tangle-subxt --limit 1 | grep -q "tangle-subxt = \"$CURRENT_VERSION\""; then

    Issue: cargo search results may not immediately reflect recent publications due to caching.

    Recommendation: Use cargo info tangle-subxt instead, or add a small delay/retry mechanism.

  2. Line 39: Version extraction uses sed which could fail if Cargo.toml format changes:

    CURRENT_VERSION=$(grep '^version = ' tangle-subxt/Cargo.toml | sed 's/version = "\(.*\)"/\1/')

    Recommendation: Use cargo metadata for more reliable version extraction:

    CURRENT_VERSION=$(cargo metadata --no-deps --format-version 1 | jq -r '.packages[] | select(.name=="tangle-subxt") | .version')
  3. Missing Error Handling: The workflow lacks explicit error handling. If cargo publish fails, the git tag will still be created.

Performance & Efficiency

  1. Line 24: fetch-depth: 0 fetches the entire git history, which is unnecessary for this workflow.
    Recommendation: Remove this line or set to fetch-depth: 1 (default).

Documentation & Maintainability

  1. Missing Workflow Documentation: The workflow lacks comments explaining the logic flow and purpose of each step.

  2. Tag Naming Convention: Consider if tangle-subxt-v$VERSION is the desired format or if it should match existing repository tagging conventions.

🚨 Potential Issues

  1. Race Condition: If multiple commits change the version simultaneously, there could be race conditions in publication.

  2. Partial Failure State: If publishing succeeds but git tagging fails, there's no cleanup mechanism.

📋 Recommended Changes

# Improved version check step
- name: Check if version changed
  id: version-check
  run: |
    # Get the current version using cargo metadata (more reliable)
    CURRENT_VERSION=$(cargo metadata --no-deps --format-version 1 --manifest-path tangle-subxt/Cargo.toml | jq -r '.packages[] | select(.name=="tangle-subxt") | .version')
    echo "Current version: $CURRENT_VERSION"
    
    # Check if this version already exists on crates.io with retry
    for i in {1..3}; do
      if cargo info tangle-subxt 2>/dev/null | grep -q "^$CURRENT_VERSION"; then
        echo "Version $CURRENT_VERSION already exists on crates.io"
        echo "should_publish=false" >> $GITHUB_OUTPUT
        exit 0
      fi
      sleep 2
    done
    
    echo "Version $CURRENT_VERSION not found on crates.io, proceeding with publish"
    echo "should_publish=true" >> $GITHUB_OUTPUT
    echo "version=$CURRENT_VERSION" >> $GITHUB_OUTPUT

# Improved publish step
- name: Publish to crates.io
  if: steps.version-check.outputs.should_publish == 'true'
  env:
    CARGO_REGISTRY_TOKEN: ${{ secrets.CARGO_REGISTRY_TOKEN }}
  run: |
    cd tangle-subxt
    cargo publish  # Removed explicit token flag for security

🔍 Additional Considerations

  1. Testing: Consider adding a dry-run step (cargo publish --dry-run) before the actual publish.

  2. Notifications: Consider adding failure notifications for monitoring.

  3. Consistency: Compare with existing publish workflows in the repository (like publish-types.yml) for consistency in patterns and practices.

📊 Overall Assessment

This workflow is functionally sound and follows many GitHub Actions best practices. The main concerns are around security (token handling), reliability (version checking), and error handling. The suggested improvements would make it more robust and maintainable.

Recommendation: ✅ Approve with suggested improvements


@drewstone drewstone merged commit 670f207 into main Jul 31, 2025
6 checks passed
@drewstone drewstone deleted the 1xstj/ci-subxt branch July 31, 2025 16:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants