api/types/image: move image options from api to client by austinvazquez · Pull Request #50776 · moby/moby

austinvazquez · 2025-08-20T16:07:43Z

- What I did
Partial for #50740

This change moves the image options out of the api into the client module.

- How I did it
Move the image options file from the api module to the client and reconciled the changes needed to be functional.

- How to verify it

- Human readable description for the release notes

api/types/image: move image options out to the client

- A picture of a cute animal (not mandatory but encouraged)

austinvazquez · 2025-08-20T16:13:33Z

Opened as draft to run integration tests with just the move as this move include multiple types compare to the others. Still need to rename the types for image specifier.

At the same time, I see some non-exported client types whose purpose was is to wrap the option types from api and provide functional style options. Going to look if this style is more preferable than providing options via input struct.

austinvazquez · 2025-08-20T21:29:55Z

Moving this one out of draft; ready for review.

I spent some time playing with refactoring the options into subpackages would provide better code management. The challenge I ran into is 1. the client's interface is split between option struct definitions and functional style options and 2. as the options are part of the client's interface subpackages didn't help reduce the package's interface.

vvoland · 2025-08-21T10:30:17Z

In general we want to move to the functional style options (like in ImageLoad, ImageSave, ImageInspect, ImageHistory).

thaJeztah · 2025-08-21T10:44:34Z

daemon/images/image_delete.go

