Simplify how we handle cache-from values when doing a pull. #137
Conversation
| @@ -260,18 +260,15 @@ async function buildAndPushImageWorkerAsync( | |||
| } | |||
|
|
|||
| // If the container specified a cacheFrom parameter, first set up the cached stages. | |||
| let cacheFrom = Promise.resolve<string[] | undefined>(undefined); | |||
| let cacheFrom: string[] = []; | |||
There was a problem hiding this comment.
first, passing around 'undefined' just made later code more complex. We can instead just pass around an empty array. The later code already checks that the array is non-empty before proceeding.
| if (pullFromCache) { | ||
| const dockerBuild = <pulumi.UnwrappedObject<DockerBuild>>pathOrBuild; | ||
| const cacheFromParam = (typeof dockerBuild.cacheFrom === "boolean" ? {} : dockerBuild.cacheFrom) || {}; | ||
| cacheFrom = pullCacheAsync(baseImageName, cacheFromParam, repositoryUrl, logResource); | ||
| cacheFrom = await pullCacheAsync(baseImageName, cacheFromParam, repositoryUrl, logResource); |
There was a problem hiding this comment.
second, concurrent execution of docker pull wasn't buying us anything. We would immediately get to a point where we would just await this promise unilaterally before proceeding. So it's not like this allowed us any extra performance. It was just extra complexity.
There was a problem hiding this comment.
note: there is an opportunity for concurrency in that we can pull on all the different cache stages concurrently. However, we already don't do that today. And, if we did, it would be entirely in the body of pullCacheAsync.
I'm not going to make that change as i don't want that to cause any potential problems in case there's an issue with too many docker commands running concurrently, or interacting with some repository.
sdk/nodejs/docker.ts
Outdated
| if (cacheFromImages && cacheFromImages.length) { | ||
| buildArgs.push(...["--cache-from", cacheFromImages.join()]); | ||
| if (cacheFrom.length) { | ||
| buildArgs.push(...["--cache-from", cacheFrom.join()]); | ||
| } |
There was a problem hiding this comment.
this is where we effectively immediately did the awaiting of cacheFrom thus making the concurrency pointless. It's not like there was other expensive work we were doing in the meantime. We were simply building up the args to then just pass to docker build. Since that build had a dep on this, it's not like we had any benefit here. (i.e. it's not like we could kick off the build concurrently while this was executing).
CyrusNajmabadi
added
the
impact/no-changelog-required
| buildArgs.push(...["--cache-from", cacheFromImages.join()]); | ||
| } | ||
| if (cacheFrom.length) { | ||
| buildArgs.push(...["--cache-from", cacheFrom.join()]); | ||
| } |
There was a problem hiding this comment.
the check of build.cacheFrom is unnecessary. We would only have a non-empty cacheFrom value if build.cacheFrom was there.
|
Moving forward with this change. Have explained the logic behind it. LMK if there are any changes you'd like to see. |
No description provided.