Add progress events to LoadDockerStep #1960
Conversation
jib-core/src/main/java/com/google/cloud/tools/jib/docker/DockerClient.java
Show resolved
Hide resolved
| ImageTarball imageTarball = new ImageTarball(builtImage, targetImageReference); | ||
| try (ProgressEventDispatcher progressEventDispatcher = | ||
| progressEventDispatcherFactory.create( | ||
| "loading to Docker daemon", imageTarball.getSize()); |
There was a problem hiding this comment.
The only problem is that we will go over the total progress allocation, because it will also write two more files (config.json and manifest.json), whose size we cannot know ahead, unfortunately. We won't crash, as we lifted the assertion that the progress completion cannot go over the given allocation at some point due to some corner case (#1522), but still, the computation here will always be incorrect. BTW, BlobPuller uses blobSizeListener coming from ThrottledProgressEventDispatcherWrapper::setProgressTarget() to lazily set the total progress, so we can do the same, but this require more changes to ImageTarball, so I can't say I like it.
There was a problem hiding this comment.
I don't really mind that much that the calculation is incorrect as long as it's close. The layers are almost always going to be much bigger than the manifest and configuration, so the progress being reported will at least be close to accurate. It certainly doesn't matter with the plugins, since the progress bar moves too fast to be able to even tell that there's any inaccuracy, but I suppose it might matter a little bit for jib-core users if they need totally accurate progress reporting. But even then, the progress event API still isn't finalized.
There was a problem hiding this comment.
Agreed. Let's just leave a code comment that the progress will always go over the limit.
jib-core/src/main/java/com/google/cloud/tools/jib/docker/ImageTarball.java
Outdated
Show resolved
Hide resolved
Fixes #1959.