client.ImagePull needs a JSON message type

[tamird]

Currently the only way to deserialize events from the io.ReadCloser returned from (*Client).ImagePull is to use github.com/docker/docker/pkg/jsonmessage.JSONMessage which forces a dependency on the docker repo. Here's what code currently has to look like:

    rc, err := cli.ImagePull(context.Background(), types.ImagePullOptions{
        ImageID: containerConfig.Image,
        Tag:     tag,
    }, nil)
    if err != nil {
        return err
    }
    defer rc.Close()
    dec := json.NewDecoder(rc)
    for {
        var message jsonmessage.JSONMessage
        if err := dec.Decode(&message); err != nil {
            if err == io.EOF {
                break
            }
            return err
        }
        log.Infof("ImagePull response: %s", message)
    }

Please introduce a new type in the engine-api repo (akin to github.com/docker/engine-api/types/events.Message) that would allow deserializing these events.

[vdemeester]

I guess we could bring JSONMessage to the engine-api, @calavera @MHBauer wdyt ?

[tamird]

github.com/docker/engine-api/types/events.Message contains a comment that indicates some deprecated fields are copied from JSONMessage. Is there an analogous custom message type planned for ImagePull?

type Message struct {
    // Deprecated information from JSONMessage.
    // With data only in container events.
    ...
}

[calavera]

I'd rather not bring JSONMessage here. There are a few packages in docker that depend on it, which would create a weird dependency.

Personally, I think JSONMessage is a bad generalization of a bunch of things we use in docker. I'd rather have a clean implementation that's independent from Docker's package.

[LK4D4]

I agree, JSONMessage is awful.

[stevvooe]

This should be hidden behind the Go API. One should not be deserializing a raw io.ReadCloser. The signature should be something like:

ImagePull(ctx context.Context, name string, opts ImagePullOptions) (Progress, error)

Where Progress is some structure reporting the progress of the pull.

[Random-Liu]

Hi, all! We plan to start using this official api. Just wanna know how and when this will be solved? :)

[stevvooe]

I have a helper function with the following signature in my local project using an api similar to this (events):

(*client).Events(ctx context.Context, options types.EventOptions) (messages <-events.Message, errs chan error)

This can be consumed with the following Go code:

ctx, cancel := context.WithCancel(ctx)

msgs, errs := client.Events(ctx, opts)
select {
case msg := <-msgs:
   if terminal(msg) {
     cancel() // canceling context shuts down channel
     return errTerminal
   }
case err := <- errs:
   // errs channel is closed or receives an error.
   // if err != nil, we msgs will get no new events.
   return err
}

I propose we follow a similar convention for the ImagePush/Pull API:

(*client).ImagePull(ctx context.Context, options types.EventOptions) (messages <-types.Progress, errs chan error)

To complement this, we need to define a Progress type. The following is the output of progress from the docker API:

{"status":"Downloading","progressDetail":{"current":5399737,"total":65687381},"progress":"[====\u003e                                              ]   5.4 MB/65.69 MB","id":"203137e8afd5"}

While we do use this in the client, we probably shouldn't be sending this much data (will require an API revision) and the actual bar should be calculated on the client-side. We can propose the minimal following Go API necessary for progress reporting:

type Progress struct {
    Resource Resource
    Status string
    Current int64
    Total   int64 
    Start   int64 
}

type Resource struct {
    Type string
    ID string
    Name string
}

Progress becomes simply the integer fields from "progressDetail" and we get a portable description of a resource, which can be used to report progress on other kinds of asynchronous operations. The contents of "progress" can be recalculated from the provided fields (using text/template for clarity):

tmpl := `{{.Resource.ID}}: {{.Status}} {{.|statusBar}} {{.Current | human}}/{{.Total | human}}`

This provides the benefit of having a future-proof type for such API methods, while avoiding dependence on JSONMessage. Eventually, we can drop the progress field from the json and reduce the amount of data sent over the docker API.

[calavera]

@stevvooe all that sounds awesome to me.

[stevvooe]

@calavera I'm going to wire this up nearly identically in my current project, and then come back with a PR if it works out.

[tamird]

@stevvooe any progress on this?

[stevvooe]

@tamird #315 is the closest to providing progress here.

If someone wants to take on the design from https://github.com/docker/engine-api/issues/89#issuecomment-198570221, that would be great, but I can probably get to this in the next month or so.