Skip to content

Commit 3db206d

Browse files
CarmenPopoviciuCarmenPopoviciu
authored
feat(containers-shared): Move isDockerfile utility function to containers-shared (#9577)
* feat(containers-shared): Move `isDockerfile` utility function to `containers-shared` * feedback fixes
1 parent c117904 commit 3db206d

File tree

10 files changed

+210
-92
lines changed

10 files changed

+210
-92
lines changed

‎packages/containers-shared/package.json‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,14 +16,19 @@
1616
"author": "[email protected]",
1717
"scripts": {
1818
"check:lint": "eslint src --ext ts",
19-
"check:type": "tsc"
19+
"check:type": "tsc -p ./tsconfig.json && pnpm run type:tests",
20+
"test": "vitest",
21+
"test:ci": "pnpm run test run",
22+
"test:watch": "pnpm run test --testTimeout=50000 --watch",
23+
"type:tests": "tsc -p ./tests/tsconfig.json"
2024
},
2125
"devDependencies": {
2226
"@cloudflare/eslint-config-worker": "workspace:*",
2327
"@cloudflare/workers-tsconfig": "workspace:*",
2428
"@types/node": "catalog:default",
2529
"typescript": "catalog:default",
26-
"undici": "catalog:default"
30+
"undici": "catalog:default",
31+
"vitest": "catalog:default"
2732
},
2833
"engines": {
2934
"node": ">20.0.0"

‎packages/containers-shared/src/utils.ts‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { spawn, StdioOptions } from "child_process";
2+
import { existsSync, statSync } from "fs";
23

34
/** helper for simple docker command call that don't require any io handling */
45
export const runDockerCmd = async (
@@ -40,3 +41,48 @@ export const verifyDockerInstalled = async (dockerPath: string) => {
4041
);
4142
}
4243
};
44+
45+
export function isDir(path: string) {
46+
const stats = statSync(path);
47+
return stats.isDirectory();
48+
}
49+
50+
export const isDockerfile = (image: string): boolean => {
51+
// TODO: move this into config validation
52+
if (existsSync(image)) {
53+
if (isDir(image)) {
54+
throw new Error(
55+
`${image} is a directory, you should specify a path to the Dockerfile`
56+
);
57+
}
58+
return true;
59+
}
60+
61+
const errorPrefix = `The image "${image}" does not appear to be a valid path to a Dockerfile, or a valid image registry path:\n`;
62+
// not found, not a dockerfile, let's try parsing the image ref as an URL?
63+
try {
64+
new URL(`https://${image}`);
65+
} catch (e) {
66+
if (e instanceof Error) {
67+
throw new Error(errorPrefix + e.message);
68+
}
69+
throw e;
70+
}
71+
const imageParts = image.split("/");
72+
73+
if (!imageParts[imageParts.length - 1].includes(":")) {
74+
throw new Error(
75+
errorPrefix +
76+
`If this is an image registry path, it needs to include at least a tag ':' (e.g: docker.io/httpd:1)`
77+
);
78+
}
79+
80+
// validate URL
81+
if (image.includes("://")) {
82+
throw new Error(
83+
errorPrefix +
84+
`Image reference should not include the protocol part (e.g: docker.io/httpd:1, not https://docker.io/httpd:1)`
85+
);
86+
}
87+
return false;
88+
};

‎packages/containers-shared/tests/helpers/run-in-tmp-dir.ts‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
/**
2+
* Based off of `wrangler/src/__tests__/helpers/run-in-tmp.ts`
3+
*
4+
* TODO: ideally we'd want this sort of helper functions to live
5+
* in a separate package in workers-sdk`, and be shared across
6+
* our packages
7+
*/
8+
9+
import * as fs from "node:fs";
10+
import os from "node:os";
11+
import * as path from "node:path";
12+
import { vi } from "vitest";
13+
14+
const originalCwd = process.cwd();
15+
16+
export function runInTempDir({ homedir } = { homedir: "./home" }) {
17+
let tmpDir: string;
18+
19+
beforeEach(() => {
20+
// Use realpath because the temporary path can point to a symlink rather than the actual path.
21+
tmpDir = fs.realpathSync(
22+
fs.mkdtempSync(path.join(os.tmpdir(), "containers-shared-tests"))
23+
);
24+
25+
process.chdir(tmpDir);
26+
process.env.PWD = tmpDir;
27+
28+
// The path that is returned from `homedir()` should be absolute.
29+
const absHomedir = path.resolve(tmpDir, homedir);
30+
31+
// Override where the home directory is so that we can write our own user config,
32+
// without destroying the real thing.
33+
fs.mkdirSync(absHomedir, { recursive: true });
34+
35+
// Note it is very important that we use the "default" value from "node:os" (e.g. `import os from "node:os";`)
36+
// rather than an alias to the module (e.g. `import * as os from "node:os";`).
37+
// This is because the module gets transpiled so that the "method" `homedir()` gets converted to a
38+
// getter that is not configurable (and so cannot be spied upon).
39+
vi.spyOn(os, "homedir")?.mockReturnValue(absHomedir);
40+
});
41+
42+
afterEach(() => {
43+
if (fs.existsSync(tmpDir)) {
44+
process.chdir(originalCwd);
45+
process.env.PWD = originalCwd;
46+
try {
47+
fs.rmSync(tmpDir, { recursive: true, force: true });
48+
} catch (e) {
49+
// Best effort - try once then just move on - they are only temp files after all.
50+
// It seems that Windows doesn't let us delete this, with errors like:
51+
//
52+
// Error: EBUSY: resource busy or locked, rmdir 'C:\Users\RUNNER~1\AppData\Local\Temp\wrangler-modules-pKJ7OQ'
53+
// ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
54+
// Serialized Error: {
55+
// "code": "EBUSY",
56+
// "errno": -4082,
57+
// "path": "C:\Users\RUNNER~1\AppData\Local\Temp\wrangler-modules-pKJ7OQ",
58+
// "syscall": "rmdir",
59+
// }
60+
}
61+
}
62+
});
63+
}

‎packages/containers-shared/tests/tsconfig.json‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
{
2+
"extends": "@cloudflare/workers-tsconfig/tsconfig.json",
3+
"compilerOptions": {
4+
"module": "CommonJS",
5+
"types": ["node", "vitest/globals"]
6+
},
7+
"include": ["**/*.ts"]
8+
}

‎packages/containers-shared/tests/utils.test.ts‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
import { mkdirSync, rmSync, writeFileSync } from "fs";
2+
import { isDockerfile } from "./../src/utils";
3+
import { runInTempDir } from "./helpers/run-in-tmp-dir";
4+
5+
describe("isDockerfile", () => {
6+
const dockerfile =
7+
'FROM node:18\nWORKDIR /app\nCOPY . .\nRUN npm install\nCMD ["node", "index.js"]';
8+
9+
runInTempDir();
10+
11+
beforeEach(() => {
12+
mkdirSync("./container-context");
13+
writeFileSync("./container-context/Dockerfile", dockerfile);
14+
});
15+
16+
it("should return true if given a valid dockerfile path", async () => {
17+
expect(isDockerfile("./container-context/Dockerfile")).toBe(true);
18+
});
19+
20+
it("should return false if given a valid image registry path", async () => {
21+
expect(isDockerfile("docker.io/httpd:1")).toBe(false);
22+
});
23+
24+
it("should error if given a non existant dockerfile", async () => {
25+
expect(() => isDockerfile("./FakeDockerfile"))
26+
.toThrowErrorMatchingInlineSnapshot(`
27+
[Error: The image "./FakeDockerfile" does not appear to be a valid path to a Dockerfile, or a valid image registry path:
28+
If this is an image registry path, it needs to include at least a tag ':' (e.g: docker.io/httpd:1)]
29+
`);
30+
});
31+
32+
it("should error if given a directory instead of a dockerfile", async () => {
33+
expect(() => isDockerfile("./container-context"))
34+
.toThrowErrorMatchingInlineSnapshot(`
35+
[Error: ./container-context is a directory, you should specify a path to the Dockerfile]
36+
`);
37+
});
38+
39+
it("should error if image registry reference contains the protocol part", async () => {
40+
expect(() => isDockerfile("http://example.com/image:tag"))
41+
.toThrowErrorMatchingInlineSnapshot(`
42+
[Error: The image "http://example.com/image:tag" does not appear to be a valid path to a Dockerfile, or a valid image registry path:
43+
Image reference should not include the protocol part (e.g: docker.io/httpd:1, not https://docker.io/httpd:1)]
44+
`);
45+
});
46+
47+
it("should error if image registry reference does not contain a tag", async () => {
48+
expect(() => isDockerfile("docker.io/httpd"))
49+
.toThrowErrorMatchingInlineSnapshot(`
50+
[Error: The image "docker.io/httpd" does not appear to be a valid path to a Dockerfile, or a valid image registry path:
51+
If this is an image registry path, it needs to include at least a tag ':' (e.g: docker.io/httpd:1)]
52+
`);
53+
});
54+
});

‎packages/containers-shared/vitest.config.mts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
import { defineConfig } from "vitest/config";
2+
3+
export default defineConfig({
4+
test: {
5+
testTimeout: 15_000,
6+
retry: 0,
7+
include: ["**/tests/**/*.test.ts"],
8+
globals: true,
9+
},
10+
});

‎packages/wrangler/src/__tests__/cloudchamber/build.test.ts‎

Lines changed: 0 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ import {
88
} from "@cloudflare/containers-shared";
99
import { ensureDiskLimits } from "../../cloudchamber/build";
1010
import { resolveAppDiskSize } from "../../cloudchamber/common";
11-
import { isDockerfile } from "../../cloudchamber/deploy";
1211
import { type ContainerApp } from "../../config/environment";
1312
import { UserError } from "../../errors";
1413
import { mockAccountId, mockApiToken } from "../helpers/mock-account-id";
@@ -175,43 +174,6 @@ describe("buildAndMaybePush", () => {
175174
).rejects.toThrow(new UserError(errorMessage));
176175
});
177176

