-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Simplify how we handle cache-from values when doing a pull. #137
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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[] = []; | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. second, concurrent execution of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
|
||
// Next, build the image. | ||
const {imageId, stages} = await buildImageAsync(baseImageName, pathOrBuild, logResource, cacheFrom); | ||
if (imageId === undefined) { | ||
throw new Error("Internal error: docker build did not produce an imageId."); | ||
} | ||
|
||
// Generate a name that uniquely will identify this built image. This is similar in purpose to | ||
// the name@digest form that can be normally be retrieved from a docker repository. However, | ||
|
@@ -332,11 +329,11 @@ async function pullCacheAsync( | |
imageName: string, | ||
cacheFrom: pulumi.Unwrap<CacheFrom>, | ||
repoUrl: string, | ||
logResource: pulumi.Resource): Promise<string[] | undefined> { | ||
logResource: pulumi.Resource): Promise<string[]> { | ||
|
||
// Ensure that we have a repository URL. If we don't, we won't be able to pull anything. | ||
if (!repoUrl) { | ||
return undefined; | ||
return []; | ||
} | ||
|
||
pulumi.log.debug(`pulling cache for ${imageName} from ${repoUrl}`, logResource); | ||
|
@@ -373,7 +370,7 @@ async function buildImageAsync( | |
imageName: string, | ||
pathOrBuild: string | pulumi.Unwrap<DockerBuild>, | ||
logResource: pulumi.Resource, | ||
cacheFrom: Promise<string[] | undefined>): Promise<BuildResult> { | ||
cacheFrom: string[]): Promise<BuildResult> { | ||
|
||
let build: pulumi.Unwrap<DockerBuild>; | ||
if (typeof pathOrBuild === "string") { | ||
|
@@ -436,7 +433,7 @@ async function buildImageAsync( | |
async function dockerBuild( | ||
imageName: string, | ||
build: pulumi.Unwrap<DockerBuild>, | ||
cacheFrom: Promise<string[] | undefined>, | ||
cacheFrom: string[], | ||
logResource: pulumi.Resource, | ||
target?: string): Promise<void> { | ||
|
||
|
@@ -453,11 +450,8 @@ async function dockerBuild( | |
if (build.target) { | ||
buildArgs.push(...["--target", build.target]); | ||
} | ||
if (build.cacheFrom) { | ||
const cacheFromImages = await cacheFrom; | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the check of |
||
if (build.extraOptions) { | ||
buildArgs.push(...build.extraOptions); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.