-func (i *ImageService) ImageDelete(ctx context.Context, imageRef string, options imagetypes.RemoveOptions) ([]imagetypes.DeleteResponse, error) {
+func (i *ImageService) ImageDelete(ctx context.Context, imageRef string, options client.ImageRemoveOptions) ([]imagetypes.DeleteResponse, error) {


We should avoid having the daemon / backend depend on the client options. I think the idea here originally was (as they were in the API) to just unmarshal to the option and re-use the same type for "symmetry", i.e.

client.ImageDelete(<RemoveOptions>)

-> API request

-> API server unmarshals request (but in some cases: query-args + body) -> <RemoveOptions>

-> call backend / daemon with <RemoveOptions>

However, that was in a situation where daemon + api + client were all in the same module, so implicitly coupled all of those.

Now that they're separate modules, we should account for them having a different "stability"; i.e., the daemon or backend may add new features, but that not necessarily impacts the "shape" of the API. Or the reverse; a new API feature was added, but the daemon doesn't need changes.

Also as @vvoland mentions; ultimately, we want to move towards functional arguments, so the client structs may disappear at some point.

One thing we should look at though;

if more types must be added for the different backends, the list of options (Image, Container, Volume) for the backend will grow.

To some extent, we tried to implement the daemon with "services" (ImageService, ContainerService, VolumeService) - but not always did a great job at splitting those

❓ as those services may evolve (e.g. ImageService for containerd snapshotters to start taking a different shape than ImageService for graph-drivers), should we organise the backend-types into those services?

☝️ i.e., backend/images or backend/imagebackend or backend/imageservice

☝️ and then put these types in those packages? (instead of prefixing each with Image|Container|Volume

thaJeztah · 2025-08-21T10:48:51Z

In general we want to move to the functional style options (like in ImageLoad, ImageSave, ImageInspect, ImageHistory).

💯 yup. I think as an intermediate step, moving these types should be OK, but we should starting making a good design for those functional arguments; i.e. try to make a boilerplate and some guidelines, so that we take a consistent approach.

Functional arguments are great, but there's multiple approaches to implement them, so we need to do some good thinking on taking the right approach that helps us remaining module compatibility. I recently watched a good talk about this, which captured quite some of the pitfalls of functional arguments, and included some good design patterns; I need to look it up again, but we can discuss those.

thaJeztah · 2025-08-21T10:57:02Z

Perhaps we need some checks in CI to make sure we don't accidentally couple modules where they shouldn't be coupled; for example;

we don't want the daemon/ itself to be importing client. But (as it implements the API server), it should be OK for it to import the API module (which, of course should be SemVer, so can use latest version).
the integration/ suites are a bit more fuzzy, and maybe because they're a bit of a "mix" or "e2e" (API) tests and integration tests; many parts are "blackbox" testing the daemon (API behavior), and for that, should be ok to import client packages.
but there's also parts that need to import the daemon/ (e.g. networking tests testing behavior of the various drivers).

daemon/containerd/image_list.go

thaJeztah · 2025-08-22T22:11:23Z

This one needs a rebase, @austinvazquez

thaJeztah

Thanks! Overall this looks good. Some suggestions;

I think it would be a bit cleaner to have the move + rename commits together (instead of first moving to client, then adding the Image prefix); it would make the change more atomic.
Same (if possible) with the server-change to decouple; that's assuming Git doesn't get confused (which it may get for some that are in their own file), but it would be "move (e.g.) api/types/images.RemoveOptions to client.ImageRemoveOptionis" and create daemon/server/imagebackend.RemoveOptions.
That way each commit would give all the changes related to the move / refactor for each type.

Also left some questions on embedding/merging the apiOptions fields. Currently it looks fine, and it's a non-exported type, so we could still change of course, but wondering if we need some design pattern for these; which could be to keep the current approach for now (keep it as a field), then we can still decide whether embedding/merging or keeping them separate is better.

thaJeztah · 2025-08-23T12:41:55Z

client/image_load_opts.go

 func ImageLoadWithQuiet(quiet bool) ImageLoadOption {
 	return imageLoadOptionFunc(func(opt *imageLoadOpts) error {
-		opt.apiOptions.Quiet = quiet
+		opt.Quiet = quiet
 		return nil


We should still fix this; it doesn't really make sense to have quiet as API feature; or at least, it really feels more appropriate to be just "presentation", so to be handled on the client side, with the API responding as usual.

thaJeztah · 2025-08-23T12:44:08Z

client/image_load_opts.go

 type imageLoadOpts struct {
-	apiOptions LoadOptions
+	// Quiet suppresses progress output
+	Quiet bool


Could use input from @vvoland on this; slightly wondering if there would still be benefits to

keep fields themselves non-exported

have separation between client-side options, and options to be sent to the API

(also considering my other comment about possibly moving Quiet to a client-side option for newer API versions)

(i.e., converge on a struct embedding api-opts and client-opts)

I haven't given this a lot of thought yet though!

Sounds good, I believe I reverted them all now, but if I missed one. LMK :)

client/image_save_opts.go

client/image_history_opts.go

client/image_inspect_opts.go

austinvazquez · 2025-08-26T15:04:34Z

@thaJeztah , I touched up the pull request to squash the move/rename commits into a single commit and reverted the change that touched the apiOptions field. Maybe offline we can chat about the direction of the functional args in the client alongside @vvoland.

client/image_inspect_opts.go

Signed-off-by: Austin Vazquez <austin.vazquez@docker.com>

thaJeztah

LGTM, thx! 🙏

austinvazquez force-pushed the move-image-options-from-api-to-client branch from 46b01c8 to 5af9062 Compare August 20, 2025 20:27

austinvazquez added area/api API impact/api kind/refactor PR's that refactor, or clean-up code area/go-sdk impact/go-sdk Noteworthy (compatibility changes) in the Go SDK labels Aug 20, 2025

austinvazquez marked this pull request as ready for review August 20, 2025 21:30

austinvazquez changed the title ~~[wip] api/types/image: move image options from api to client~~ api/types/image: move image options from api to client Aug 20, 2025

austinvazquez requested review from corhere, dmcgowan and thaJeztah August 20, 2025 21:31

thaJeztah reviewed Aug 21, 2025

View reviewed changes

daemon/containerd/image_list.go Outdated Show resolved Hide resolved

austinvazquez force-pushed the move-image-options-from-api-to-client branch from 9efbd23 to e172048 Compare August 21, 2025 17:08

austinvazquez added the release-blocker PRs we want to block a release on label Aug 21, 2025

austinvazquez added this to the 29.0.0 milestone Aug 21, 2025

austinvazquez force-pushed the move-image-options-from-api-to-client branch from e172048 to f946ae8 Compare August 23, 2025 00:39

thaJeztah reviewed Aug 23, 2025

View reviewed changes

austinvazquez force-pushed the move-image-options-from-api-to-client branch from f946ae8 to bc53816 Compare August 26, 2025 12:24

thaJeztah reviewed Aug 26, 2025

View reviewed changes

client/image_history_opts.go Show resolved Hide resolved

thaJeztah reviewed Aug 26, 2025

View reviewed changes

client/image_inspect_opts.go Outdated Show resolved Hide resolved

austinvazquez force-pushed the move-image-options-from-api-to-client branch from bc53816 to 775da78 Compare August 26, 2025 15:00

dmcgowan approved these changes Aug 26, 2025

View reviewed changes

thaJeztah reviewed Aug 26, 2025

View reviewed changes

client/image_inspect_opts.go Show resolved Hide resolved

austinvazquez force-pushed the move-image-options-from-api-to-client branch 2 times, most recently from 4305b7a to c8ccfd7 Compare August 26, 2025 20:32

api/types/image: move image option types to client

853aed1

Signed-off-by: Austin Vazquez <austin.vazquez@docker.com>

austinvazquez force-pushed the move-image-options-from-api-to-client branch from c8ccfd7 to 853aed1 Compare August 26, 2025 20:38

thaJeztah approved these changes Aug 26, 2025

View reviewed changes

thaJeztah merged commit 6288414 into moby:master Aug 26, 2025
243 of 248 checks passed

austinvazquez deleted the move-image-options-from-api-to-client branch August 27, 2025 02:02

This was referenced Sep 5, 2025

Prepare release notes for api/v1.52.0-beta.0 #50904

Merged

Prepare release notes for client/v0.1.0-beta.0 #50913

Merged

dmcgowan mentioned this pull request Sep 24, 2025

Prepare release notes for api/v1.52.0-rc.0 #51030

Closed

		func (i *ImageService) ImageDelete(ctx context.Context, imageRef string, options imagetypes.RemoveOptions) ([]imagetypes.DeleteResponse, error) {
		func (i *ImageService) ImageDelete(ctx context.Context, imageRef string, options client.ImageRemoveOptions) ([]imagetypes.DeleteResponse, error) {

Conversation

austinvazquez commented Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

austinvazquez commented Aug 20, 2025

Uh oh!

austinvazquez commented Aug 20, 2025

Uh oh!

vvoland commented Aug 21, 2025

Uh oh!

thaJeztah Aug 21, 2025

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Aug 21, 2025

Uh oh!

thaJeztah commented Aug 21, 2025

Uh oh!

Uh oh!

thaJeztah commented Aug 22, 2025

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah Aug 23, 2025

Choose a reason for hiding this comment

Uh oh!

thaJeztah Aug 23, 2025

Choose a reason for hiding this comment

Uh oh!

austinvazquez Aug 26, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

austinvazquez commented Aug 26, 2025

Uh oh!

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

austinvazquez commented Aug 20, 2025 •

edited

Loading