Skip to content

Add release workflow for PyPI publishing - #18

Merged
agileguy merged 2 commits into
mainfrom
add-release-workflow
Feb 7, 2026
Merged

agileguy merged 2 commits into
mainfrom
add-release-workflow

Conversation

@agileguy

@agileguy agileguy commented Feb 7, 2026

Copy link
Copy Markdown
Owner

Adds GitHub Actions workflow to publish to PyPI on version tags (v*).

@claude

claude Bot commented Feb 7, 2026

Copy link
Copy Markdown

Pull Request Review: Release Workflow for PyPI Publishing

I've reviewed PR #18 and here's my comprehensive feedback:


✅ Strengths

  1. Good Security Practices

    • Uses trusted PyPI publishing with id-token: write permission
    • Properly configured OIDC for PyPI (via TWINE_USERNAME: token)
    • Follows GitHub Actions best practices with specific permissions
  2. Comprehensive Quality Gates

    • Runs linting, type checking, and tests before publishing
    • Verifies build artifacts with twine check
    • Creates GitHub release with auto-generated notes
  3. Clear Workflow Structure

    • Well-organized steps with descriptive names
    • Good use of output variables for version extraction
    • Helpful verification messages at the end

⚠️ Issues & Recommendations

1. CRITICAL: Missing PyPI Token Configuration

The workflow references secrets.PYPI_TOKEN but you're using OIDC authentication. This is a mismatch.

Option A - Use PyPI Trusted Publishing (Recommended):
Replace the manual twine upload with:

- name: Publish to PyPI
  uses: pypa/gh-action-pypi-publish@release/v1
  with:
    print-hash: true

This requires configuring Trusted Publisher on PyPI (pypi.org/manage/account/publishing/) - no secrets needed!

Option B - Keep manual token approach:
Create API token on PyPI, add as repository secret named PYPI_TOKEN, keep current code (but Option A is more secure)

2. Missing Version Validation

The workflow doesn't verify that the tag version matches VERSION file. Add a step to prevent accidentally pushing v0.8.1 tag when VERSION still says 0.8.0.

3. Linting Only Checks src/

Line 30 runs: ruff check src/

Should be: ruff check src/ tests/

(To match CI workflow and CLAUDE.md requirements)

4. No Coverage Check

The CI workflow requires 65% coverage, but release workflow doesn't verify this. Consider adding coverage check before release.

5. No Rollback Plan

If PyPI upload fails, the GitHub release is already created. Consider creating the release as draft first, then publishing it after successful PyPI upload.

6. Missing Test Uploads to TestPyPI

Best practice: test the upload process with TestPyPI first before publishing to production PyPI.


🔒 Security Considerations

  1. ✅ Good: Uses pinned action versions (@v4, @v5, @v1)
  2. ✅ Good: Minimal permissions granted
  3. ⚠️ Consider: Add --require-hashes for reproducible builds (advanced)

📚 Documentation Gaps

Per CLAUDE.md, this PR should update:

  1. README.md - Add section on:

    • Installing from PyPI: pip install logview
    • Release process
    • Version tagging conventions
  2. ACTIONS.md - Document this change with files changed and what was added

  3. CLAUDE.md - Update semantic versioning section with how to trigger releases


📋 Pre-Merge Checklist

Must fix before merge:

  • Fix PyPI publishing method (Option A or B above)
  • Add version validation step
  • Fix linting to include tests/

Should fix:

  • Add coverage check
  • Update README.md with installation instructions
  • Update ACTIONS.md with this change

Nice to have:

  • TestPyPI uploads
  • Draft-first release approach

🎯 Priority Fixes

The main issue is the PyPI authentication configuration - this will fail on first run. Once that's fixed and docs are updated (per CLAUDE.md requirements), this will be ready to merge.

Overall this is a solid foundation for automated releases!

@claude

claude Bot commented Feb 7, 2026

Copy link
Copy Markdown

PR Review: Add Release Workflow for PyPI Publishing

Thank you for adding the PyPI release automation! This is a solid foundation, but I've identified several important issues and improvements needed before merging.


Critical Issues

1. Missing PyPI Secret Configuration
The workflow references secrets.PYPI_TOKEN but this secret may not be configured. Before merging:

2. No Dry-Run or Test PyPI Support
The workflow jumps straight to production PyPI. Consider adding a TestPyPI step for validation first.

3. Linting Runs on Subset of Files
Line 30: ruff check src/ should be ruff check src/ tests/ to match CI and CLAUDE.md guidelines

4. Missing Version Validation
The workflow doesn't verify that the git tag version matches the VERSION file. Add validation before building.


High Priority Issues

5. Incomplete README Update
The README removes installation script docs but doesn't clearly explain the new PyPI-based flow.

6. No Workflow Testing
This workflow has never been tested. Test on a fork or with a test tag (v0.8.0-rc1) first.

7. Missing Build Artifact Upload
Upload dist/ as GitHub artifacts for debugging and backup.


Medium Priority Issues

8. Duplicate Quality Checks
Reruns lint/type/test that already ran in CI. Consider skipping or adding CI status check.

9. No Changelog Validation
Per CLAUDE.md, CHANGELOG.md should be updated. Add validation to verify this.

10. Missing Workflow Permissions Documentation
Add comments explaining what contents: write and id-token: write are for.

11. Hardcoded Python Version
Uses Python 3.12 but CI tests 3.11+3.12. Document why 3.12 for builds.


Best Practices

12. Consider Trusted Publishing (OIDC)
PyPI supports Trusted Publishing - more secure, no tokens needed. Already has id-token: write.

13. Add Release Checklist
Include pre-merge checklist in PR description.

14. Notification on Failure
Add notification step if release fails.


What's Good

  • Comprehensive quality checks ✓
  • GitHub release creation with auto-notes ✓
  • Build verification with twine check ✓
  • Clear naming ✓
  • Least-privilege permissions ✓
  • README updates with PyPI badges ✓

Recommended Action Plan

Before merging:

  1. Add PYPI_TOKEN secret to repository
  2. Add version validation step
  3. Fix linting command to include tests/
  4. Test workflow on test tag
  5. Add build artifact upload
  6. Add CHANGELOG validation

Nice to have:

  • TestPyPI step
  • Failure notifications
  • Release checklist

After merging:

  1. Monitor first release (v0.8.0) closely
  2. Consider Trusted Publishing migration
  3. Document release process

Security

✅ Good: OIDC permissions, no hardcoded creds, twine validation
⚠️ Watch: Use scoped token, consider environment protection, TestPyPI prevents bad releases

@agileguy
agileguy merged commit 07a79fc into main Feb 7, 2026
7 checks passed
@agileguy
agileguy deleted the add-release-workflow branch February 7, 2026 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant