Tasks that return status streams should be replaced with calls to IProgress<StreamStatus> about docker.dotnet HOT 8 CLOSED

1. Agreed, I think anything that returns Task other than the image downloads can be done this way.
2. Correct that is the right pattern for Long Running Tasks, I would love to see that change!

from docker.dotnet.

This has been merged. Self assigning to cleanup Push/PullImageAsync in the v1.13 branch given the inclusion of JSONMessage etc.

from docker.dotnet.

Instead of returning Task<Stream> we should return Task. Then add a parameter to the function that takes IProgress<PullStatus> progress. Each time that we receive the new "stream value" we deserialize it into the PullStatus and then Report() any waiter. The client looks like this:

var p = new Progress<Docker.DotNet.PullStatus>(status =>
{
// Do something with the status? Update a UI/PS/Service on the Sync Context of your choice.
});

await PullImageAsync(/*params*/, p);

PullStatus is just a moniker. If Docker always does the same status struct then we can just use that instead for all methods that return a Task<Stream> where <Stream> contains only data that is just status. IE: docker logs is not a candidate here because the logs are an actual value.

from docker.dotnet.

I updated the tile which was very misleading. My response above is what we actually want. Not to change the return type to return progress but rather pass in an IProgress of the appropriate type so we can handle the status reporting to waiters.

from docker.dotnet.

@jterry75 as mentioned on #143, Lets implement this issue.

I would like to align here the proper method signatures:

1. For methods that are today returning Task<Stream> we should do as you mentioned here, correct?
2. For methods that are long running monitoring, like the ones on MISC endpoint, which endlessly monitor events on the engine/swarm, I would suggest return an IObservable<T> where T is a structure with the general status update. That way, people don't need to await on that and instead can just subscribe. They can even use Rx to consume. As you may know, IObservable<T> is available on .Net so we don't need add any dependencies... What do you think?

When I got answer for those two I'll submit the PR.

Thanks

from docker.dotnet.

Perfect! Will do it tonight when get home. Thanks @jterry75

from docker.dotnet.

@jterry75 it is done on #147.

I decided to not use IObservable<T> because it will be too big breaking change on the public APIs.

I think IProgress<T> is enough for that case. Specially that most of the time, the events are limited to an small number.

Please review when you can so I can move to the others. Thanks! Appreciate the support! :)

from docker.dotnet.

Thanks @jterry75 let me what else todo :)

from docker.dotnet.