Docker: Support multiple cache_from values for docker_image.

[riisi]

Fixes #20596

[kaos]

Nice.

[huonw]

Thanks for taking this on!

[huonw]

Does this old value continue to work? Or does it become a hard error after this change?

[riisi]

It will be a hard error as it is validating against a list intarget.py below

[huonw]

Ah I see. That's potentially something we should try to improve, given this is a feature released as stable and even highlighted in release notes (e.g. https://www.pantsbuild.org/blog/2024/01/24/pants-2-19#docker-improvements), so making a change like this would mean that any users who've adopted it will have a harder time upgrading.

Do we have any path for doing this with some sort of a deprecation and guidance for users? I can think of:

• add this as a field with a new name and deprecate the old one
• allow it to be either the dict or a list of such (and can emit deprecation warnings about the dict version, to eventually be able to remove that code)
• anything else?

[riisi]

Yes, I was wondering what we want to do / usually do there.

Probably the easiest is to modify the ListOfDictStringToStringField to accept a single dict - I think that would be reasonable in general.

I'm not sure if we would want a deprecation - or just always allow both a single dict / list of dicts.

[kaos]

Wow, nice catch @huonw

I agree that translating a dict value into a single element list wrapping the dict is a sensible thing to do.

Question is if we want to accept this as a feature long term, or if we want to issue a warning doing so, to urge users to explicitly use a single value list?

I think supporting this as a feature does align with how list options are supported on the cli. I.e. --flag=1 --flag=2 collects into a list option of [1, 2]. It's not the same, for sure, but the semantics are close enough I think.

[kaos]

Thanks :)

[huonw]

Nice! Thanks for adjusting to be backwards compatible.

Two questions:

• there's some docs using this field, do we need to update them, or any other docs?

  pants/docs/docs/docker/index.mdx

  Lines 214 to 224 in b8b3c72

  	docker_image(
  	name="with-local-cache-backend",
  	cache_to={
  	"type": "local",
  	"dest": "/tmp/docker-cache/pants-example"
  	},
  	cache_from={
  	"type": "local",
  	"src": "/tmp/docker-cache/pants-example"
  	}
  	)

• does the --cache-to flag allow being passed multiple times too?

[riisi]

@huonw Good catch on docs - crossed my mind earlier but I forgot.

Hopefully the update is sufficient. cache_to does not support multiple destinations in Docker - that's in the linked Docker docs.

[huonw]

Looks great.

cache_to does not support multiple destinations in Docker - that's in the linked Docker docs

Ah, good catch, thanks. Those docs appear to be a bit ambiguous or maybe out of date? They link to moby/buildkit#3024 which is merged. Someone else has already noticed this confusion in docker/docs#18590. Happy for Pants to just follow exactly what's documented though, and add support for multiple cache-to if/when that becomes clearly supported by docker itself.

[riisi]

Oh that's interesting. I'd not come across that.
At least it would be fairly easy to add.