Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Workflow to Incorporate Trusted Publisher Mechanism for Publishing to Test PyPI #18

Merged
merged 27 commits into from
Jun 11, 2024

Conversation

Vicbi
Copy link
Collaborator

@Vicbi Vicbi commented Jun 3, 2024

Update Workflow to Incorporate Trusted Publisher Mechanism for Publishing to Test PyPI

⚙️ Release Notes

This pull request updates the GitHub Actions workflow to incorporate the trusted publisher mechanism for publishing our Python package to Test PyPI.

📝 Code of Conduct & Contributing Guidelines

By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines:

Copy link

codecov bot commented Jun 3, 2024

Codecov Report

Attention: Patch coverage is 38.70968% with 19 lines in your changes missing coverage. Please review.

Project coverage is 77.25%. Comparing base (614093c) to head (2c587c6).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #18      +/-   ##
==========================================
- Coverage   78.06%   77.25%   -0.80%     
==========================================
  Files          15       15              
  Lines        1180     1182       +2     
==========================================
- Hits          921      913       -8     
- Misses        259      269      +10     
Flag Coverage Δ
unittests 77.25% <38.71%> (-0.80%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...peline/data_flattening/fhir_resources_flattener.py 92.44% <100.00%> (ø)
...zi_data_pipeline/data_processing/data_processor.py 67.13% <47.06%> (-3.64%) ⬇️
..._pipeline/data_processing/observation_processor.py 32.84% <23.08%> (-1.59%) ⬇️

... and 12 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 614093c...2c587c6. Read the comment docs.

Copy link
Member

@PSchmiedmayer PSchmiedmayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the work and great to see the deployment at https://test.pypi.org/project/spezi-data-pipeline/#description!

Nice to see this being usable as a dependency!

Looking at the README handling in pypip it might be that the relative links to images don't seem to work on the export. It might make sense to add a full hosting path to the images in the MD README file?

.github/workflows/build-and-test.yml Outdated Show resolved Hide resolved
.github/workflows/publish-to-pypi.yml Outdated Show resolved Hide resolved
.yamllint Outdated Show resolved Hide resolved
.github/workflows/update-authors.yml Outdated Show resolved Hide resolved
#
# SPDX-License-Identifier: MIT
#

---
name: Publish Python Package

on:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be cool if we can also run the action using workflow_dispatch to, e.g., test this as part of a PR check or a manual testing release from one branch. Would be cool to switch the environment below based on that context.

Alternatively we could do this for tags that don't follow a major version scheme but rather have a postfix like 1.0.0-b1 or 1.0.0-b2.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice idea, I have added some conditions for setting the environment and the repo for publishing the package in the publish-to-pypi.yml workflow:

- name: Determine target repository
        id: get-repo
        run: |
          if [[ "${{ github.event_name }}" == "workflow_dispatch" ]]; then
            echo "::set-output name=repo::https://test.pypi.org/legacy/"
            echo "::set-output name=environment::test"
          elif [[ "${GITHUB_REF##*/}" == "main" ]]; then
            echo "::set-output name=repo::https://upload.pypi.org/legacy/"
            echo "::set-output name=environment::release"
          elif [[ "${GITHUB_REF##*/}" =~ ^[0-9]+\.[0-9]+\.[0-9]+-(b[0-9]+|alpha\.[0-9]+)$ ]]; then
            echo "::set-output name=repo::https://test.pypi.org/legacy/"
            echo "::set-output name=environment::test"
          else
            echo "::set-output name=repo::https://upload.pypi.org/legacy/"
            echo "::set-output name=environment::test"
          fi

      - name: Set environment
        run: echo "Environment: ${{ steps.get-repo.outputs.environment }}"

      - name: Publish package distributions to PyPI/Test PyPI
        uses: pypa/gh-action-pypi-publish@release/v1
        with:
          repository-url: ${{ steps.get-repo.outputs.repo }}

We can discuss the conditions together!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great if we can verify that it works as intended with e.g. a workflow dispatch that triggers this with a beta tag (e.g. 0.1.0-b1, 0.1.0-b2...).

I suspect that we need to differently set the deployment environment (telling us which secrets will be exposed and e.g., variables injected): https://docs.github.com/en/actions/deployment/targeting-different-environments/using-environments-for-deployment as noted in the comment below.

I would suggest to maybe extract that logic in a previous job (which environment should be used, what's the version number if we have passed in one, and what's the target URL ...) and then use that output of the job as an input for the other one and connect them using a needs relationship. I think that might be the easiest way to get the data into the environment: argument. Or at least that would be my first try. Let me know how I can help and if you run into any issues!

@@ -20,13 +22,16 @@ jobs:
build_and_publish:
needs: build_and_test
runs-on: ubuntu-latest

environment: test
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be fun to see if we can automatically use test and release depending of if we are on the main branch & use a tag or of the workflow is triggered by something different than a tag.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was addressed from the comment above with extending publish-to-pypi.yml workflow to determine the env and the target repository based on certain conditions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I unfortunately think that this might not address the environment issue here.
As far as I know we need to change the value that is provided here as an argument to the environment input as we need to multiplex the GitHub deployment environment (telling us which secrets will be exposed and e.g., variables injected): https://docs.github.com/en/actions/deployment/targeting-different-environments/using-environments-for-deployment

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Breakdown the possible scenarios:

1. Workflow dispatch with a valid beta tag

  • The workflow is manually triggered via workflow_dispatch.
  • Checks if the provided tag_name input matches the regex pattern for a beta tag (e.g., 1.0.0-b1).
  • Sets the repository URL to the Test PyPI repository, the environment to test, and the version to the provided tag_name.
  • If the tag_name does not match the valid beta tag pattern, the job fails.

2. Push to a tag matching the release pattern

  • The workflow is triggered by a push event.
  • Checks if the current ref (the branch or tag name) matches the regex pattern for a release tag (e.g., 1.0.0).
  • Sets the repository URL to the official PyPI repository, the environment to release, and the version to the tag name.

3. Push to a tag matching the beta or alpha pattern

  • The workflow is triggered by a push event.
  • Checks if the current ref (the branch or tag name) matches the regex pattern for a test tag (e.g., 1.0.0).
  • Sets the repository URL to the test PyPI repository, the environment to test, and the version to the tag name.

4. Invalid ref condition

@Vicbi
Copy link
Collaborator Author

Vicbi commented Jun 5, 2024

Thank you for the comments @PSchmiedmayer ! I have added some code updates including the automation of the environment change in the workflows. Happy to discuss any details after your review the changes! 👍

@Vicbi Vicbi requested a review from PSchmiedmayer June 5, 2024 05:14
Copy link
Member

@PSchmiedmayer PSchmiedmayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the work here @Vicbi; we are getting closer and closer. Sorry for the additional feedback and let me know what you think about the additional suggestions and improvements to the GitHub action to get the environment selection working as expected.

One the workflow dispatch is working, we can use that to trigger an other release in the testing environment and check if the changes in the README fixed the image display issues at https://test.pypi.org/project/spezi-data-pipeline/ 🚀

.github/workflows/pull_request.yml Outdated Show resolved Hide resolved
@@ -20,13 +22,16 @@ jobs:
build_and_publish:
needs: build_and_test
runs-on: ubuntu-latest

environment: test
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I unfortunately think that this might not address the environment issue here.
As far as I know we need to change the value that is provided here as an argument to the environment input as we need to multiplex the GitHub deployment environment (telling us which secrets will be exposed and e.g., variables injected): https://docs.github.com/en/actions/deployment/targeting-different-environments/using-environments-for-deployment

Comment on lines 52 to 54
if [[ "${{ github.event_name }}" == "workflow_dispatch" ]]; then
echo "repo=https://test.pypi.org/legacy/" >> $GITHUB_ENV
echo "environment=test" >> $GITHUB_ENV
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is that we won't have a git tag here to actually tag a specific release if we don't pass it in as an argument to the GitHub action workflow. Otherwise it might just take any previous tag (most probably the most recent one).

We might need to do something similar to out ResearchKit fork where we pass this in as an argument and then forward it to the other GitHub Actions as an argument or environment variable (confusing as it's different to the GitHub deployment environment 🙈).
If the environment variable is set, the script that sets the version code in the configuration file to publish should prefer this over the latest tag.

You can see how we define that input in our ResearchKit fork:
https://github.com/StanfordBDHG/ResearchKit/blob/main/.github/workflows/release.yml#L18

Comment on lines 55 to 57
elif [[ "${GITHUB_REF##*/}" == "main" ]]; then
echo "repo=https://upload.pypi.org/legacy/" >> $GITHUB_ENV
echo "environment=release" >> $GITHUB_ENV
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to fill in that case. Either we are executed as a workflow dispatch with a beta version number (e.g. in a PR or even on the main branch) or we are triggered by a tag. Otherwise it will be really hard to define which version number we want to use to publish the package (even in the testing environment).

Comment on lines 61 to 64
else
echo "repo=https://upload.pypi.org/legacy/" >> $GITHUB_ENV
echo "environment=test" >> $GITHUB_ENV
fi
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, I would suggest to fail if we run into this case as it will be hard to determine the version number without outside input.

#
# SPDX-License-Identifier: MIT
#

---
name: Publish Python Package

on:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great if we can verify that it works as intended with e.g. a workflow dispatch that triggers this with a beta tag (e.g. 0.1.0-b1, 0.1.0-b2...).

I suspect that we need to differently set the deployment environment (telling us which secrets will be exposed and e.g., variables injected): https://docs.github.com/en/actions/deployment/targeting-different-environments/using-environments-for-deployment as noted in the comment below.

I would suggest to maybe extract that logic in a previous job (which environment should be used, what's the version number if we have passed in one, and what's the target URL ...) and then use that output of the job as an input for the other one and connect them using a needs relationship. I think that might be the easiest way to get the data into the environment: argument. Or at least that would be my first try. Let me know how I can help and if you run into any issues!

@PSchmiedmayer PSchmiedmayer merged commit 6e61a75 into main Jun 11, 2024
10 of 11 checks passed
@PSchmiedmayer PSchmiedmayer deleted the publish-test-pypi branch June 11, 2024 18:28
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.

2 participants