Specify publish dependencies on deployments

[lilatomic]

Infrastructure deployments can depend on other targets being packaged and published. For example, a Helm chart can deploy Docker images. This capability is similar to how runtime_package_dependencies allows packaging other targets for use in tests (for example, shunit2_test). Pants already has support for publishing dependencies before deploying them. However, this behaviour is not exposed to users and is only filled by the Helm backend when it identifies Docker targets to push.

This MR add a field publish_dependencies which allows users to manually specify targets to publish before deploying. closes #21522 .

Notes:

• experimental-deploy goals need to hook this field into their targets, but the execution is handled automatically
• I used a shared field so it's easy to use it for all backends implementing it
• Dependencies are propagated (eg if a dep in publish_dependencies changes, is the deployment marked as changed)

[Nishikoh]

This implementation is exactly what I've been waiting for! I'm absolutely thrilled thank you so much for this cool feature! 🚀

[alonsodomin]

I like the idea and in general, the implementation looks good to me. But it looks like that when the publish_dependencies field is used, then the inferred dependencies are ignored...?

I would like to see a behavior more akin to the dependencies field. Meaning that when dependencies are explicitly provided, these are added to the set of inferred ones. Similarly, the publish_dependencies field could be use to exclude inferred ones using the special syntax !path/to:target as the dependencies field does.

[alonsodomin]

shouldn't publish_targets here be the intersection set of these two other sets?

[grihabor]

It's not clear how users should use these 2 options. Since every deployment now has it's own publish_dependencies, shouldn't we delete the goal's publish_dependencies option?

[grihabor]

Right now we're using --no-experimental-deploy-publish-dependencies in CI, because we publish docker images in a separate step, so we don't need to do it again in deploy step. So yes, if these 2 options are going to be used together, we would need the set intersection

[lilatomic]

I think I've missed something. This is how I thought of it

• inferred_publish_targets : targets a backend infers should be published. For example, Helm can infer Docker images that need to be published before this deployment
• specified_publish_targets : manually specified dependencies that should be published before this deployment

Like for normal dependencies, backends can infer dependencies and users can manually add some. The result should be both of these (set union).

I would think that --no-experimental-deploy-publish-dependencies would turn both of these off.

[grihabor]

I think it's very confusing because deploy_subsystem.publish_dependencies and target.publish_dependencies have the same names but different meaning. It would be much better to change the field name to dependencies_to_publish or runtime_deploy_dependencies (similar to runtime_package_dependencies because these dependencies need to be deployed and present at runtime)

[grihabor]

off topic: Personally I don't like the name runtime_package_dependencies, and I would prefer dependencies_to_package and dependencies_to_publish instead

[benjyw]

This is probably fine as a stopgap, but it is yet another case where we have a "type" of dependency (runtime deps, build time deps, and so on), and I wish we had a principled way of expressing those.

[benjyw]

Actually, now that I think of it, why aren't these just regular dependencies, where we detect that the dependency is a publishable thing, and therefore know to publish?

[lilatomic]

I like the idea and in general, the implementation looks good to me. But it looks like that when the publish_dependencies field is used, then the inferred dependencies are ignored...?

I think I might be misunderstanding. But yes, dependencies in the dependencies field are not used to determine what needs to be published. Each backend can still infer publishable dependencies from those, like Helm does.

shouldn't publish_targets here be the intersection set of these two other sets?

We use the union of the 2 sets so that the backend can do its inference but users can still specify overrides with publish_dependencies

[lilatomic]

I would like to see a behavior more akin to the dependencies field. Meaning that when dependencies are explicitly provided, these are added to the set of inferred ones. Similarly, the publish_dependencies field could be use to exclude inferred ones using the special syntax !path/to:target as the dependencies field does.

Oh, that's a good idea! I'll see about implementing that.

[lilatomic]

Actually, now that I think of it, why aren't these just regular dependencies, where we detect that the dependency is a publishable thing, and therefore know to publish?

I think publishing every publishable item in a dependency would be unexpected. A few examples:

1. Terraform modules / Helm charts referenced locally : these support local filepath references, but are also (or could also be) publishable. I think it's necessary to allow for not publishing these
2. Not deployments, but Docker images which reference other images in the same repo can build off of other local images (only package) or remote versions of those images (would require pulling, and at some point a publish)

That said, we could also add that ability (probably with a toggle). I think it might be as simple as filtering all deps for unionmembership of PublishFieldSet.

[alonsodomin]

I'm afraid there is not a general solution to detecting which dependencies can be published. In the case of helm_deployment, scanning the transitive dependencies looking for publishable artifacts can lead to not only finding dependent remote (3rd party) images, but it can also lead to finding other publishable artifacts like a python package, which most likely wouldn't be expected to be published when running a deployment. Hence why the list of publishable artifacts for a Helm deployment is gathered in the Helm backend (and constrained to only local Docker images), as it's specific to the actual deploy goal implementation provided by the backend.

Worth mentioning that I always questioned this approach. The main motivation I had to implement a publish step during the deployment was to prevent getting a "deployment failed" error in the case in which the end user had forgotten to publish the images first (plus the fact that it sort of make sense as part of a Continuous Delivery workflow). This is very handy but the question remained: show actually Pants do that? What if end users want to separate the two steps (publishing and deploying)? How bad would be the end user experience if the deploy goal didn't publish anything and failed telling the users that some of the dependencies need to be publish first?

Since this goal was added we already added a flag to disable the publishing part of the process, meaning that there is a use case already for separating the two steps. So that makes me think probably the best direction is to remove the functionality as it could be that it's more intuitive for the end user that the deploy goal only deploys stuff and does nothing else. This would also simplify the implementation a lot but I believe we need some kind of user feedback here to properly understand how the majority of end users would like to use this feature.