infra: pypi trusted publishing ci#245
infra: pypi trusted publishing ci#245ParticularlyPythonicBS wants to merge 1 commit intounstablefrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdds a publish GitHub Actions workflow triggered by tag pushes, bumps the package Changes
Sequence Diagram(s)sequenceDiagram
participant Tag as "Git Tag (v*)"
participant GH as "GitHub Actions"
participant Runner as "Runner (ubuntu-latest)"
participant PyPI as "PyPI (uv publish)"
Tag->>GH: push tag triggers workflow
GH->>Runner: start job (pypi env)
Runner->>Runner: checkout repo\ninstall setup-uv\nsetup Python 3.12\nuv build
Runner->>Runner: run tests/smoke_test.py (wheel & sdist)
Runner->>PyPI: uv publish (uses token)
PyPI-->>Runner: publish result
Runner-->>GH: job completes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ParticularlyPythonicBS
force-pushed
the
infra/publish_ci
branch
4 times, most recently
from
e261834 to
e10e0f9
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI Agents
In @.github/workflows/publish.yml:
- Around line 31-32: Add a pre-publish verification step before the "Publish"
run that extracts the tag version from GITHUB_REF (e.g. strip "refs/tags/v"),
reads the package version by running python -c "import temoa;
print(temoa.__version__)", compares the two, and fails the job (exit 1) with a
clear error if they differ; if they match, allow the workflow to continue to the
existing uv publish step and emit a success message.
- Around line 27-30: Add a timeout to the two smoke test workflow steps ("Smoke
test (wheel)" and "Smoke test (source distribution)") so a hung CLI won't stall
the run; update each job step to include a timeout-minutes (e.g.,
timeout-minutes: 10) alongside the existing name and run keys to ensure the
subprocess in tests/smoke_test.py is bounded.
In @tests/smoke_test.py:
- Around line 17-21: The test_help function uses subprocess.run without a
timeout which can hang; update the subprocess.run call in test_help to include a
timeout (match the value used in test_cli, e.g., timeout=10) and optionally
catch subprocess.TimeoutExpired to fail the test with a clear message; target
the subprocess.run invocation inside test_help.
- Around line 5-7: The test_import test currently only asserts temoa.__version__
is not None; strengthen it to ensure temoa.__version__ is a non-empty string by
checking its type and that its length is > 0 (e.g., assert
isinstance(temoa.__version__, str) and assert temoa.__version__.strip() != ""),
referencing the test_import function and temoa.__version__ to locate and update
the assertion.
- Around line 9-15: The subprocess.run call in test_cli can hang indefinitely;
add a timeout (e.g., timeout=10) to the subprocess.run invocation in the
test_cli function and handle subprocess.TimeoutExpired by failing the test
(raise or pytest.fail) with a clear message; update the call from
subprocess.run([...], capture_output=True, text=True) to include timeout=10 and
wrap it in try/except subprocess.TimeoutExpired to provide a deterministic test
failure if the CLI is unresponsive.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/publish.ymltemoa/__about__.pytests/smoke_test.py
🧰 Additional context used
🪛 Ruff (0.14.10)
tests/smoke_test.py
11-11: Starting a process with a partial executable path
(S607)
19-19: Starting a process with a partial executable path
(S607)
29-29: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (5)
.github/workflows/publish.yml (2)
3-7: LGTM! Appropriate trigger configuration.The tag-based trigger for version releases follows conventional patterns for publishing workflows.
10-16: LGTM! Correct permissions for trusted publishing.The
id-token: writepermission is properly configured for OIDC-based trusted publishing to PyPI.temoa/__about__.py (1)
3-3: LGTM! Version bump aligns with release workflow.The version change from development (
4.0.0a1.dev20251201) to alpha (4.0.0a1) is appropriate for the new publishing workflow targeting stable releases.tests/smoke_test.py (2)
1-3: LGTM! Imports are appropriate for smoke tests.The direct import of
temoais correct for this use case, as the workflow installs the package before running tests.
23-31: LGTM! Exception handling is appropriate for smoke tests.The bare
except Exceptionis acceptable in this context, as the smoke test needs to catch and report any failure. The static analysis warning (BLE001) can be safely ignored here.
| - name: Smoke test (wheel) | ||
| run: uv run --isolated --no-project --with dist/*.whl tests/smoke_test.py | ||
| - name: Smoke test (source distribution) | ||
| run: uv run --isolated --no-project --with dist/*.tar.gz tests/smoke_test.py |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding timeouts to smoke test steps.
The subprocess calls in tests/smoke_test.py don't have explicit timeouts, which could cause the workflow to hang if the CLI commands fail to respond. Consider adding a timeout-minutes directive to these steps as a safeguard.
🔎 Proposed enhancement
# Check that basic features work and we didn't miss to include crucial files
- name: Smoke test (wheel)
+ timeout-minutes: 5
run: uv run --isolated --no-project --with dist/*.whl tests/smoke_test.py
- name: Smoke test (source distribution)
+ timeout-minutes: 5
run: uv run --isolated --no-project --with dist/*.tar.gz tests/smoke_test.pyCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @.github/workflows/publish.yml around lines 27 - 30, Add a timeout to the two
smoke test workflow steps ("Smoke test (wheel)" and "Smoke test (source
distribution)") so a hung CLI won't stall the run; update each job step to
include a timeout-minutes (e.g., timeout-minutes: 10) alongside the existing
name and run keys to ensure the subprocess in tests/smoke_test.py is bounded.
| - name: Publish | ||
| run: uv publish |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding version tag verification before publishing.
The workflow doesn't verify that the Git tag matches the version in temoa/__about__.py. This could lead to publishing artifacts with mismatched version metadata.
🔎 Proposed verification step
Add this step before the publish step:
- name: Verify version matches tag
run: |
TAG_VERSION="${GITHUB_REF#refs/tags/v}"
PACKAGE_VERSION=$(uv run python -c "import temoa; print(temoa.__version__)")
if [ "$TAG_VERSION" != "$PACKAGE_VERSION" ]; then
echo "Error: Tag version ($TAG_VERSION) doesn't match package version ($PACKAGE_VERSION)"
exit 1
fi
echo "✅ Version verified: $PACKAGE_VERSION"🤖 Prompt for AI Agents
In @.github/workflows/publish.yml around lines 31 - 32, Add a pre-publish
verification step before the "Publish" run that extracts the tag version from
GITHUB_REF (e.g. strip "refs/tags/v"), reads the package version by running
python -c "import temoa; print(temoa.__version__)", compares the two, and fails
the job (exit 1) with a clear error if they differ; if they match, allow the
workflow to continue to the existing uv publish step and emit a success message.
| def test_import() -> None: | ||
| print(f"Importing temoa version: {temoa.__version__}") | ||
| assert temoa.__version__ is not None |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider strengthening the version assertion.
The current assertion only checks for None. Consider verifying it's a non-empty string to catch more potential issues.
🔎 Proposed enhancement
def test_import() -> None:
print(f"Importing temoa version: {temoa.__version__}")
- assert temoa.__version__ is not None
+ assert temoa.__version__ is not None
+ assert isinstance(temoa.__version__, str)
+ assert len(temoa.__version__) > 0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_import() -> None: | |
| print(f"Importing temoa version: {temoa.__version__}") | |
| assert temoa.__version__ is not None | |
| def test_import() -> None: | |
| print(f"Importing temoa version: {temoa.__version__}") | |
| assert temoa.__version__ is not None | |
| assert isinstance(temoa.__version__, str) | |
| assert len(temoa.__version__) > 0 |
🤖 Prompt for AI Agents
In @tests/smoke_test.py around lines 5 - 7, The test_import test currently only
asserts temoa.__version__ is not None; strengthen it to ensure temoa.__version__
is a non-empty string by checking its type and that its length is > 0 (e.g.,
assert isinstance(temoa.__version__, str) and assert temoa.__version__.strip()
!= ""), referencing the test_import function and temoa.__version__ to locate and
update the assertion.
| def test_cli() -> None: | ||
| print("Running temoa --version CLI command...") | ||
| result = subprocess.run(["temoa", "--version"], capture_output=True, text=True) | ||
| print(f"CLI output: {result.stdout.strip()}") | ||
| assert result.returncode == 0 | ||
| assert "Temoa Version:" in result.stdout | ||
| assert temoa.__version__ in result.stdout |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add timeout to subprocess calls to prevent hangs.
While the test logic is sound, subprocess calls without timeouts could cause the CI to hang indefinitely if the CLI becomes unresponsive.
🔎 Proposed fix
def test_cli() -> None:
print("Running temoa --version CLI command...")
- result = subprocess.run(["temoa", "--version"], capture_output=True, text=True)
+ result = subprocess.run(["temoa", "--version"], capture_output=True, text=True, timeout=30)
print(f"CLI output: {result.stdout.strip()}")
assert result.returncode == 0
assert "Temoa Version:" in result.stdout
assert temoa.__version__ in result.stdout📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_cli() -> None: | |
| print("Running temoa --version CLI command...") | |
| result = subprocess.run(["temoa", "--version"], capture_output=True, text=True) | |
| print(f"CLI output: {result.stdout.strip()}") | |
| assert result.returncode == 0 | |
| assert "Temoa Version:" in result.stdout | |
| assert temoa.__version__ in result.stdout | |
| def test_cli() -> None: | |
| print("Running temoa --version CLI command...") | |
| result = subprocess.run(["temoa", "--version"], capture_output=True, text=True, timeout=30) | |
| print(f"CLI output: {result.stdout.strip()}") | |
| assert result.returncode == 0 | |
| assert "Temoa Version:" in result.stdout | |
| assert temoa.__version__ in result.stdout |
🧰 Tools
🪛 Ruff (0.14.10)
11-11: Starting a process with a partial executable path
(S607)
🤖 Prompt for AI Agents
In @tests/smoke_test.py around lines 9 - 15, The subprocess.run call in test_cli
can hang indefinitely; add a timeout (e.g., timeout=10) to the subprocess.run
invocation in the test_cli function and handle subprocess.TimeoutExpired by
failing the test (raise or pytest.fail) with a clear message; update the call
from subprocess.run([...], capture_output=True, text=True) to include timeout=10
and wrap it in try/except subprocess.TimeoutExpired to provide a deterministic
test failure if the CLI is unresponsive.
| def test_help() -> None: | ||
| print("Running temoa --help CLI command...") | ||
| result = subprocess.run(["temoa", "--help"], capture_output=True, text=True) | ||
| assert result.returncode == 0 | ||
| assert "The Temoa Project" in result.stdout |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add timeout to subprocess call.
Same recommendation as test_cli() - add a timeout parameter to prevent potential hangs.
🔎 Proposed fix
def test_help() -> None:
print("Running temoa --help CLI command...")
- result = subprocess.run(["temoa", "--help"], capture_output=True, text=True)
+ result = subprocess.run(["temoa", "--help"], capture_output=True, text=True, timeout=30)
assert result.returncode == 0
assert "The Temoa Project" in result.stdout📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_help() -> None: | |
| print("Running temoa --help CLI command...") | |
| result = subprocess.run(["temoa", "--help"], capture_output=True, text=True) | |
| assert result.returncode == 0 | |
| assert "The Temoa Project" in result.stdout | |
| def test_help() -> None: | |
| print("Running temoa --help CLI command...") | |
| result = subprocess.run(["temoa", "--help"], capture_output=True, text=True, timeout=30) | |
| assert result.returncode == 0 | |
| assert "The Temoa Project" in result.stdout |
🧰 Tools
🪛 Ruff (0.14.10)
19-19: Starting a process with a partial executable path
(S607)
🤖 Prompt for AI Agents
In @tests/smoke_test.py around lines 17 - 21, The test_help function uses
subprocess.run without a timeout which can hang; update the subprocess.run call
in test_help to include a timeout (match the value used in test_cli, e.g.,
timeout=10) and optionally catch subprocess.TimeoutExpired to fail the test with
a clear message; target the subprocess.run invocation inside test_help.
e10e0f9 to
edd743a
Compare
ParticularlyPythonicBS
force-pushed
the
infra/publish_ci
branch
from
edd743a to
591772f
Compare
ParticularlyPythonicBS
force-pushed
the
infra/publish_ci
branch
from
591772f to
d1ca3a6
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
tests/smoke_test.py (2)
9-15: 🧹 Nitpick | 🔵 TrivialAdd explicit
check=Falseto document intent and add timeout.Since you're manually checking
returncode, adding explicitcheck=Falsedocuments this intent and silences the linter warning (PLW1510). The timeout concern from prior feedback also remains applicable.🔧 Proposed fix
def test_cli() -> None: print("Running temoa --version CLI command...") - result = subprocess.run(["temoa", "--version"], capture_output=True, text=True) + result = subprocess.run(["temoa", "--version"], capture_output=True, text=True, check=False, timeout=30) print(f"CLI output: {result.stdout.strip()}") assert result.returncode == 0 assert "Temoa Version:" in result.stdout assert temoa.__version__ in result.stdout,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/smoke_test.py` around lines 9 - 15, In test_cli update the subprocess.run call inside test_cli to pass explicit check=False and a reasonable timeout (e.g., timeout=10) so the test documents that return codes are checked manually and cannot hang; locate the subprocess.run invocation in the test_cli function and add these kwargs while keeping capture_output=True and text=True unchanged.
17-21: 🧹 Nitpick | 🔵 TrivialSame recommendations apply: explicit
check=Falseand timeout.🔧 Proposed fix
def test_help() -> None: print("Running temoa --help CLI command...") - result = subprocess.run(["temoa", "--help"], capture_output=True, text=True) + result = subprocess.run(["temoa", "--help"], capture_output=True, text=True, check=False, timeout=30) assert result.returncode == 0 assert "The Temoa Project" in result.stdout,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/smoke_test.py` around lines 17 - 21, The subprocess.run invocation in test_help (function test_help) should explicitly pass check=False and a reasonable timeout to avoid hanging tests; update the call to subprocess.run(["temoa", "--help"], capture_output=True, text=True, check=False, timeout=30) so the test explicitly opts out of raising on nonzero exit and fails fast on hangs while keeping the existing assertions on result.returncode and result.stdout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/smoke_test.py`:
- Around line 9-15: In test_cli update the subprocess.run call inside test_cli
to pass explicit check=False and a reasonable timeout (e.g., timeout=10) so the
test documents that return codes are checked manually and cannot hang; locate
the subprocess.run invocation in the test_cli function and add these kwargs
while keeping capture_output=True and text=True unchanged.
- Around line 17-21: The subprocess.run invocation in test_help (function
test_help) should explicitly pass check=False and a reasonable timeout to avoid
hanging tests; update the call to subprocess.run(["temoa", "--help"],
capture_output=True, text=True, check=False, timeout=30) so the test explicitly
opts out of raising on nonzero exit and fails fast on hangs while keeping the
existing assertions on result.returncode and result.stdout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 45d3af42-a00d-437e-8f01-3d66a1cb33ed
📒 Files selected for processing (3)
.github/workflows/publish.ymltemoa/__about__.pytests/smoke_test.py
ParticularlyPythonicBS
force-pushed
the
infra/publish_ci
branch
from
d1ca3a6 to
c79f90c
Compare
1 participant
Adds CI workflow for uploading tagged
vcommits to pypi when enabled through trusted publishing.Also adds a simple smoke_test to ensure the build functions as intended
Summary by CodeRabbit