Skip to content

[DNM] trying generics for streams#51170

Draft
thaJeztah wants to merge 3 commits intomoby:masterfrom
thaJeztah:generics_streams
Draft

[DNM] trying generics for streams#51170
thaJeztah wants to merge 3 commits intomoby:masterfrom
thaJeztah:generics_streams

Conversation

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Oct 11, 2025

@thaJeztah thaJeztah force-pushed the generics_streams branch 2 times, most recently from ef59c6a to 3f0470a Compare October 11, 2025 14:02
"github.com/moby/moby/client/pkg/jsonmessage"
)

type PushMessage = jsonmessage.JSONMessage
Copy link
Contributor

Choose a reason for hiding this comment

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

could declare additional attributes to handle Aux raw message decoding, which are specific to push; This could avoid use of WithAuxCallback by client, having to guess the actual message types

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, possibly! This was really a quick "let's see if this would work".

I had to run out for an appointment, but will play a bit with this later.

Overall, I like what it looks like (and would indeed allow us to differentiate responses instead of "everything is a json-message".

Also; feel free to play with it yourself!

Copy link
Member Author

Choose a reason for hiding this comment

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

Pushed a quick commit to also try to make the Aux field strong typed; unit-tests looks happy, but probably could add some more to actually use the aux-message part.

@ndeloof
Copy link
Contributor

ndeloof commented Oct 16, 2025

I wonder we could refactor DisplayJSONMessages to accept a formater function to manage messages, so we can use api types (jsonstream.Progress) vs having derived types jsonmessage.JSONProgress to handle rendering

@thaJeztah thaJeztah force-pushed the generics_streams branch 2 times, most recently from 180bea4 to 174b59c Compare October 16, 2025 13:43
Comment on lines -197 to +201
func DisplayJSONMessagesStream(in io.Reader, out io.Writer, terminalFd uintptr, isTerminal bool, auxCallback func(JSONMessage)) error {
func DisplayJSONMessagesStream[A any](in io.Reader, out io.Writer, terminalFd uintptr, isTerminal bool, auxCallback func(JSONMessage[A])) error {
Copy link
Member Author

Choose a reason for hiding this comment

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

We could possibly internalise this if we want to keep the old signature (roughly); still need to look if we still need these, or if they need to be rewritten.

@thaJeztah thaJeztah force-pushed the generics_streams branch 6 times, most recently from 6165701 to 5b6e6cb Compare October 17, 2025 01:13
ID string `json:"id,omitempty"`
Error *jsonstream.Error `json:"errorDetail,omitempty"`
Aux *json.RawMessage `json:"aux,omitempty"` // Aux contains out-of-band data, such as digests for push signing and image id after building.
Aux *A `json:"aux,omitempty"` // Aux contains out-of-band data, such as digests for push signing and image id after building.
Copy link
Contributor

Choose a reason for hiding this comment

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

using the /imageBuild API, aux is raw base64 string during the build but eventually build result with id=moby.image.id and aux as a struct with ID field, so we can't use a single type Aux for the whole operation

@thaJeztah thaJeztah force-pushed the generics_streams branch 4 times, most recently from 925af2e to 6c1feac Compare October 25, 2025 00:25
This allows for a strict type to be used for different endpoints.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
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.

2 participants