-
-
Notifications
You must be signed in to change notification settings - Fork 201
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
Add support for building a container from context archive #671
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,38 +1,24 @@ | ||
import Dockerode, { ImageBuildOptions } from "dockerode"; | ||
import byline from "byline"; | ||
import tar from "tar-fs"; | ||
import path from "path"; | ||
import { existsSync, promises as fs } from "fs"; | ||
import dockerIgnore from "@balena/dockerignore"; | ||
import { getAuthConfig } from "../../auth/get-auth-config"; | ||
import { ImageName } from "../../image-name"; | ||
import { ImageClient } from "./image-client"; | ||
import AsyncLock from "async-lock"; | ||
import { log, buildLog, pullLog } from "../../../common"; | ||
import stream from "stream"; | ||
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. this is the node stream api https://nodejs.org/api/stream.html#stream right? const stream = require('node:stream'); has this doubt 🤔 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. Yes, it is. I can change the import name with |
||
|
||
export class DockerImageClient implements ImageClient { | ||
private readonly existingImages = new Set<string>(); | ||
private readonly imageExistsLock = new AsyncLock(); | ||
|
||
constructor(protected readonly dockerode: Dockerode, protected readonly indexServerAddress: string) {} | ||
|
||
async build(context: string, opts: ImageBuildOptions): Promise<void> { | ||
async build(context: stream.Readable, opts: ImageBuildOptions): Promise<void> { | ||
try { | ||
log.debug(`Building image "${opts.t}" with context "${context}"...`); | ||
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. What's the reasoning behind moving this logic to the 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. I understand your point. I decided to raise the filtering to a higher abstraction level, recognizing that the At the time of implementation, I faced two options:
|
||
const isDockerIgnored = await this.createIsDockerIgnoredFunction(context); | ||
const tarStream = tar.pack(context, { | ||
ignore: (aPath) => { | ||
const relativePath = path.relative(context, aPath); | ||
if (relativePath === opts.dockerfile) { | ||
return false; | ||
} else { | ||
return isDockerIgnored(relativePath); | ||
} | ||
}, | ||
}); | ||
log.debug(`Building image "${opts.t}"...`); | ||
await new Promise<void>((resolve) => { | ||
this.dockerode | ||
.buildImage(tarStream, opts) | ||
.buildImage(context, opts) | ||
.then((stream) => byline(stream)) | ||
.then((stream) => { | ||
stream.setEncoding("utf-8"); | ||
|
@@ -44,27 +30,13 @@ export class DockerImageClient implements ImageClient { | |
stream.on("end", () => resolve()); | ||
}); | ||
}); | ||
log.debug(`Built image "${opts.t}" with context "${context}"`); | ||
log.debug(`Built image "${opts.t}"`); | ||
} catch (err) { | ||
log.error(`Failed to build image: ${err}`); | ||
throw err; | ||
} | ||
} | ||
|
||
private async createIsDockerIgnoredFunction(context: string): Promise<(path: string) => boolean> { | ||
const dockerIgnoreFilePath = path.join(context, ".dockerignore"); | ||
if (!existsSync(dockerIgnoreFilePath)) { | ||
return () => false; | ||
} | ||
|
||
const dockerIgnorePatterns = await fs.readFile(dockerIgnoreFilePath, { encoding: "utf-8" }); | ||
const instance = dockerIgnore({ ignorecase: false }); | ||
instance.add(dockerIgnorePatterns); | ||
const filter = instance.createFilter(); | ||
|
||
return (aPath: string) => !filter(aPath); | ||
} | ||
|
||
async exists(imageName: ImageName): Promise<boolean> { | ||
return this.imageExistsLock.acquire(imageName.string, async () => { | ||
if (this.existingImages.has(imageName.string)) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,10 @@ import { getAuthConfig, getContainerRuntimeClient, ImageName } from "../containe | |
import { getReaper } from "../reaper/reaper"; | ||
import { getDockerfileImages } from "../utils/dockerfile-parser"; | ||
import { createLabels, LABEL_TESTCONTAINERS_SESSION_ID } from "../utils/labels"; | ||
import tar from "tar-fs"; | ||
import { existsSync, promises as fs } from "fs"; | ||
import dockerIgnore from "@balena/dockerignore"; | ||
import Dockerode from "dockerode"; | ||
|
||
export type BuildOptions = { | ||
deleteOnExit: boolean; | ||
|
@@ -19,12 +23,12 @@ export class GenericContainerBuilder { | |
private target?: string; | ||
|
||
constructor( | ||
private readonly context: string, | ||
private readonly context: NodeJS.ReadableStream | string, | ||
private readonly dockerfileName: string, | ||
private readonly uuid: Uuid = new RandomUuid() | ||
) {} | ||
|
||
public withBuildArgs(buildArgs: BuildArgs): GenericContainerBuilder { | ||
public withBuildArgs(buildArgs: BuildArgs): this { | ||
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. thanks to do the return this pattern consistent here 👏 |
||
this.buildArgs = buildArgs; | ||
return this; | ||
} | ||
|
@@ -44,6 +48,12 @@ export class GenericContainerBuilder { | |
return this; | ||
} | ||
|
||
/** | ||
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. I really really really really appreciate these comments for our methods and functions but, since they are not anywhere in the library, I don't know if it would be convenient to start doing it. 🤔 |
||
* Build the image. | ||
* | ||
* @param image - The image name to tag the built image with. | ||
* @param options - Options for the build. Defaults to `{ deleteOnExit: true }`. | ||
*/ | ||
public async build( | ||
image = `localhost/${this.uuid.nextUuid()}:${this.uuid.nextUuid()}`, | ||
options: BuildOptions = { deleteOnExit: true } | ||
|
@@ -52,26 +62,49 @@ export class GenericContainerBuilder { | |
const reaper = await getReaper(client); | ||
|
||
const imageName = ImageName.fromString(image); | ||
const dockerfile = path.resolve(this.context, this.dockerfileName); | ||
|
||
const imageNames = await getDockerfileImages(dockerfile, this.buildArgs); | ||
const registryConfig = await this.getRegistryConfig(client.info.containerRuntime.indexServerAddress, imageNames); | ||
const labels = createLabels(); | ||
if (options.deleteOnExit) { | ||
labels[LABEL_TESTCONTAINERS_SESSION_ID] = reaper.sessionId; | ||
} | ||
|
||
log.info(`Building Dockerfile "${dockerfile}" as image "${imageName}"...`); | ||
await client.image.build(this.context, { | ||
let contextStream: NodeJS.ReadableStream; | ||
const imageBuildOptions: Dockerode.ImageBuildOptions = { | ||
t: imageName.string, | ||
dockerfile: this.dockerfileName, | ||
buildargs: this.buildArgs, | ||
pull: this.pullPolicy ? "true" : undefined, | ||
nocache: !this.cache, | ||
registryconfig: registryConfig, | ||
labels, | ||
target: this.target, | ||
}); | ||
}; | ||
|
||
if (typeof this.context !== "string") { | ||
contextStream = this.context; | ||
} else { | ||
// Get the registry config for the images in the Dockerfile | ||
const dockerfile = path.resolve(this.context, this.dockerfileName); | ||
const imageNames = await getDockerfileImages(dockerfile, this.buildArgs); | ||
imageBuildOptions.registryconfig = await this.getRegistryConfig( | ||
client.info.containerRuntime.indexServerAddress, | ||
imageNames | ||
); | ||
|
||
// Create a tar stream of the context directory, excluding the files that are ignored by .dockerignore | ||
const dockerignoreFilter = await newDockerignoreFilter(this.context); | ||
contextStream = tar.pack(this.context, { | ||
ignore: (aPath) => { | ||
const relativePath = path.relative(<string>this.context, aPath); | ||
if (relativePath === this.dockerfileName) { | ||
return false; | ||
} else { | ||
return dockerignoreFilter(relativePath); | ||
} | ||
}, | ||
}); | ||
} | ||
|
||
log.info(`Building Dockerfile "${this.dockerfileName}" as image "${imageName.string}"...`); | ||
await client.image.build(contextStream, imageBuildOptions); | ||
|
||
const container = new GenericContainer(imageName.string); | ||
if (!(await client.image.exists(imageName))) { | ||
|
@@ -105,3 +138,17 @@ export class GenericContainerBuilder { | |
.reduce((prev, next) => ({ ...prev, ...next }), {} as RegistryConfig); | ||
} | ||
} | ||
|
||
async function newDockerignoreFilter(context: string): Promise<(path: string) => boolean> { | ||
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. Only to curiosity in your approach, why 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. I believe class methods should access or modify the object instance. In this case, |
||
const dockerIgnoreFilePath = path.join(context, ".dockerignore"); | ||
if (!existsSync(dockerIgnoreFilePath)) { | ||
return () => false; | ||
} | ||
|
||
const dockerIgnorePatterns = await fs.readFile(dockerIgnoreFilePath, { encoding: "utf-8" }); | ||
const instance = dockerIgnore({ ignorecase: false }); | ||
instance.add(dockerIgnorePatterns); | ||
const filter = instance.createFilter(); | ||
|
||
return (aPath: string) => !filter(aPath); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,6 @@ | ||
import fs from "fs"; | ||
import path from "path"; | ||
import tar from "tar-stream"; | ||
import { GenericContainer } from "./generic-container"; | ||
import { Wait } from "../wait-strategies/wait"; | ||
import { PullPolicy } from "../utils/pull-policy"; | ||
|
@@ -103,3 +105,27 @@ describe("GenericContainer Dockerfile", () => { | |
await startedContainer.stop(); | ||
}); | ||
}); | ||
|
||
describe("GenericContainer fromContextArchive", () => { | ||
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. thanks to add tests 🎉 |
||
jest.setTimeout(180_000); | ||
|
||
const fixtures = path.resolve(__dirname, "..", "..", "fixtures", "docker"); | ||
|
||
it("should build and start", async () => { | ||
const fixturesCtx = path.resolve(fixtures, "docker"); | ||
|
||
// Generate a context stream with a Dockerfile named "alpine.Dockerfile" | ||
const tarStream = tar.pack(); | ||
tarStream.entry({ name: "alpine.Dockerfile" }, fs.readFileSync(path.join(fixturesCtx, "Dockerfile"))); | ||
tarStream.entry({ name: "index.js" }, fs.readFileSync(path.join(fixturesCtx, "index.js"))); | ||
tarStream.entry({ name: "test.txt" }, "hello world"); | ||
tarStream.finalize(); | ||
|
||
const container = await GenericContainer.fromContextArchive(tarStream, "alpine.Dockerfile").build(); | ||
const startedContainer = await container.withExposedPorts(8080).start(); | ||
|
||
await checkContainerIsHealthy(startedContainer); | ||
|
||
await startedContainer.stop(); | ||
}); | ||
}); |
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.
tar-stream
shouldn't be in dependencies 🤔 ?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.
I only used it in the tests to generate a test tar stream. It is not used in the package's production code.