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

COMP: Update GitHub actions for publishing to PyPI #136

Merged
merged 4 commits into from
Sep 18, 2024

Conversation

Leengit
Copy link
Collaborator

@Leengit Leengit commented Sep 17, 2024

GitHub gave errors about publishing HistomicsStream 2.5.0. I am hoping that bumping actions/setup-python@v1 to actions/setup-python@v2 in .github/workflows/build-test-package.yml will be sufficient to fix the problem, but I don't know how to test that other than merging this pull request and attempting to make a 2.5.1 release via GitHub.

@Leengit Leengit added the bug Something isn't working label Sep 17, 2024
@Leengit Leengit self-assigned this Sep 17, 2024
@Leengit Leengit marked this pull request as ready for review September 18, 2024 12:47
@Leengit Leengit requested a review from manthey September 18, 2024 12:51
@@ -66,7 +66,7 @@ jobs:
steps:
- uses: actions/checkout@master
- name: Set up Python "3.9"
uses: actions/setup-python@v1
uses: actions/setup-python@v2
Copy link

Choose a reason for hiding this comment

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

Is there a reason not to jump all the way to the latest version? v5 exists, for instance.

In other projects, we use dependabot to update actions.

And, I'll note that the pypi-publish action recommends a version of release/v1 rather than master.

Copy link
Collaborator Author

@Leengit Leengit Sep 18, 2024

Choose a reason for hiding this comment

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

I was aiming for the minimal change that works, to minimize the chance of breaking something else. Though, if v5 is expected to be backward compatible ... I'll make that change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In other projects, we use dependabot to update actions.

I don't know anything about how to do that. If that is something you can do in your sleep, that'd be great! Otherwise, I'd appreciate your opinion on the relative importance of pursuing that.

Copy link

Choose a reason for hiding this comment

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

You add a dependabot config file in the .githib folder (see https://github.com/DigitalSlideArchive/superpixel-classification/blob/main/.github/dependabot.yml as an example -- you can just copy that). Then on some schedule if there are updates to the actions, dependabot will make PRs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have added .github/dependabot.yml as a copy of the one from DigitalSlideArchive/superpixel-classification.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And, I'll note that the pypi-publish action recommends a version of release/v1 rather than master.

Thank you. Done.

@manthey
Copy link

manthey commented Sep 18, 2024

One way to mostly test this without merging would be to have another step in the workflow that would publish to test pypi ( with: repository-url: https://test.pypi.org/legacy/ in the publish action, and probably with skip-existing: true only for the test.pypi part of the code).

@Leengit
Copy link
Collaborator Author

Leengit commented Sep 18, 2024

One way to mostly test this without merging would be to have another step in the workflow that would publish to test pypi ( with: repository-url: https://test.pypi.org/legacy/ in the publish action, and probably with skip-existing: true only for the test.pypi part of the code).

Thank you, I have added that. Let's see how it goes.

@Leengit
Copy link
Collaborator Author

Leengit commented Sep 18, 2024

Unfortunately, publishing to test.pypi.org is not working. I get an error whether or not I have

if: github.event.repository.fork == false

in the new workflow. The error is:

Error: Trusted publishing exchange failure: 
OpenID Connect token retrieval failed: GitHub: missing or insufficient OIDC token permissions, the ACTIONS_ID_TOKEN_REQUEST_TOKEN environment variable was unset

The workflow context indicates that this action was called from a
pull request on a fork. GitHub doesn't give these workflows OIDC permissions,
even if `id-token: write` is explicitly configured.

To fix this, change your publishing workflow to use an event that
forks of your repository cannot trigger (such as tag or release
creation, or a manually triggered workflow dispatch).

@Leengit
Copy link
Collaborator Author

Leengit commented Sep 18, 2024

Actually, the publishing action to test.pypi.org does not report failure for the build on Leengit/HistomicsStream because it is detected that it is a fork, and the action is skipped. However publishing fails for the build on DigitalSlideArchive/HistomicsStream, with the same error. It appears that even the build on DigitalSlideArchive/HistomicsStream doesn't have sufficient permission when triggered by a push to this pull request.

@Leengit
Copy link
Collaborator Author

Leengit commented Sep 18, 2024

@manthey What do you think?: Possibly after removing the just added test.pypi.org action, I would like to merge this pull request, and try to release as v2.5.1 from GitHub.

@manthey
Copy link

manthey commented Sep 18, 2024

@manthey What do you think?: Possibly after removing the just added test.pypi.org action, I would like to merge this pull request, and try to release as v2.5.1 from GitHub.

Yes, give it a try.

@Leengit Leengit merged commit fb332e5 into DigitalSlideArchive:master Sep 18, 2024
5 of 6 checks passed
@Leengit Leengit deleted the pypi_publish branch September 18, 2024 20:58
@Leengit
Copy link
Collaborator Author

Leengit commented Oct 1, 2024

The reason that the GitHub action to publish to PyPI was failing was not the above but more mundane; failure to update the __version__ string within histomics_stream/__init__.py.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants