-
-
Notifications
You must be signed in to change notification settings - Fork 10
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
Added support for trusted publishers. #73
Conversation
f7a7e8b
to
6095ad8
Compare
python -m pip install poetry githubrelease httpx==0.16.1 autopub | ||
echo "##[set-output name=release;]$(autopub check)" | ||
- name: Publish | ||
python -m pip install poetry autopub[github] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume the github
extra works just fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I believe the github
extra should work fine. There was a point at which I pinned httpx
in order to work around an issue with githubrelease
, but I imagine that has since been resolved. I'd rather take the approach you have here and then re-pin httpx
if and when needed. 👍
python -m pip install poetry autopub[github] | ||
echo "release=$(autopub check)" >> "$GITHUB_OUTPUT" | ||
EOF=$(dd if=/dev/urandom bs=15 count=1 status=none | base64) | ||
echo "release<<$EOF" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set-ouput
is deprecated — I have switched it over to multiline strings since we will never know how much autopub check
will output… If autopub check
were to write into $GITHUB_OUTPUT/STATE
on its own, we wouldn't need to handle this extra. I mean it has special handling for CircleCI also… https://github.com/autopub/autopub/blob/8d41aed1ca9ff0aa197a7850eb7d4314a96a1206/autopub/check_release.py#L13
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intention was always for AutoPub to internally handle as much of the machinery as possible, so I agree that AutoPub should be enhanced to obviate the need to handle this in GitHub Actions. 👍
run: | | ||
git remote set-url origin https://$GITHUB_TOKEN@github.com/${{ github.repository }} | ||
autopub prepare | ||
poetry build | ||
autopub build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to set the build-system
in pyproject.toml
for that to work since autopub
does not seem to detect that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. That's odd. Even without adding an explicit key+value for tool.autopub.build-system
, AutoPub should indeed detect it automatically from build-system.requires
, as you can see here: https://github.com/autopub/autopub/blob/8d41aed1ca9ff0aa197a7850eb7d4314a96a1206/autopub/base.py#L94-L96
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I have some cleanups for it there: autopub/autopub#38 -- requires
is a list so the in would only check for requires = ['poetry']
. Using the build_backend instead with a bit of fuzzy matching seems to work though.
.github/workflows/ci.yml
Outdated
run: | | ||
git remote set-url origin https://$GITHUB_TOKEN@github.com/${{ github.repository }} | ||
autopub prepare | ||
poetry build | ||
autopub build | ||
autopub commit | ||
autopub githubrelease |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like that the githubrelease upload has the packages from autopub build
and the pypi upload builds it's own packages. I'd also love to use build-and-inspect-python-package
from the pypi-package
workflow to be able to inspect the outputs properly before. I guess I might move autopub githubrelease
into the other workflow and run the other workflow on tags only? This would still result in a tag being pushed but we could then only run autopub githubrelease
& pypi upload after we checked the output from build-and-inspect-python-package
manually?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed — this seems like a more sensible approach. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arg, the tags are not created by autopub commit
but by autopub githubrelease
. Honestly autopub is really trying to make this hard here. But I am somewhat out of ideas :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tag is created as part of the githubrelease
step because the latter needs to know the tag in order to create the release. It might be feasible to separate the tagging step into a separate command (autopub tag
), but then we would need some way to maintain "state" such that the tag info could be passed to the githubrelease
step.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mhm the state could be the tag (?) of the current commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In an effort to find a more efficient place for us to discuss this topic, I created an AutoPub issue along with a proposal. What do you think? autopub/autopub#39
.github/workflows/pypi-package.yml
Outdated
|
||
release-pypi: | ||
name: Publish released package to pypi.org | ||
environment: release-pypi # TODO: how to name the env |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also Deployment
like we have now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deployment
seems like a sensible choice to me. 👍
@@ -92,3 +87,6 @@ DJANGO_SETTINGS_MODULE = 'testproj.settings' | |||
[tool.flake8] | |||
ignore = ['E203', 'E501', 'W503'] | |||
max-line-length = 88 | |||
|
|||
[tool.check-wheel-contents] | |||
ignore = ['W004'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Required because Django migrations are not valid module names.
You can see the generated metadata at https://github.com/apollo13/kagi/actions/runs/5154225573 I think it is rather nice to be able to inspect the wheel contents / metadata etc before uploading. |
It looks like the wheel contains the tests: https://github.com/apollo13/kagi/actions/runs/5154225573/jobs/9282424241#step:3:245. I think, we should exclude the test files form the package. |
@MarkusH for that we'd probably have to move them out of the package unless we want to work against the build tooling? |
@apollo13 I would mind moving them. Back in the setup.py times, one could exclude files with a specific pattern. Does that not exist anymore? |
It should be possible to exclude the tests from the build by adding something like the following to [tool.poetry]
[…]
exclude = [
"kagi/tests"
] |
e378a66
to
3e62852
Compare
This change is getting larger than I'd like it to be. I'll work on a new minimal set that will allow us to use trusted publishers and we can add the other improvements later on. Hynek gave me a bit to think about so I am currently playing with tools like https://github.com/your-tools/tbump (for release bumping & tagging) as well as towncrier for the changelog (cause I like the idea of adding the changelog immediately and not as an afterthought). I am sorry for closing this, but I guess it gave us all a bit to think about and maybe we will revive parts in the future? Expect the new PR today… |
I also like recording changelog entries as part of contributions and not as an afterthought, which is precisely why AutoPub does just that via the I rather like how AutoPub allows maintainers to issue releases merely by merging pull requests, and I'd like to continue improving AutoPub to add support for trusted publishers as well as work out any obstacles that inhibit alternative use cases. |
Mhm I must have missed something then. I thought as soon as a RELEASE.md file is there this triggers a release. Or differently put: if you have three pull requests, how do you submit a changelog entry for all of them without having to do a release for each of them?
Understood, and I fully accept that. But I will focus more on kagi itself than improving autopub. |
This would exclude the tests from the source package as well though (unless I made a mistake when testing locally). |
Related issue: python-poetry/poetry#3380 |
@justinmayer this will be a lot to review, I'll leave a few comments inline so you can chime in and suggest how we should do it properly.