DEVOPS-1154: [WIP] Devops 1154 - #216
Conversation
There was a problem hiding this comment.
Pull request overview
This PR appears to transition PyPI publishing from token-based uploads inside reusable workflows to an OIDC “trusted publishing” approach by introducing a composite action and removing the direct pypa/gh-action-pypi-publish steps from the reusable workflows.
Changes:
- Removed the “Publish package to PyPI” step from two reusable workflows that previously published to PyPI/TestPyPI.
- Added a new composite action (
reusable-python-publish_to_pypi) intended to publish distributions to PyPI/TestPyPI via trusted publishing, supporting artifact or release-asset sources.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
.github/workflows/reusable-python-release_pypi_assets.yml |
Removes the PyPI/TestPyPI publish step from the release-asset publishing workflow. |
.github/workflows/reusable-python-publish_pypi_package.yml |
Removes the PyPI/TestPyPI publish step from the package publish workflow. |
.github/actions/reusable-python-publish_to_pypi/action.yml |
Adds a composite action to publish to PyPI/TestPyPI using OIDC trusted publishing. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| Must be invoked as a step in a job defined directly in the caller's own | ||
| top-level workflow file (not from within a reusable *workflow*), so the | ||
| OIDC token's workflow_ref matches the repository's configured PyPI | ||
| trusted publisher. |
| - name: Download build artifact | ||
| if: ${{ inputs.source == 'artifact' }} | ||
| uses: actions/download-artifact@v8 | ||
| with: | ||
| name: ${{ inputs.artifact-name }} | ||
| path: dist-download | ||
| - name: Download release assets |
504ca60 to
3feab80
Compare
|
|
sebhmg
left a comment
sebhmg
left a comment
There was a problem hiding this comment.
suggestion: have 2 different simpler actions instead of bundling 2 uses-case into one action
| source: | ||
| description: 'Where to fetch the built packages from: "artifact" or "release"' | ||
| required: true | ||
| type: string | ||
| artifact-name: | ||
| description: 'Name of the workflow artifact to download. Required when source is "artifact"' | ||
| required: false | ||
| type: string | ||
| default: '' | ||
| release-tag: | ||
| description: 'Tag of the GitHub release to download assets from. Required when source is "release"' | ||
| required: false | ||
| type: string | ||
| default: '' | ||
| github-token: | ||
| description: 'Token used to download release assets via "gh release download". Required when source is "release"' | ||
| required: false | ||
| type: string | ||
| default: '' |
There was a problem hiding this comment.
suggestion: keep tow different actions: one for release, one for build artifact
within this single action, there is extra complexity with the requirement on expect params depending on value of source, and parsm being exclusive
As use case as separeted, having 2 different actions seem to make sense: publishing to prod will always fetch from release, while publishing to dev will always fetch from build.
There was a problem hiding this comment.
suggestion: have the pipeline use directly actions/download-artifact
this composite action does not appear to add any logic on top of actions/download-artifact
| description: 'List of virtual repository names to publish to (e.g. ["public-pypi-dev", "geophysics-pypi-dev"])' | ||
| description: > | ||
| List of virtual repository names to publish to (e.g. ["public-pypi-dev", "geophysics-pypi-dev"]). | ||
| "pypi"/"test-pypi" are a deliberate no-op here; PyPI publishing runs in the caller (DEVOPS-1154). |
There was a problem hiding this comment.
issue (coding-style): do not refer to JIRA issue number in code
instead refer to issue in PR comments
| - name: Report PyPI publish is handled by the caller | ||
| if: ${{ matrix.virtual-repo-name == 'pypi' || matrix.virtual-repo-name == 'test-pypi' }} | ||
| run: | | ||
| echo "::notice::Not publishing '${{ matrix.virtual-repo-name }}' here: PyPI trusted publishing must run as a direct step in the caller's top-level workflow, not through this reusable workflow. See the 'virtual-repo-names' input description." |
There was a problem hiding this comment.
issue: report as error and fail the pipeline
A workflow that attempt to publish to pypi or pypi-test expect it to happen. It must not be a silent failure for that worklow.
True, not a failure from the reusable workflow, but an error from the calling workflow due to wrong input parameters
There was a problem hiding this comment.
issue (security: also address zizmor warning about template injection
There was a problem hiding this comment.
issue: same comments as in the other workflow apply here
DEVOPS-1154 - address warnings upon publishing Python packages from GitHub
testing with MiraGeoscience/simpeg#166