178-
describe("isDockerfile", () => {
179-
it("should return true if given a valid dockerfile path", async () => {
180-
expect(isDockerfile("./container-context/Dockerfile")).toBe(true);
181-
});
182-
it("should return false if given a valid image registry path", async () => {
183-
expect(isDockerfile("docker.io/httpd:1")).toBe(false);
184-
});
185-
186-
it("should error if given a non existant dockerfile", async () => {
187-
expect(() => isDockerfile("./FakeDockerfile"))
188-
.toThrowErrorMatchingInlineSnapshot(`
189-
[Error: The image "./FakeDockerfile" does not appear to be a valid path to a Dockerfile, or a valid image registry path:
190-
If this is an image registry path, it needs to include at least a tag ':' (e.g: docker.io/httpd:1)]
191-
`);
192-
});
193-
it("should error if given a directory instead of a dockerfile", async () => {
194-
expect(() => isDockerfile("./container-context"))
195-
.toThrowErrorMatchingInlineSnapshot(`
196-
[Error: ./container-context is a directory, you should specify a path to the Dockerfile]
197-
`);
198-
});
199-
it("should error if image registry reference contains the protocol part", async () => {
200-
expect(() => isDockerfile("http://example.com/image:tag"))
201-
.toThrowErrorMatchingInlineSnapshot(`
202-
[Error: The image "http://example.com/image:tag" does not appear to be a valid path to a Dockerfile, or a valid image registry path:
203-
Image reference should not include the protocol part (e.g: docker.io/httpd:1, not https://docker.io/httpd:1)]
204-
`);
205-
});
206-
it("should error if image registry reference does not contain a tag", async () => {
207-
expect(() => isDockerfile("docker.io/httpd"))
208-
.toThrowErrorMatchingInlineSnapshot(`
209-
[Error: The image "docker.io/httpd" does not appear to be a valid path to a Dockerfile, or a valid image registry path:
210-
If this is an image registry path, it needs to include at least a tag ':' (e.g: docker.io/httpd:1)]
211-
`);
212-
});
213-
});
214-
215177
describe("ensureDiskLimits", () => {
216178
const accountBase = {
217179
limits: { disk_mb_per_deployment: 2000 },

‎packages/wrangler/src/cloudchamber/build.ts‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,12 @@
1-
import { existsSync, statSync } from "fs";
1+
import { existsSync } from "fs";
22
import { join } from "path";
33
import {
44
constructBuildCommand,
55
dockerBuild,
66
dockerImageInspect,
77
dockerLoginManagedRegistry,
88
DOMAIN,
9+
isDir,
910
runDockerCmd,
1011
} from "@cloudflare/containers-shared";
1112
import {
@@ -72,11 +73,6 @@ export function pushYargs(yargs: CommonYargsArgvJSON) {
7273
.positional("TAG", { type: "string", demandOption: true });
7374
}
7475

75-
export function isDir(path: string) {
76-
const stats = statSync(path);
77-
return stats.isDirectory();
78-
}
79-
8076
export async function buildAndMaybePush(
8177
args: BuildArgs,
8278
pathToDocker: string,

‎packages/wrangler/src/cloudchamber/deploy.ts‎

Lines changed: 17 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { existsSync } from "fs";
1+
import { isDockerfile } from "@cloudflare/containers-shared";
22
import { type Config } from "../config";
33
import { type ContainerApp } from "../config/environment";
44
import { getDockerPath } from "../environment-variables/misc-variables";
@@ -7,7 +7,7 @@ import { isNonInteractiveOrCI } from "../is-interactive";
77
import { logger } from "../logger";
88
import { fetchVersion } from "../versions/api";
99
import { apply } from "./apply";
10-
import { buildAndMaybePush, isDir } from "./build";
10+
import { buildAndMaybePush } from "./build";
1111
import { fillOpenAPIConfiguration } from "./common";
1212
import type { BuildArgs } from "@cloudflare/containers-shared/src/types";
1313

@@ -18,11 +18,22 @@ export async function maybeBuildContainer(
1818
dryRun: boolean,
1919
pathToDocker: string
2020
) {
21-
if (
22-
!isDockerfile(containerConfig.image ?? containerConfig.configuration.image)
23-
) {
24-
return containerConfig.image ?? containerConfig.configuration.image;
21+
try {
22+
if (
23+
!isDockerfile(
24+
containerConfig.image ?? containerConfig.configuration.image
25+
)
26+
) {
27+
return containerConfig.image ?? containerConfig.configuration.image;
28+
}
29+
} catch (err) {
30+
if (err instanceof Error) {
31+
throw new UserError(err.message);
32+
}
33+
34+
throw err;
2535
}
36+
2637
const options = getBuildArguments(containerConfig, imageTag);
2738
logger.log("Building image", options.tag);
2839
const tag = await buildAndMaybePush(
@@ -126,43 +137,3 @@ export function getBuildArguments(
126137
args: container.image_vars,
127138
};
128139
}
129-
130-
export const isDockerfile = (image: string): boolean => {
131-
// TODO: move this into config validation
132-
if (existsSync(image)) {
133-
if (isDir(image)) {
134-
throw new UserError(
135-
`${image} is a directory, you should specify a path to the Dockerfile`
136-
);
137-
}
138-
return true;
139-
}
140-
141-
const errorPrefix = `The image "${image}" does not appear to be a valid path to a Dockerfile, or a valid image registry path:\n`;
142-
// not found, not a dockerfile, let's try parsing the image ref as an URL?
143-
try {
144-
new URL(`https://${image}`);
145-
} catch (e) {
146-
if (e instanceof Error) {
147-
throw new UserError(errorPrefix + e.message);
148-
}
149-
throw e;
150-
}
151-
const imageParts = image.split("/");
152-
153-
if (!imageParts[imageParts.length - 1].includes(":")) {
154-
throw new UserError(
155-
errorPrefix +
156-
`If this is an image registry path, it needs to include at least a tag ':' (e.g: docker.io/httpd:1)`
157-
);
158-
}
159-
160-
// validate URL
161-
if (image.includes("://")) {
162-
throw new UserError(
163-
errorPrefix +
164-
`Image reference should not include the protocol part (e.g: docker.io/httpd:1, not https://docker.io/httpd:1)`
165-
);
166-
}
167-
return false;
168-
};

‎pnpm-lock.yaml‎

Lines changed: 3 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)