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

Write built packages during publish #19346

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

bobthemighty
Copy link
Contributor

@bobthemighty bobthemighty commented Jun 17, 2023

Opening for early feedback, I've not tested this properly and it needs a bit of a polish, but I've run it locally and seen that Docker manifests are written to dist during publish, so ... works on my machine.

I've not yet checked what artifacts specifically are created by Helm, so I want to check that writing those makes sense, and I need to test the publish goal rule properly, but is this an insane thing to do?

I kinda hoped to be able to remove the names property from PublishPackages and calculate it on request from the packages, but that's a non-starter because we have several names for the same package depending on the registry we're pushing to. I want to check that this works sanely if you push to multiple registries, but I'm guessing that mergedigest will take care of duplicate digests for me.

fixes #19341

Copy link
Member

@kaos kaos left a comment

Choose a reason for hiding this comment

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

Nice!

One comment on how to propagate the built packages but apart from that I think this is a solid improvement.

Noting that it's not only the docker image metadata that is fixed by this, but also any python wheels you may publish (or any other backends publishable artifacts when implemented) would show up in dist now too, which seems correct as I think that pants publish should be equivalent to pants package publish as the publish goal will request the packaging implicitly, but sidestepping the package goal rule we missed to materialize the artifacts in the dist dir.

@@ -131,6 +133,7 @@ class PublishPackages:
"""

names: tuple[str, ...]
packages: tuple[BuiltPackage, ...]
Copy link
Member

Choose a reason for hiding this comment

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

Not to keen on adding this field here. It's only use is by the "internal" nature handled by the package_for_publish rule to propagate to the goal rule. Rather, I'd suggest adding a new result type for that rule that returns all the packages as well as the PublishProcesses.

That way we don't need to touch the backends rules with this change.

@tgolsson
Copy link
Contributor

tgolsson commented Jul 7, 2023

Saw this by accident; wondering whether this should be opt-in somehow. For my OCI backend this'd mean dumping a 5-6 GB chunk into dist whenever we publish our ML images, which seems a bit senseless. It seems to me the docker backend is behaving weirdly in this case by writing a "summary" of the artifact and not the artifact itself, leading to a feature that becomes weird for other backends. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docker publish does not write metadata to dist
3 participants