feat(ci): add PyPI release workflow for splunk-ao#48
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
fercor-cisco
left a comment
There was a problem hiding this comment.
🤖 This review was generated by the Astra agent (claude-opus-4-8). It may contain mistakes.
Verdict: request_changes — Publish step relies on a mutable action ref against repo's SHA-pinning convention, and the version-setting step can silently diverge pyproject from init on non-literal input.
General Comments
- 🟡 minor (design): The PR description tells users to validate pre-releases with the Test PyPI workflow
release-splunk-ao-test.yaml, but no such workflow exists in the repo (the only workflows arecla.yml,ci-tests.yaml, and this new file). Either add the Test PyPI workflow or drop/adjust the reference so maintainers aren't pointed at something that isn't there.
Follow-ups
Suggested follow-up work that could be tracked as Shortcut stories:
.github/workflows/release-splunk-ao.yaml:17-18: Consider adding a top-levelconcurrencygroup (e.g.release-splunk-ao) to prevent two manual dispatches from racing and attempting to publish the same version simultaneously, mirroring the concurrency block already used in ci-tests.yaml..github/workflows/release-splunk-ao.yaml:34-39: The patched version is applied only to the local checkout and never committed back or tagged, so PyPI and the git history can drift (the repo still says 0.1.0 after a 0.2.0 release). Consider creating a git tag / GitHub release as part of this workflow, or document the expectation that pyproject is bumped via a separate PR before dispatch.
| path: dist/ | ||
|
|
||
| - name: Publish to PyPI | ||
| uses: pypa/gh-action-pypi-publish@release/v1 |
There was a problem hiding this comment.
🟠 major (security): This repo's ci-tests.yaml deliberately pins every third-party action to a full commit SHA (with comments explaining why). This new workflow breaks that convention: actions/checkout@v4, actions/setup-python@v5, actions/upload-artifact@v4, actions/download-artifact@v4, and most importantly pypa/gh-action-pypi-publish@release/v1 are all mutable refs. @release/v1 is a branch, so its tip can change under you. Since this is the highest-trust step in the repo (OIDC-authenticated publish to PyPI), pin it (and the other actions) to a SHA to match the established supply-chain posture.
| uses: pypa/gh-action-pypi-publish@release/v1 | |
| - name: Publish to PyPI | |
| uses: pypa/gh-action-pypi-publish@<full-commit-sha> # release/v1 | |
| with: | |
| verbose: true | |
| print-hash: true |
🤖 Generated by Astra
There was a problem hiding this comment.
@adityamehra pinning third-party actions is indeed important for the security posture. Many recent high level exploits were introduced by compromising this supply chain.
There was a problem hiding this comment.
Agreed — this is an important supply-chain safeguard, especially for the PyPI publish step which runs with OIDC identity. All five action references (, , , , and ) have been pinned to their full commit SHAs with the version tag preserved as a comment, matching the pattern in ci-tests.yaml. Pushed in the latest commit.
| run: | | ||
| poetry version "$INPUT_VERSION" | ||
| sed -i "s/__version__ = \".*\"/__version__ = \"${INPUT_VERSION}\"/" src/splunk_ao/__init__.py | ||
|
|
||
| - name: Get current version | ||
| id: get-version | ||
| run: echo "version=$(poetry version --short)" >> "$GITHUB_OUTPUT" | ||
|
|
||
| - name: Build package |
There was a problem hiding this comment.
🟡 minor (bug): poetry version "$INPUT_VERSION" interprets bump keywords (major, minor, patch, premajor, etc.) as bump rules rather than literal versions, whereas the sed line always writes the literal input string. If a maintainer enters patch, poetry bumps 0.1.0 -> 0.1.1 in pyproject.toml while __init__.py gets __version__ = "patch" — the built artifact then ships mismatched versions. The input is also otherwise unvalidated, so a typo like 1.0 or a value containing //" silently produces a bad build or breaks the sed expression. Validate the input against a version pattern up front so both files stay in sync.
| run: | | |
| poetry version "$INPUT_VERSION" | |
| sed -i "s/__version__ = \".*\"/__version__ = \"${INPUT_VERSION}\"/" src/splunk_ao/__init__.py | |
| - name: Get current version | |
| id: get-version | |
| run: echo "version=$(poetry version --short)" >> "$GITHUB_OUTPUT" | |
| - name: Build package | |
| - name: Set version | |
| env: | |
| INPUT_VERSION: ${{ inputs.version }} | |
| run: | | |
| if ! printf '%s' "$INPUT_VERSION" | grep -Eq '^[0-9]+\.[0-9]+\.[0-9]+([abprc.][0-9]+)*$'; then | |
| echo "::error::Invalid version '$INPUT_VERSION'; expected an explicit version like 0.1.0" | |
| exit 1 | |
| fi | |
| poetry version "$INPUT_VERSION" | |
| sed -i "s/__version__ = \".*\"/__version__ = \"${INPUT_VERSION}\"/" src/splunk_ao/__init__.py |
🤖 Generated by Astra
There was a problem hiding this comment.
Good catch — both issues are now addressed in the latest commit:
-
Version format validation: Added an upfront
grep -Eqcheck that rejects anything that is not an explicit PEP 440 version number (e.g.0.1.0,1.2.3b1). Bump keywords likepatch,major, orminorwill now fail fast with a clear::error::message before touching any files. -
pyproject.toml update via sed: Switched from
poetry version "$INPUT_VERSION"tosedfor thepyproject.tomlbump. This makes both file updates symmetric and removes the ambiguity between Poetry bump-rule keywords and literal version strings entirely, matching the approach used in thesplunk-ao-adkandsplunk-ao-a2aworkflows.
- Replace mutable action refs (v4, v5, release/v1) with full commit SHAs to match the supply-chain convention established in ci-tests.yaml - Add upfront version format validation in the Set version step to reject Poetry bump keywords (patch, major, etc.) and malformed inputs - Switch pyproject.toml update to sed (consistent with adk/a2a workflows) to remove ambiguity between Poetry bump rules and literal versions Co-authored-by: Cursor <cursoragent@cursor.com>
|
Thanks for the thorough review @fercor-cisco! Here is a rundown of where we landed: ✅ SHA pinning (inline comments) — Addressed in the latest commit. All actions are now pinned to full commit SHAs, including ✅ Version input validation (inline comment) — Also addressed. Added a regex check that rejects bump keywords and malformed strings before any file is modified. Switched 🤔 Concurrency group — We have decided to skip this for now. The workflow is 📝 Version drift (version not committed back) — You are right that this is a real concern. We cannot automate the commit-back step yet because there is no GitHub service account configured for this repository to push commits from CI. The expected process is to bump 📝 PR description (Test PyPI reference) — Updated the PR description to reference the actual Test PyPI workflow ( |
fercor-cisco
left a comment
There was a problem hiding this comment.
🤖 This review was generated by the Astra agent (claude-opus-4-8). It may contain mistakes.
Verdict: approve — Solid minimal release workflow; prior review feedback addressed. Remaining items are minor (regex doesn't fully cover PEP 440; build job lacks least-privilege token).
Follow-ups
Suggested follow-up work that could be tracked as Shortcut stories:
.github/workflows/release-splunk-ao.yaml:1-73: Version drift: the version bump to pyproject.toml/init.py is applied only in the CI runner and never committed back, so the repo and the published artifact can diverge. The PR acknowledges this and documents a manual bump-PR-then-dispatch process pending a CI service account. Track automating the commit-back (or tag-driven release) once the service account exists.
| env: | ||
| INPUT_VERSION: ${{ inputs.version }} | ||
| run: | | ||
| if ! printf '%s' "$INPUT_VERSION" | grep -Eq '^[0-9]+\.[0-9]+\.[0-9]+([abprc.][0-9]+)*$'; then |
There was a problem hiding this comment.
🟡 minor (bug): This regex does not actually match the full PEP 440 surface the PR description claims. The character class requires every suffix marker to be a single char followed by digits, so canonical release-candidate and dev/post forms are rejected:
1.2.3rc1→ rejected (ris followed byc, not a digit).rcis PEP 440's preferred RC spelling; only the alternatecform (1.2.3c1) passes.1.2.3.dev0→ rejected (.must be followed by a digit, but is followed byd).1.2.3.post1→ rejected (same reason).
For the common X.Y.Z final-release path this is fine, but a maintainer trying to publish an RC or dev pre-release will hit a confusing Invalid version error even though the value is valid PEP 440 / acceptable to PyPI. Either broaden the pattern to cover a/b/rc pre-releases and .postN/.devN, or tone the description/error message down so it doesn't promise full PEP 440 support.
| if ! printf '%s' "$INPUT_VERSION" | grep -Eq '^[0-9]+\.[0-9]+\.[0-9]+([abprc.][0-9]+)*$'; then | |
| if ! printf '%s' "$INPUT_VERSION" | grep -Eq '^[0-9]+\.[0-9]+\.[0-9]+((a|b|rc)[0-9]+)?(\.post[0-9]+)?(\.dev[0-9]+)?$'; then |
🤖 Generated by Astra
There was a problem hiding this comment.
Great catch — the original character class ([abprc.][0-9]+)* was wrong on two counts: rc was treated as two separate single characters (rejecting rc1), and .dev/.post suffixes broke because . was followed by a letter rather than a digit.
Updated to the suggested anchored alternation:
^[0-9]+\.[0-9]+\.[0-9]+((a|b|rc)[0-9]+)?(\.post[0-9]+)?(\.dev[0-9]+)?$
This now correctly accepts 1.2.3, 1.2.3a1, 1.2.3b2, 1.2.3rc1, 1.2.3.post1, and 1.2.3.dev0, while still rejecting bump keywords and malformed inputs. Applied to all five release workflows (splunk-ao, splunk-ao-adk, splunk-ao-adk-test, splunk-ao-a2a, splunk-ao-a2a-test).
| build: | ||
| name: Build package | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
🟡 minor (security): The build job declares no permissions: block, so it inherits the repository's default GITHUB_TOKEN scope (often read/write). The publish job correctly scopes itself to id-token: write only. Given this PR's explicit supply-chain hardening focus (SHA pinning, OIDC publish), apply least privilege to the build job too — it only needs to read the checked-out repo. Consider adding permissions: contents: read to the build job (or a top-level workflow default).
| build: | |
| name: Build package | |
| runs-on: ubuntu-latest | |
| build: | |
| name: Build package | |
| runs-on: ubuntu-latest | |
| permissions: | |
| contents: read |
🤖 Generated by Astra
There was a problem hiding this comment.
Good point — added permissions: contents: read to the build job so it explicitly opts in to the minimum scope it needs rather than inheriting the repo-wide default. Applied the same to the test and build jobs across all five release workflows for consistency.
- Fix regex to correctly accept rc, .dev, and .post PEP 440 forms: old: ([abprc.][0-9]+)* incorrectly rejects rc1 and .dev0 new: ((a|b|rc)[0-9]+)?(\.post[0-9]+)?(\.dev[0-9]+)? - Add permissions: contents: read to the build job so it does not inherit the default repo-wide GITHUB_TOKEN scope Co-authored-by: Cursor <cursoragent@cursor.com>
Summary
Adds a GitHub Actions workflow (`release-splunk-ao.yaml`) that publishes the `splunk-ao` package to PyPI using PyPI Trusted Publisher (OIDC) — no stored API tokens required.
How it works
Pre-requisites
Testing / Validation
Before running this workflow for a production release, validate the package with the Test PyPI workflow: see PR #44 (`release-splunk-ao-test.yaml`) which publishes `.dev` pre-releases to test.pypi.org/project/splunk-ao.
Version management note
Because there is no GitHub service account configured for this repo yet, the version bump in `pyproject.toml` is not committed back automatically. The expected process is:
This will be automated once a service account is available.
Related