Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Build Docker image and push to GHCR #230
base: unstable/v1
Are you sure you want to change the base?
Build Docker image and push to GHCR #230
Changes from all commits
9756850
769f499
3384fad
9ae1850
1b535ae
213c885
5f0743b
049447a
2fb692b
30c8fae
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Two separate workflow runs is often hard to track. Instead, I adopted a practice of modularizing the workflow pieces as reusable workflows having the
reusable-
prefix in their names. This allows embedding everything in all the right places. Let's try this, WDYT?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 will return to this suggestion at a later time.
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.
Why would this be needed outside the container?
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.
Because you've set up a test that modifies the
$PATH
(#112, 1350b8b).gh-action-pypi-publish/.github/workflows/self-smoke-test-action.yml
Lines 85 to 90 in 699cd61
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.
Isn't it easier to do it like this?
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.
You would think so, but the syntax is needed to work around a limitation of composite actions. See:
actions/runner#2473
github/docs#25336 (comment)
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.
Oh, good to know! It's probably safer too.
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.
This is not the right way of using constraint files. Here's what should be done instead:
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.
Another thing to consider, provided that it's actually needed (see https://github.com/pypa/gh-action-pypi-publish/pull/230/files#r1619055808), is making a virtualenv managed by this action (in its checkout/tmp dir so that the end-user's projects aren't polluted).
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.
This would break the deprecation messages, it seems. Have you checked?
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 think so. The new inputs don't have defaults, so for example,
${{ inputs.repository-url || inputs.repository_url }}
will default toinputs.repository_url
anddeprecationMessage
will be logged.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.
Why do you check for
'gh-action-pypi-publish' in repo
? How about forks that renamed it?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.
Could you check if using just
would work?
Most of the time, any valid JSON is also valid YAML so GHA should be able to read it...
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.
Alternatively, we could have a template for the YAML file that would be post-processed by Template strings,
str.format_map()
or even just f-strings.Any of these would let us avoid having an external dependency.
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.
Compiling PyYAML on various VMs might be problematic, so I'd pass
--only-binary=:all:
via pip-opts or even put it into the config. This way, installing would be more predictable on different versions of Ubuntu VMs that GitHub has.Another concern is that originally, I opted for a docker-based action so that the external runtime is not mutated (that's one of a few reasons). By installing things with
pip
we violate that promise. I was kinda hoping that it'd be possible to avoid using a third-party dependency at all.