diff --git a/workflow-parser/src/model/converter/container.test.ts b/workflow-parser/src/model/converter/container.test.ts new file mode 100644 index 0000000..a57f379 --- /dev/null +++ b/workflow-parser/src/model/converter/container.test.ts @@ -0,0 +1,314 @@ +/* eslint-disable @typescript-eslint/no-non-null-assertion */ +import {nullTrace} from "../../test-utils/null-trace.js"; +import {parseWorkflow} from "../../workflows/workflow-parser.js"; +import {convertWorkflowTemplate, ErrorPolicy} from "../convert.js"; + +async function getErrors(content: string): Promise { + const result = parseWorkflow({name: "wf.yaml", content}, nullTrace); + const template = await convertWorkflowTemplate(result.context, result.value!, undefined, { + errorPolicy: ErrorPolicy.TryConversion + }); + return (template.errors ?? []).map((e: {Message: string}) => e.Message); +} + +function expectNoContainerErrors(errors: string[]): void { + const containerErrors = errors.filter(e => e.includes("Container image")); + expect(containerErrors).toHaveLength(0); +} + +function expectContainerError(errors: string[], count = 1): void { + const containerErrors = errors.filter(e => e.includes("Container image cannot be empty")); + expect(containerErrors).toHaveLength(count); +} + +describe("container image validation", () => { + describe("shorthand form", () => { + it("container: '' is silent for job container", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + container: '' + steps: + - run: echo hi`); + expectNoContainerErrors(errors); + }); + + it("container: docker:// errors", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + container: docker:// + steps: + - run: echo hi`); + expectContainerError(errors); + }); + + it("container: valid-image passes", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + container: ubuntu:16.04 + steps: + - run: echo hi`); + expectNoContainerErrors(errors); + }); + }); + + describe("mapping form", () => { + it("container image: '' errors", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + container: + image: '' + steps: + - run: echo hi`); + expectContainerError(errors); + }); + + it("container image: docker:// errors", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + container: + image: docker:// + steps: + - run: echo hi`); + expectContainerError(errors); + }); + + it("container: {} (empty object, missing image) errors", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + container: {} + steps: + - run: echo hi`); + expectContainerError(errors); + }); + + it("container image: null errors", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + container: + image: + steps: + - run: echo hi`); + expectContainerError(errors); + }); + + it("empty image with expression in other field still errors", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + container: + image: '' + options: \${{ matrix.opts }} + steps: + - run: echo hi`); + expectContainerError(errors); + }); + }); + + describe("services shorthand", () => { + it("services svc: '' errors", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + services: + svc: '' + steps: + - run: echo hi`); + expectContainerError(errors); + }); + + it("services svc: docker:// errors", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + services: + svc: docker:// + steps: + - run: echo hi`); + expectContainerError(errors); + }); + }); + + describe("services mapping", () => { + it("services svc image: '' errors", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + services: + svc: + image: '' + steps: + - run: echo hi`); + expectContainerError(errors); + }); + + it("services svc image: docker:// errors", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + services: + svc: + image: docker:// + steps: + - run: echo hi`); + expectContainerError(errors); + }); + + it("services svc: {} (empty object) errors", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + services: + svc: {} + steps: + - run: echo hi`); + expectContainerError(errors); + }); + + it("empty image with expression sibling service still errors", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + services: + svc1: + image: '' + svc2: \${{ matrix.svc }} + steps: + - run: echo hi`); + expectContainerError(errors); + }); + }); + + describe("expression safety", () => { + it("container: expression skips validation", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + container: \${{ matrix.container }} + steps: + - run: echo hi`); + expectNoContainerErrors(errors); + }); + + it("container image: expression skips validation", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + container: + image: \${{ matrix.image }} + options: --privileged + steps: + - run: echo hi`); + expectNoContainerErrors(errors); + }); + + it("container with expression key skips validation", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + container: + \${{ vars.KEY }}: ubuntu + steps: + - run: echo hi`); + expectNoContainerErrors(errors); + }); + + it("services: expression skips validation", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + services: \${{ fromJSON(inputs.services) }} + steps: + - run: echo hi`); + expectNoContainerErrors(errors); + }); + + it("services with expression alias key skips validation", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + services: + \${{ matrix.alias }}: postgres + steps: + - run: echo hi`); + expectNoContainerErrors(errors); + }); + + it("services container with expression key skips validation", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + services: + db: + \${{ vars.KEY }}: postgres + steps: + - run: echo hi`); + expectNoContainerErrors(errors); + }); + + it("container with all expression fields skips validation", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + container: + image: \${{ matrix.image }} + options: \${{ matrix.options }} + steps: + - run: echo hi`); + expectNoContainerErrors(errors); + }); + + it("services svc: expression skips validation", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + services: + db: \${{ matrix.db }} + steps: + - run: echo hi`); + expectNoContainerErrors(errors); + }); + + it("services image: expression skips validation", async () => { + const errors = await getErrors(`on: push +jobs: + build: + runs-on: linux + services: + db: + image: \${{ matrix.db_image }} + options: --health-cmd pg_isready + steps: + - run: echo hi`); + expectNoContainerErrors(errors); + }); + }); +}); diff --git a/workflow-parser/src/model/converter/container.ts b/workflow-parser/src/model/converter/container.ts index 4c4bf79..8ef9b27 100644 --- a/workflow-parser/src/model/converter/container.ts +++ b/workflow-parser/src/model/converter/container.ts @@ -3,103 +3,164 @@ import {MappingToken, SequenceToken, StringToken, TemplateToken} from "../../tem import {isString} from "../../templates/tokens/type-guards.js"; import {Container, Credential} from "../workflow-template.js"; -export function convertToJobContainer(context: TemplateContext, container: TemplateToken): Container | undefined { +const DOCKER_URI_PREFIX = "docker://"; + +function isEmptyImage(value: string): boolean { + const trimmed = value.startsWith(DOCKER_URI_PREFIX) ? value.substring(DOCKER_URI_PREFIX.length) : value; + return trimmed.length === 0; +} + +export function convertToJobContainer( + context: TemplateContext, + container: TemplateToken, + isServiceContainer = false +): Container | undefined { + if (container.isExpression) { + return; + } + + // Shorthand form + if (isString(container)) { + const image = container.assertString("container item"); + if (!image || image.value.length === 0) { + if (isServiceContainer) { + context.error(container, "Container image cannot be empty"); + } + return; + } + + if (isEmptyImage(image.value)) { + context.error(container, "Container image cannot be empty"); + return; + } + + return {image}; + } + + // Mapping form + const mapping = container.assertMapping("container item"); + if (!mapping) { + return; + } + let image: StringToken | undefined; let env: MappingToken | undefined; let ports: SequenceToken | undefined; let volumes: SequenceToken | undefined; let options: StringToken | undefined; + let credentials: Credential | undefined; + let hasExpressionKey = false; + let hasExpression = false; - // Skip validation for expressions for now to match - // behavior of the other parsers - for (const [, token] of TemplateToken.traverse(container)) { - if (token.isExpression) { + for (const item of mapping) { + if (item.key.isExpression) { + hasExpressionKey = true; + continue; + } + + const key = item.key.assertString("container item key"); + + switch (key.value) { + case "image": + if (item.value.isExpression) { + hasExpression = true; + break; + } + image = item.value.assertString("container image"); + break; + case "credentials": + if (!item.value.isExpression) { + credentials = convertCredentials(context, item.value); + } + break; + case "env": + if (!item.value.isExpression) { + env = item.value.assertMapping("container env"); + } + break; + case "ports": + if (!item.value.isExpression) { + ports = item.value.assertSequence("container ports"); + } + break; + case "volumes": + if (!item.value.isExpression) { + volumes = item.value.assertSequence("container volumes"); + } + break; + case "options": + if (!item.value.isExpression) { + options = item.value.assertString("container options"); + } + break; + default: + context.error(key, `Unexpected container item key: ${key.value}`); + } + } + + // Validate image + if (image) { + if (isEmptyImage(image.value)) { + context.error(image, "Container image cannot be empty"); return; } + return {image, credentials, env, ports, volumes, options}; } - if (isString(container)) { - // Workflow uses shorthand syntax `container: image-name` - image = container.assertString("container item"); - return {image: image}; - } - - const mapping = container.assertMapping("container item"); - if (mapping) - for (const item of mapping) { - const key = item.key.assertString("container item key"); - const value = item.value; - - switch (key.value) { - case "image": - image = value.assertString("container image"); - break; - case "credentials": - convertToJobCredentials(context, value); - break; - case "env": - env = value.assertMapping("container env"); - for (const envItem of env) { - envItem.key.assertString("container env value"); - } - break; - case "ports": - ports = value.assertSequence("container ports"); - for (const port of ports) { - port.assertString("container port"); - } - break; - case "volumes": - volumes = value.assertSequence("container volumes"); - for (const volume of volumes) { - volume.assertString("container volume"); - } - break; - case "options": - options = value.assertString("container options"); - break; - default: - context.error(key, `Unexpected container item key: ${key.value}`); - } - } - - if (!image) { + // No image key — skip error if expression keys could provide one + if (!hasExpressionKey && !hasExpression) { context.error(container, "Container image cannot be empty"); - } else { - return {image, env, ports, volumes, options}; } } export function convertToJobServices(context: TemplateContext, services: TemplateToken): Container[] | undefined { - const serviceList: Container[] = []; + if (services.isExpression) { + return; + } + const serviceList: Container[] = []; const mapping = services.assertMapping("services"); + for (const service of mapping) { + if (service.key.isExpression) { + continue; + } + service.key.assertString("service key"); - const container = convertToJobContainer(context, service.value); + const container = convertToJobContainer(context, service.value, true); if (container) { serviceList.push(container); } } + return serviceList; } -function convertToJobCredentials(context: TemplateContext, value: TemplateToken): Credential | undefined { +function convertCredentials(context: TemplateContext, value: TemplateToken): Credential | undefined { const mapping = value.assertMapping("credentials"); + if (!mapping) { + return; + } let username: StringToken | undefined; let password: StringToken | undefined; for (const item of mapping) { + if (item.key.isExpression) { + continue; + } + const key = item.key.assertString("credentials item"); - const value = item.value; + if (item.value.isExpression) { + continue; + } switch (key.value) { case "username": - username = value.assertString("credentials username"); + username = item.value.assertString("credentials username"); break; case "password": - password = value.assertString("credentials password"); + password = item.value.assertString("credentials password"); break; default: context.error(key, `credentials key ${key.value}`); diff --git a/workflow-parser/src/model/converter/job.ts b/workflow-parser/src/model/converter/job.ts index 514de15..dda4a63 100644 --- a/workflow-parser/src/model/converter/job.ts +++ b/workflow-parser/src/model/converter/job.ts @@ -50,7 +50,7 @@ export function convertJob(context: TemplateContext, jobKey: StringToken, token: break; case "container": - convertToJobContainer(context, item.value); + handleTemplateTokenErrors(item.value, context, undefined, () => convertToJobContainer(context, item.value)); container = item.value; break; @@ -103,7 +103,7 @@ export function convertJob(context: TemplateContext, jobKey: StringToken, token: break; case "services": - convertToJobServices(context, item.value); + handleTemplateTokenErrors(item.value, context, undefined, () => convertToJobServices(context, item.value)); services = item.value; break; diff --git a/workflow-parser/src/workflow-v1.0.json b/workflow-parser/src/workflow-v1.0.json index 07a6d6d..6d21be4 100644 --- a/workflow-parser/src/workflow-v1.0.json +++ b/workflow-parser/src/workflow-v1.0.json @@ -2345,7 +2345,7 @@ "mapping": { "properties": { "image": { - "type": "non-empty-string", + "type": "string", "description": "Use `jobs..container.image` to define the Docker image to use as the container to run the action. The value can be the Docker Hub image or a registry name." }, "options": { @@ -2390,7 +2390,7 @@ "matrix" ], "one-of": [ - "non-empty-string", + "string", "container-mapping" ] }, diff --git a/workflow-parser/testdata/skipped-tests.txt b/workflow-parser/testdata/skipped-tests.txt index 9f0ac87..44f4fae 100644 --- a/workflow-parser/testdata/skipped-tests.txt +++ b/workflow-parser/testdata/skipped-tests.txt @@ -91,3 +91,4 @@ yaml-schema-sequence.yml yaml-schema-str-flow-styles.yml yaml-schema-string.yml yaml-schema-timestamp.yml +job-container-invalid.yml