From 4b7efe88ef4a47acb02ca8f88bfcca4fe4bf211a Mon Sep 17 00:00:00 2001
From: Nikola Jokic <nikola-jokic@github.com>
Date: Fri, 3 Jun 2022 14:10:15 +0200
Subject: [PATCH 01/10] refactored tests and added docker build test, repaired
 state.network

---
 .../docker/src/dockerCommands/container.ts    | 41 ++++++++--
 .../docker/src/hooks/run-container-step.ts    | 39 +++++-----
 packages/docker/src/index.ts                  |  2 +
 packages/docker/src/utils.ts                  |  7 ++
 packages/docker/tests/cleanup-job-test.ts     | 44 +++++------
 packages/docker/tests/container-build-test.ts | 33 +++++++++
 packages/docker/tests/e2e-test.ts             | 61 ++++++++-------
 packages/docker/tests/prepare-job-test.ts     | 16 ++--
 packages/docker/tests/test-setup.ts           | 74 +++++++++++++------
 9 files changed, 204 insertions(+), 113 deletions(-)
 create mode 100644 packages/docker/tests/container-build-test.ts

diff --git a/packages/docker/src/dockerCommands/container.ts b/packages/docker/src/dockerCommands/container.ts
index 9d46166..0a4904e 100644
--- a/packages/docker/src/dockerCommands/container.ts
+++ b/packages/docker/src/dockerCommands/container.ts
@@ -7,7 +7,6 @@ import {
   ServiceContainerInfo,
   StepContainerInfo
 } from 'hooklib/lib'
-import path from 'path'
 import { env } from 'process'
 import { v4 as uuidv4 } from 'uuid'
 import { runDockerCommand, RunDockerCommandOptions } from '../utils'
@@ -146,17 +145,43 @@ export async function containerBuild(
   args: RunContainerStepArgs,
   tag: string
 ): Promise<void> {
-  const context = path.dirname(`${env.GITHUB_WORKSPACE}/${args.dockerfile}`)
+  if (!args.dockerfile) {
+    throw new Error('Container build expets args.dockerfile to be set')
+  }
+
   const dockerArgs: string[] = ['build']
   dockerArgs.push('-t', tag)
-  dockerArgs.push('-f', `${env.GITHUB_WORKSPACE}/${args.dockerfile}`)
-  dockerArgs.push(context)
+  dockerArgs.push('-f', args.dockerfile)
+  dockerArgs.push(getBuildContext(args.dockerfile))
   // TODO: figure out build working directory
   await runDockerCommand(dockerArgs, {
-    workingDir: args['buildWorkingDirectory']
+    workingDir: getWorkingDir(args.dockerfile)
   })
 }
 
+function getBuildContext(dockerfilePath: string): string {
+  const pathSplit = dockerfilePath.split('/')
+  pathSplit.splice(pathSplit.length - 1)
+  return pathSplit.join('/')
+}
+
+function getWorkingDir(dockerfilePath: string): string {
+  const workspace = env.GITHUB_WORKSPACE as string
+  let workingDir = workspace
+  if (!dockerfilePath?.includes(workspace)) {
+    // This is container action
+    const pathSplit = dockerfilePath.split('/')
+    const actionIndex = pathSplit?.findIndex(d => d === '_actions')
+    if (actionIndex) {
+      const actionSubdirectoryDepth = 3 // handle + repo + [branch | tag]
+      pathSplit.splice(actionIndex + actionSubdirectoryDepth + 1)
+      workingDir = pathSplit.join('/')
+    }
+  }
+
+  return workingDir
+}
+
 export async function containerLogs(id: string): Promise<void> {
   const dockerArgs: string[] = ['logs']
   dockerArgs.push('--details')
@@ -330,7 +355,7 @@ export async function containerExecStep(
 export async function containerRun(
   args: RunContainerStepArgs,
   name: string,
-  network: string
+  network?: string
 ): Promise<void> {
   if (!args.image) {
     throw new Error('expected image to be set')
@@ -340,7 +365,9 @@ export async function containerRun(
   dockerArgs.push('--name', name)
   dockerArgs.push(`--workdir=${args.workingDirectory}`)
   dockerArgs.push(`--label=${getRunnerLabel()}`)
-  dockerArgs.push(`--network=${network}`)
+  if (network) {
+    dockerArgs.push(`--network=${network}`)
+  }
 
   if (args.createOptions) {
     dockerArgs.push(...args.createOptions.split(' '))
diff --git a/packages/docker/src/hooks/run-container-step.ts b/packages/docker/src/hooks/run-container-step.ts
index 7823288..02555dc 100644
--- a/packages/docker/src/hooks/run-container-step.ts
+++ b/packages/docker/src/hooks/run-container-step.ts
@@ -1,13 +1,12 @@
+import { RunContainerStepArgs } from 'hooklib/lib'
+import { v4 as uuidv4 } from 'uuid'
 import {
   containerBuild,
-  registryLogin,
-  registryLogout,
   containerPull,
-  containerRun
+  containerRun,
+  registryLogin,
+  registryLogout
 } from '../dockerCommands'
-import { v4 as uuidv4 } from 'uuid'
-import * as core from '@actions/core'
-import { RunContainerStepArgs } from 'hooklib/lib'
 import { getRunnerLabel } from '../dockerCommands/constants'
 
 export async function runContainerStep(
@@ -15,23 +14,23 @@ export async function runContainerStep(
   state
 ): Promise<void> {
   const tag = generateBuildTag() // for docker build
-  if (!args.image) {
-    core.error('expected an image')
-  } else {
-    if (args.dockerfile) {
-      await containerBuild(args, tag)
-      args.image = tag
-    } else {
-      const configLocation = await registryLogin(args)
-      try {
-        await containerPull(args.image, configLocation)
-      } finally {
-        await registryLogout(configLocation)
-      }
+  if (args.image) {
+    const configLocation = await registryLogin(args)
+    try {
+      await containerPull(args.image, configLocation)
+    } finally {
+      await registryLogout(configLocation)
     }
+  } else if (args.dockerfile) {
+    await containerBuild(args, tag)
+    args.image = tag
+  } else {
+    throw new Error(
+      'run container step should have image or dockerfile fields specified'
+    )
   }
   // container will get pruned at the end of the job based on the label, no need to cleanup here
-  await containerRun(args, tag.split(':')[1], state.network)
+  await containerRun(args, tag.split(':')[1], state?.network)
 }
 
 function generateBuildTag(): string {
diff --git a/packages/docker/src/index.ts b/packages/docker/src/index.ts
index c620392..4cb91b7 100644
--- a/packages/docker/src/index.ts
+++ b/packages/docker/src/index.ts
@@ -13,6 +13,7 @@ import {
   runContainerStep,
   runScriptStep
 } from './hooks'
+import { checkEnvironment } from './utils'
 
 async function run(): Promise<void> {
   const input = await getInputFromStdin()
@@ -23,6 +24,7 @@ async function run(): Promise<void> {
   const state = input['state']
 
   try {
+    checkEnvironment()
     switch (command) {
       case Command.PrepareJob:
         await prepareJob(args as PrepareJobArgs, responseFile)
diff --git a/packages/docker/src/utils.ts b/packages/docker/src/utils.ts
index b624403..69580fe 100644
--- a/packages/docker/src/utils.ts
+++ b/packages/docker/src/utils.ts
@@ -2,6 +2,7 @@
 /* eslint-disable @typescript-eslint/no-require-imports */
 /* eslint-disable import/no-commonjs */
 import * as core from '@actions/core'
+import { env } from 'process'
 // Import this way otherwise typescript has errors
 const exec = require('@actions/exec')
 
@@ -42,6 +43,12 @@ export function sanitize(val: string): string {
   return newNameBuilder.join('')
 }
 
+export function checkEnvironment(): void {
+  if (!env.GITHUB_WORKSPACE) {
+    throw new Error('GITHUB_WORKSPACE is not set')
+  }
+}
+
 // isAlpha accepts single character and checks if
 // that character is [a-zA-Z]
 function isAlpha(val: string): boolean {
diff --git a/packages/docker/tests/cleanup-job-test.ts b/packages/docker/tests/cleanup-job-test.ts
index efee5b0..d0946be 100644
--- a/packages/docker/tests/cleanup-job-test.ts
+++ b/packages/docker/tests/cleanup-job-test.ts
@@ -1,47 +1,41 @@
-import { prepareJob, cleanupJob } from '../src/hooks'
-import { v4 as uuidv4 } from 'uuid'
 import * as fs from 'fs'
 import * as path from 'path'
+import { v4 as uuidv4 } from 'uuid'
+import { cleanupJob, prepareJob } from '../src/hooks'
 import TestSetup from './test-setup'
 
 const prepareJobInputPath = path.resolve(
   `${__dirname}/../../../examples/prepare-job.json`
 )
 
-const tmpOutputDir = `${__dirname}/${uuidv4()}`
-
 let prepareJobOutputPath: string
-let prepareJobData: any
+let prepareJobDefinition: any
 
 let testSetup: TestSetup
 
 jest.useRealTimers()
 
 describe('cleanup job', () => {
-  beforeAll(() => {
-    fs.mkdirSync(tmpOutputDir, { recursive: true })
-  })
-
-  afterAll(() => {
-    fs.rmSync(tmpOutputDir, { recursive: true })
-  })
-
   beforeEach(async () => {
-    const prepareJobRawData = fs.readFileSync(prepareJobInputPath, 'utf8')
-    prepareJobData = JSON.parse(prepareJobRawData.toString())
-
-    prepareJobOutputPath = `${tmpOutputDir}/prepare-job-output-${uuidv4()}.json`
-    fs.writeFileSync(prepareJobOutputPath, '')
-
     testSetup = new TestSetup()
     testSetup.initialize()
 
-    prepareJobData.args.container.userMountVolumes = testSetup.userMountVolumes
-    prepareJobData.args.container.systemMountVolumes =
-      testSetup.systemMountVolumes
-    prepareJobData.args.container.workingDirectory = testSetup.workingDirectory
+    const prepareJobRawData = fs.readFileSync(prepareJobInputPath, 'utf8')
+    prepareJobDefinition = JSON.parse(prepareJobRawData.toString())
 
-    await prepareJob(prepareJobData.args, prepareJobOutputPath)
+    prepareJobOutputPath = `${
+      testSetup.testDir
+    }/prepare-job-output-${uuidv4()}.json`
+    fs.writeFileSync(prepareJobOutputPath, '')
+
+    prepareJobDefinition.args.container.userMountVolumes =
+      testSetup.userMountVolumes
+    prepareJobDefinition.args.container.systemMountVolumes =
+      testSetup.systemMountVolumes
+    prepareJobDefinition.args.container.workingDirectory =
+      testSetup.containerWorkingDirectory
+
+    await prepareJob(prepareJobDefinition.args, prepareJobOutputPath)
   })
 
   afterEach(() => {
@@ -56,7 +50,7 @@ describe('cleanup job', () => {
     )
     const parsedPrepareJobOutput = JSON.parse(prepareJobOutputContent)
     await expect(
-      cleanupJob(prepareJobData.args, parsedPrepareJobOutput.state, null)
+      cleanupJob(prepareJobDefinition.args, parsedPrepareJobOutput.state, null)
     ).resolves.not.toThrow()
   })
 })
diff --git a/packages/docker/tests/container-build-test.ts b/packages/docker/tests/container-build-test.ts
new file mode 100644
index 0000000..8b0150c
--- /dev/null
+++ b/packages/docker/tests/container-build-test.ts
@@ -0,0 +1,33 @@
+import * as fs from 'fs'
+import { containerBuild } from '../src/dockerCommands'
+import TestSetup from './test-setup'
+
+let testSetup
+let runContainerStepDefinition
+const runContainerStepInputPath = `${__dirname}/../../../examples/run-container-step.json`
+
+describe('container build', () => {
+  beforeEach(() => {
+    testSetup = new TestSetup()
+    testSetup.initialize()
+
+    let runContainerStepJson = fs.readFileSync(
+      runContainerStepInputPath,
+      'utf8'
+    )
+    runContainerStepDefinition = JSON.parse(runContainerStepJson.toString())
+    runContainerStepDefinition.image = ''
+    const actionPath = testSetup.initializeDockerAction()
+    runContainerStepDefinition.dockerfile = `${actionPath}/Dockerfile`
+  })
+
+  afterEach(() => {
+    testSetup.teardown()
+  })
+
+  it('should build container', async () => {
+    await expect(
+      containerBuild(runContainerStepDefinition, 'example-test-tag')
+    ).resolves.not.toThrow()
+  })
+})
diff --git a/packages/docker/tests/e2e-test.ts b/packages/docker/tests/e2e-test.ts
index cdc889b..4c5e1d7 100644
--- a/packages/docker/tests/e2e-test.ts
+++ b/packages/docker/tests/e2e-test.ts
@@ -1,12 +1,12 @@
-import {
-  prepareJob,
-  cleanupJob,
-  runScriptStep,
-  runContainerStep
-} from '../src/hooks'
 import * as fs from 'fs'
 import * as path from 'path'
 import { v4 as uuidv4 } from 'uuid'
+import {
+  cleanupJob,
+  prepareJob,
+  runContainerStep,
+  runScriptStep
+} from '../src/hooks'
 import TestSetup from './test-setup'
 
 const prepareJobJson = fs.readFileSync(
@@ -21,10 +21,9 @@ const containerStepJson = fs.readFileSync(
 
 const tmpOutputDir = `${__dirname}/_temp/${uuidv4()}`
 
-let prepareJobData: any
-let scriptStepJson: any
-let scriptStepData: any
-let containerStepData: any
+let prepareJobDefinition: any
+let scriptStepDefinition: any
+let runContainerStepDefinition: any
 
 let prepareJobOutputFilePath: string
 
@@ -44,23 +43,29 @@ describe('e2e', () => {
     testSetup = new TestSetup()
     testSetup.initialize()
 
-    prepareJobData = JSON.parse(prepareJobJson)
-    prepareJobData.args.container.userMountVolumes = testSetup.userMountVolumes
-    prepareJobData.args.container.systemMountVolumes =
+    prepareJobDefinition = JSON.parse(prepareJobJson)
+    prepareJobDefinition.args.container.userMountVolumes =
+      testSetup.userMountVolumes
+    prepareJobDefinition.args.container.systemMountVolumes =
       testSetup.systemMountVolumes
-    prepareJobData.args.container.workingDirectory = testSetup.workingDirectory
+    prepareJobDefinition.args.container.workingDirectory =
+      testSetup.containerWorkingDirectory
 
-    scriptStepJson = fs.readFileSync(
+    const scriptStepJson = fs.readFileSync(
       path.resolve(__dirname + '/../../../examples/run-script-step.json'),
       'utf8'
     )
-    scriptStepData = JSON.parse(scriptStepJson)
-    scriptStepData.args.workingDirectory = testSetup.workingDirectory
+    scriptStepDefinition = JSON.parse(scriptStepJson)
+    scriptStepDefinition.args.workingDirectory =
+      testSetup.containerWorkingDirectory
 
-    containerStepData = JSON.parse(containerStepJson)
-    containerStepData.args.workingDirectory = testSetup.workingDirectory
-    containerStepData.args.userMountVolumes = testSetup.userMountVolumes
-    containerStepData.args.systemMountVolumes = testSetup.systemMountVolumes
+    runContainerStepDefinition = JSON.parse(containerStepJson)
+    runContainerStepDefinition.args.workingDirectory =
+      testSetup.containerWorkingDirectory
+    runContainerStepDefinition.args.userMountVolumes =
+      testSetup.userMountVolumes
+    runContainerStepDefinition.args.systemMountVolumes =
+      testSetup.systemMountVolumes
 
     prepareJobOutputFilePath = `${tmpOutputDir}/prepare-job-output-${uuidv4()}.json`
     fs.writeFileSync(prepareJobOutputFilePath, '')
@@ -73,27 +78,27 @@ describe('e2e', () => {
 
   it('should prepare job, then run script step, then run container step then cleanup', async () => {
     await expect(
-      prepareJob(prepareJobData.args, prepareJobOutputFilePath)
+      prepareJob(prepareJobDefinition.args, prepareJobOutputFilePath)
     ).resolves.not.toThrow()
     let rawState = fs.readFileSync(prepareJobOutputFilePath, 'utf-8')
     let resp = JSON.parse(rawState)
     await expect(
-      runScriptStep(scriptStepData.args, resp.state)
+      runScriptStep(scriptStepDefinition.args, resp.state)
     ).resolves.not.toThrow()
     await expect(
-      runContainerStep(containerStepData.args, resp.state)
+      runContainerStep(runContainerStepDefinition.args, resp.state)
     ).resolves.not.toThrow()
     await expect(cleanupJob(resp, resp.state, null)).resolves.not.toThrow()
   })
 
   it('should prepare job, then run script step, then run container step with Dockerfile then cleanup', async () => {
     await expect(
-      prepareJob(prepareJobData.args, prepareJobOutputFilePath)
+      prepareJob(prepareJobDefinition.args, prepareJobOutputFilePath)
     ).resolves.not.toThrow()
     let rawState = fs.readFileSync(prepareJobOutputFilePath, 'utf-8')
     let resp = JSON.parse(rawState)
     await expect(
-      runScriptStep(scriptStepData.args, resp.state)
+      runScriptStep(scriptStepDefinition.args, resp.state)
     ).resolves.not.toThrow()
 
     const dockerfilePath = `${tmpOutputDir}/Dockerfile`
@@ -104,7 +109,9 @@ ENV TEST=test
 ENTRYPOINT [ "tail", "-f", "/dev/null" ]
     `
     )
-    const containerStepDataCopy = JSON.parse(JSON.stringify(containerStepData))
+    const containerStepDataCopy = JSON.parse(
+      JSON.stringify(runContainerStepDefinition)
+    )
     process.env.GITHUB_WORKSPACE = tmpOutputDir
     containerStepDataCopy.args.dockerfile = 'Dockerfile'
     containerStepDataCopy.args.context = '.'
diff --git a/packages/docker/tests/prepare-job-test.ts b/packages/docker/tests/prepare-job-test.ts
index e94aa08..56f8241 100644
--- a/packages/docker/tests/prepare-job-test.ts
+++ b/packages/docker/tests/prepare-job-test.ts
@@ -7,20 +7,11 @@ jest.useRealTimers()
 
 let prepareJobOutputPath: string
 let prepareJobData: any
-const tmpOutputDir = `${__dirname}/_temp/${uuidv4()}`
 const prepareJobInputPath = `${__dirname}/../../../examples/prepare-job.json`
 
 let testSetup: TestSetup
 
 describe('prepare job', () => {
-  beforeAll(() => {
-    fs.mkdirSync(tmpOutputDir, { recursive: true })
-  })
-
-  afterAll(() => {
-    fs.rmSync(tmpOutputDir, { recursive: true })
-  })
-
   beforeEach(async () => {
     testSetup = new TestSetup()
     testSetup.initialize()
@@ -31,9 +22,12 @@ describe('prepare job', () => {
     prepareJobData.args.container.userMountVolumes = testSetup.userMountVolumes
     prepareJobData.args.container.systemMountVolumes =
       testSetup.systemMountVolumes
-    prepareJobData.args.container.workingDirectory = testSetup.workingDirectory
+    prepareJobData.args.container.workingDirectory =
+      testSetup.containerWorkingDirectory
 
-    prepareJobOutputPath = `${tmpOutputDir}/prepare-job-output-${uuidv4()}.json`
+    prepareJobOutputPath = `${
+      testSetup.testDir
+    }/prepare-job-output-${uuidv4()}.json`
     fs.writeFileSync(prepareJobOutputPath, '')
   })
 
diff --git a/packages/docker/tests/test-setup.ts b/packages/docker/tests/test-setup.ts
index 58434b9..efb9c9d 100644
--- a/packages/docker/tests/test-setup.ts
+++ b/packages/docker/tests/test-setup.ts
@@ -1,11 +1,12 @@
 import * as fs from 'fs'
-import { v4 as uuidv4 } from 'uuid'
-import { env } from 'process'
 import { Mount } from 'hooklib'
+import { env } from 'process'
+import { v4 as uuidv4 } from 'uuid'
 
 export default class TestSetup {
   private testdir: string
   private runnerMockDir: string
+
   private runnerMockSubdirs = {
     work: '_work',
     externals: 'externals',
@@ -15,36 +16,20 @@ export default class TestSetup {
     githubHome: '_work/_temp/_github_home',
     githubWorkflow: '_work/_temp/_github_workflow'
   }
-
-  private readonly projectName = 'example'
+  private readonly projectName = 'test'
 
   constructor() {
     this.testdir = `${__dirname}/_temp/${uuidv4()}`
     this.runnerMockDir = `${this.testdir}/runner/_layout`
   }
 
-  private get allTestDirectories() {
-    const resp = [this.testdir, this.runnerMockDir]
-
-    for (const [key, value] of Object.entries(this.runnerMockSubdirs)) {
-      resp.push(`${this.runnerMockDir}/${value}`)
-    }
-
-    resp.push(
-      `${this.runnerMockDir}/_work/${this.projectName}/${this.projectName}`
-    )
-
-    return resp
-  }
-
   public initialize(): void {
     for (const dir of this.allTestDirectories) {
       fs.mkdirSync(dir, { recursive: true })
     }
-    env['RUNNER_NAME'] = 'test'
-    env[
-      'RUNNER_TEMP'
-    ] = `${this.runnerMockDir}/${this.runnerMockSubdirs.workTemp}`
+    env.RUNNER_NAME = 'test'
+    env.RUNNER_TEMP = `${this.runnerMockDir}/${this.runnerMockSubdirs.workTemp}`
+    env.GITHUB_WORKSPACE = this.runnerProjectWorkDir
   }
 
   public teardown(): void {
@@ -61,6 +46,24 @@ export default class TestSetup {
     ]
   }
 
+  public get runnerProjectWorkDir() {
+    return `${this.runnerMockDir}/_work/${this.projectName}/${this.projectName}`
+  }
+
+  public get testDir() {
+    return this.testdir
+  }
+
+  private get allTestDirectories() {
+    const resp = [this.testdir, this.runnerMockDir, this.runnerProjectWorkDir]
+
+    for (const [key, value] of Object.entries(this.runnerMockSubdirs)) {
+      resp.push(`${this.runnerMockDir}/${value}`)
+    }
+
+    return resp
+  }
+
   public get systemMountVolumes(): Mount[] {
     return [
       {
@@ -106,7 +109,32 @@ export default class TestSetup {
     ]
   }
 
-  public get workingDirectory(): string {
+  public get containerWorkingDirectory(): string {
     return `/__w/${this.projectName}/${this.projectName}`
   }
+
+  public initializeDockerAction(): string {
+    const actionPath = `${this.testdir}/_actions/example-handle/example-repo/example-branch/mock-directory`
+    fs.mkdirSync(actionPath, { recursive: true })
+    this.writeDockerfile(actionPath)
+    this.writeEntrypoint(actionPath)
+    return actionPath
+  }
+
+  private writeDockerfile(actionPath: string) {
+    const content = `FROM alpine:3.10
+COPY entrypoint.sh /entrypoint.sh
+ENTRYPOINT ["/entrypoint.sh"]`
+    fs.writeFileSync(`${actionPath}/Dockerfile`, content)
+  }
+
+  private writeEntrypoint(actionPath) {
+    const content = `#!/bin/sh -l
+echo "Hello $1"
+time=$(date)
+echo "::set-output name=time::$time"`
+    const entryPointPath = `${actionPath}/entrypoint.sh`
+    fs.writeFileSync(entryPointPath, content)
+    fs.chmodSync(entryPointPath, 0o755)
+  }
 }

From 5ec2edbe11af3517cd144d097a22ba0a05522f8d Mon Sep 17 00:00:00 2001
From: Nikola Jokic <97525037+nikola-jokic@users.noreply.github.com>
Date: Fri, 3 Jun 2022 15:02:10 +0200
Subject: [PATCH 02/10] err message suggestion

Co-authored-by: Thomas Boop <52323235+thboop@users.noreply.github.com>
---
 packages/docker/src/dockerCommands/container.ts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/packages/docker/src/dockerCommands/container.ts b/packages/docker/src/dockerCommands/container.ts
index 0a4904e..b9ae66e 100644
--- a/packages/docker/src/dockerCommands/container.ts
+++ b/packages/docker/src/dockerCommands/container.ts
@@ -146,7 +146,7 @@ export async function containerBuild(
   tag: string
 ): Promise<void> {
   if (!args.dockerfile) {
-    throw new Error('Container build expets args.dockerfile to be set')
+    throw new Error('Container build expects 'args.dockerfile' to be set')
   }
 
   const dockerArgs: string[] = ['build']

From b0cf60b678c1730545a60e45c0bbaf16954bf598 Mon Sep 17 00:00:00 2001
From: Nikola Jokic <nikola-jokic@github.com>
Date: Fri, 3 Jun 2022 15:10:14 +0200
Subject: [PATCH 03/10] changed split to dirname and fixed syntax error in err
 message

---
 packages/docker/src/dockerCommands/container.ts | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/packages/docker/src/dockerCommands/container.ts b/packages/docker/src/dockerCommands/container.ts
index b9ae66e..043d40b 100644
--- a/packages/docker/src/dockerCommands/container.ts
+++ b/packages/docker/src/dockerCommands/container.ts
@@ -7,6 +7,7 @@ import {
   ServiceContainerInfo,
   StepContainerInfo
 } from 'hooklib/lib'
+import * as path from 'path'
 import { env } from 'process'
 import { v4 as uuidv4 } from 'uuid'
 import { runDockerCommand, RunDockerCommandOptions } from '../utils'
@@ -146,7 +147,7 @@ export async function containerBuild(
   tag: string
 ): Promise<void> {
   if (!args.dockerfile) {
-    throw new Error('Container build expects 'args.dockerfile' to be set')
+    throw new Error("Container build expects 'args.dockerfile' to be set")
   }
 
   const dockerArgs: string[] = ['build']
@@ -160,9 +161,7 @@ export async function containerBuild(
 }
 
 function getBuildContext(dockerfilePath: string): string {
-  const pathSplit = dockerfilePath.split('/')
-  pathSplit.splice(pathSplit.length - 1)
-  return pathSplit.join('/')
+  return path.dirname(dockerfilePath)
 }
 
 function getWorkingDir(dockerfilePath: string): string {

From 171956673cac4440420984d9e298f72a91168579 Mon Sep 17 00:00:00 2001
From: Ferenc Hammerl <fhammerl@github.com>
Date: Fri, 3 Jun 2022 06:52:06 -0700
Subject: [PATCH 04/10] Handle empty registry property in input

---
 packages/docker/src/dockerCommands/container.ts | 13 +++++++------
 packages/docker/src/hooks/run-container-step.ts |  2 +-
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/packages/docker/src/dockerCommands/container.ts b/packages/docker/src/dockerCommands/container.ts
index 0a4904e..f35ddf8 100644
--- a/packages/docker/src/dockerCommands/container.ts
+++ b/packages/docker/src/dockerCommands/container.ts
@@ -3,6 +3,7 @@ import * as fs from 'fs'
 import {
   ContainerInfo,
   JobContainerInfo,
+  Registry,
   RunContainerStepArgs,
   ServiceContainerInfo,
   StepContainerInfo
@@ -266,19 +267,19 @@ export async function containerPorts(id: string): Promise<string[]> {
   return portMappings.split('\n')
 }
 
-export async function registryLogin(args): Promise<string> {
-  if (!args.registry) {
+export async function registryLogin(registry?: Registry): Promise<string> {
+  if (!registry) {
     return ''
   }
   const credentials = {
-    username: args.registry.username,
-    password: args.registry.password
+    username: registry.username,
+    password: registry.password
   }
 
   const configLocation = `${env.RUNNER_TEMP}/.docker_${uuidv4()}`
   fs.mkdirSync(configLocation)
   try {
-    await dockerLogin(configLocation, args.registry.serverUrl, credentials)
+    await dockerLogin(configLocation, registry.serverUrl, credentials)
   } catch (error) {
     fs.rmdirSync(configLocation, { recursive: true })
     throw error
@@ -296,7 +297,7 @@ export async function registryLogout(configLocation: string): Promise<void> {
 async function dockerLogin(
   configLocation: string,
   registry: string,
-  credentials: { username: string; password: string }
+  credentials: { username?: string; password?: string }
 ): Promise<void> {
   const credentialsArgs =
     credentials.username && credentials.password
diff --git a/packages/docker/src/hooks/run-container-step.ts b/packages/docker/src/hooks/run-container-step.ts
index 02555dc..76cdec9 100644
--- a/packages/docker/src/hooks/run-container-step.ts
+++ b/packages/docker/src/hooks/run-container-step.ts
@@ -15,7 +15,7 @@ export async function runContainerStep(
 ): Promise<void> {
   const tag = generateBuildTag() // for docker build
   if (args.image) {
-    const configLocation = await registryLogin(args)
+    const configLocation = await registryLogin(args.registry)
     try {
       await containerPull(args.image, configLocation)
     } finally {

From c2f9b10f4df0959df448ec6d0eb9e382b3c43a72 Mon Sep 17 00:00:00 2001
From: Ferenc Hammerl <fhammerl@github.com>
Date: Fri, 3 Jun 2022 06:56:26 -0700
Subject: [PATCH 05/10] Fix case sensitive image in test

---
 packages/docker/tests/container-pull-test.ts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/packages/docker/tests/container-pull-test.ts b/packages/docker/tests/container-pull-test.ts
index 77bb5cb..bab1cae 100644
--- a/packages/docker/tests/container-pull-test.ts
+++ b/packages/docker/tests/container-pull-test.ts
@@ -4,7 +4,7 @@ jest.useRealTimers()
 
 describe('container pull', () => {
   it('should fail', async () => {
-    const arg = { image: 'doesNotExist' }
+    const arg = { image: 'does-not-exist' }
     await expect(containerPull(arg.image, '')).rejects.toThrow()
   })
   it('should succeed', async () => {

From 7010d21bff8e0b25bb5bd950bade53336d2fd086 Mon Sep 17 00:00:00 2001
From: Nikola Jokic <nikola-jokic@github.com>
Date: Fri, 3 Jun 2022 16:24:20 +0200
Subject: [PATCH 06/10] repaired tests

---
 packages/docker/tests/cleanup-job-test.ts |  2 ++
 packages/docker/tests/e2e-test.ts         | 24 ++++++++---------------
 packages/docker/tests/prepare-job-test.ts |  2 ++
 3 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/packages/docker/tests/cleanup-job-test.ts b/packages/docker/tests/cleanup-job-test.ts
index d0946be..16c37e7 100644
--- a/packages/docker/tests/cleanup-job-test.ts
+++ b/packages/docker/tests/cleanup-job-test.ts
@@ -34,6 +34,8 @@ describe('cleanup job', () => {
       testSetup.systemMountVolumes
     prepareJobDefinition.args.container.workingDirectory =
       testSetup.containerWorkingDirectory
+    prepareJobDefinition.args.container.registry = null
+    prepareJobDefinition.args.services.forEach(s => (s.registry = null))
 
     await prepareJob(prepareJobDefinition.args, prepareJobOutputPath)
   })
diff --git a/packages/docker/tests/e2e-test.ts b/packages/docker/tests/e2e-test.ts
index 4c5e1d7..00c26a2 100644
--- a/packages/docker/tests/e2e-test.ts
+++ b/packages/docker/tests/e2e-test.ts
@@ -19,8 +19,6 @@ const containerStepJson = fs.readFileSync(
   'utf8'
 )
 
-const tmpOutputDir = `${__dirname}/_temp/${uuidv4()}`
-
 let prepareJobDefinition: any
 let scriptStepDefinition: any
 let runContainerStepDefinition: any
@@ -30,14 +28,6 @@ let prepareJobOutputFilePath: string
 let testSetup: TestSetup
 
 describe('e2e', () => {
-  beforeAll(() => {
-    fs.mkdirSync(tmpOutputDir, { recursive: true })
-  })
-
-  afterAll(() => {
-    fs.rmSync(tmpOutputDir, { recursive: true })
-  })
-
   beforeEach(() => {
     // init dirs
     testSetup = new TestSetup()
@@ -50,6 +40,8 @@ describe('e2e', () => {
       testSetup.systemMountVolumes
     prepareJobDefinition.args.container.workingDirectory =
       testSetup.containerWorkingDirectory
+    prepareJobDefinition.args.container.registry = null
+    prepareJobDefinition.args.services.forEach(s => (s.registry = null))
 
     const scriptStepJson = fs.readFileSync(
       path.resolve(__dirname + '/../../../examples/run-script-step.json'),
@@ -58,6 +50,7 @@ describe('e2e', () => {
     scriptStepDefinition = JSON.parse(scriptStepJson)
     scriptStepDefinition.args.workingDirectory =
       testSetup.containerWorkingDirectory
+    scriptStepDefinition.args.registry = null
 
     runContainerStepDefinition = JSON.parse(containerStepJson)
     runContainerStepDefinition.args.workingDirectory =
@@ -65,14 +58,15 @@ describe('e2e', () => {
     runContainerStepDefinition.args.userMountVolumes =
       testSetup.userMountVolumes
     runContainerStepDefinition.args.systemMountVolumes =
-      testSetup.systemMountVolumes
+      runContainerStepDefinition.args.registry = null
 
-    prepareJobOutputFilePath = `${tmpOutputDir}/prepare-job-output-${uuidv4()}.json`
+    prepareJobOutputFilePath = `${
+      testSetup.testDir
+    }/prepare-job-output-${uuidv4()}.json`
     fs.writeFileSync(prepareJobOutputFilePath, '')
   })
 
   afterEach(() => {
-    fs.rmSync(prepareJobOutputFilePath, { force: true })
     testSetup.teardown()
   })
 
@@ -101,7 +95,7 @@ describe('e2e', () => {
       runScriptStep(scriptStepDefinition.args, resp.state)
     ).resolves.not.toThrow()
 
-    const dockerfilePath = `${tmpOutputDir}/Dockerfile`
+    const dockerfilePath = `${testSetup.testDir}/Dockerfile`
     fs.writeFileSync(
       dockerfilePath,
       `FROM ubuntu:latest
@@ -112,10 +106,8 @@ ENTRYPOINT [ "tail", "-f", "/dev/null" ]
     const containerStepDataCopy = JSON.parse(
       JSON.stringify(runContainerStepDefinition)
     )
-    process.env.GITHUB_WORKSPACE = tmpOutputDir
     containerStepDataCopy.args.dockerfile = 'Dockerfile'
     containerStepDataCopy.args.context = '.'
-    console.log(containerStepDataCopy.args)
     await expect(
       runContainerStep(containerStepDataCopy.args, resp.state)
     ).resolves.not.toThrow()
diff --git a/packages/docker/tests/prepare-job-test.ts b/packages/docker/tests/prepare-job-test.ts
index 56f8241..12c5e6f 100644
--- a/packages/docker/tests/prepare-job-test.ts
+++ b/packages/docker/tests/prepare-job-test.ts
@@ -24,6 +24,8 @@ describe('prepare job', () => {
       testSetup.systemMountVolumes
     prepareJobData.args.container.workingDirectory =
       testSetup.containerWorkingDirectory
+    prepareJobData.args.container.registry = null
+    prepareJobData.args.services.forEach(s => (s.registry = null))
 
     prepareJobOutputPath = `${
       testSetup.testDir

From 150bc0503af0b75e883be6ea9878f7511ba98270 Mon Sep 17 00:00:00 2001
From: Nikola Jokic <nikola-jokic@github.com>
Date: Wed, 8 Jun 2022 15:02:40 +0200
Subject: [PATCH 07/10] substituited all -e key=value to -e key

---
 packages/docker/src/dockerCommands/container.ts | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/packages/docker/src/dockerCommands/container.ts b/packages/docker/src/dockerCommands/container.ts
index e47fc84..633dadb 100644
--- a/packages/docker/src/dockerCommands/container.ts
+++ b/packages/docker/src/dockerCommands/container.ts
@@ -43,13 +43,9 @@ export async function createContainer(
   }
 
   if (args.environmentVariables) {
-    for (const [key, value] of Object.entries(args.environmentVariables)) {
+    for (const [key] of Object.entries(args.environmentVariables)) {
       dockerArgs.push('-e')
-      if (!value) {
-        dockerArgs.push(`"${key}"`)
-      } else {
-        dockerArgs.push(`"${key}=${value}"`)
-      }
+      dockerArgs.push(`"${key}"`)
     }
   }
 
@@ -319,13 +315,9 @@ export async function containerExecStep(
 ): Promise<void> {
   const dockerArgs: string[] = ['exec', '-i']
   dockerArgs.push(`--workdir=${args.workingDirectory}`)
-  for (const [key, value] of Object.entries(args['environmentVariables'])) {
+  for (const [key] of Object.entries(args['environmentVariables'])) {
     dockerArgs.push('-e')
-    if (!value) {
-      dockerArgs.push(`"${key}"`)
-    } else {
-      dockerArgs.push(`"${key}=${value}"`)
-    }
+    dockerArgs.push(`"${key}"`)
   }
 
   // Todo figure out prepend path and update it here

From ee2554e2c0ee5e3ba8f7c6bc96904ba88b8f0169 Mon Sep 17 00:00:00 2001
From: Nikola Jokic <nikola-jokic@github.com>
Date: Wed, 8 Jun 2022 16:49:44 +0200
Subject: [PATCH 08/10] filter out empty ports

---
 packages/docker/src/dockerCommands/container.ts | 2 +-
 packages/docker/src/hooks/prepare-job.ts        | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/packages/docker/src/dockerCommands/container.ts b/packages/docker/src/dockerCommands/container.ts
index 20a03a5..eee67a5 100644
--- a/packages/docker/src/dockerCommands/container.ts
+++ b/packages/docker/src/dockerCommands/container.ts
@@ -271,7 +271,7 @@ export async function healthCheck({
 export async function containerPorts(id: string): Promise<string[]> {
   const dockerArgs = ['port', id]
   const portMappings = (await runDockerCommand(dockerArgs)).trim()
-  return portMappings.split('\n')
+  return portMappings.split('\n').filter(p => !!p)
 }
 
 export async function registryLogin(registry?: Registry): Promise<string> {
diff --git a/packages/docker/src/hooks/prepare-job.ts b/packages/docker/src/hooks/prepare-job.ts
index 076658b..581e6d1 100644
--- a/packages/docker/src/hooks/prepare-job.ts
+++ b/packages/docker/src/hooks/prepare-job.ts
@@ -186,15 +186,15 @@ function transformDockerPortsToContextPorts(
   meta: ContainerMetadata
 ): ContextPorts {
   // ex: '80/tcp -> 0.0.0.0:80'
-  const re = /^(\d+)\/(\w+)? -> (.*):(\d+)$/
+  const re = /^(\d+)(\/\w+)? -> (.*):(\d+)$/
   const contextPorts: ContextPorts = {}
 
-  if (meta.ports) {
+  if (meta.ports?.length) {
     for (const port of meta.ports) {
       const matches = port.match(re)
       if (!matches) {
         throw new Error(
-          'Container ports could not match the regex: "^(\\d+)\\/(\\w+)? -> (.*):(\\d+)$"'
+          'Container ports could not match the regex: "^(\\d+)(\\/\\w+)? -> (.*):(\\d+)$"'
         )
       }
       contextPorts[matches[1]] = matches[matches.length - 1]

From 3d0ca83d2d5d4e4e09018df7b4c37649802dcdec Mon Sep 17 00:00:00 2001
From: Nikola Jokic <nikola-jokic@github.com>
Date: Wed, 8 Jun 2022 17:37:43 +0200
Subject: [PATCH 09/10] removed quotes around -e env variables

---
 packages/docker/src/dockerCommands/container.ts | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/packages/docker/src/dockerCommands/container.ts b/packages/docker/src/dockerCommands/container.ts
index 9af91c8..29886b7 100644
--- a/packages/docker/src/dockerCommands/container.ts
+++ b/packages/docker/src/dockerCommands/container.ts
@@ -44,7 +44,7 @@ export async function createContainer(
   if (args.environmentVariables) {
     for (const [key] of Object.entries(args.environmentVariables)) {
       dockerArgs.push('-e')
-      dockerArgs.push(`"${key}"`)
+      dockerArgs.push(key)
     }
   }
 
@@ -340,11 +340,11 @@ export async function containerExecStep(
   dockerArgs.push(`--workdir=${args.workingDirectory}`)
   for (const [key] of Object.entries(args['environmentVariables'])) {
     dockerArgs.push('-e')
-    dockerArgs.push(`"${key}"`)
+    dockerArgs.push(key)
   }
 
   if (args.prependPath?.length) {
-    dockerArgs.push('-e', `"PATH=${args.prependPath.join(':')}:$PATH"`)
+    dockerArgs.push('-e', `PATH=${args.prependPath.join(':')}:$PATH`)
   }
 
   dockerArgs.push(containerId)

From 2aa6f9d9c82c6d09579b9ff17a796fa1ec4b967f Mon Sep 17 00:00:00 2001
From: Nikola Jokic <nikola-jokic@github.com>
Date: Wed, 8 Jun 2022 17:39:37 +0200
Subject: [PATCH 10/10] added quotes back to the path

---
 packages/docker/src/dockerCommands/container.ts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/packages/docker/src/dockerCommands/container.ts b/packages/docker/src/dockerCommands/container.ts
index 29886b7..89bf737 100644
--- a/packages/docker/src/dockerCommands/container.ts
+++ b/packages/docker/src/dockerCommands/container.ts
@@ -344,7 +344,7 @@ export async function containerExecStep(
   }
 
   if (args.prependPath?.length) {
-    dockerArgs.push('-e', `PATH=${args.prependPath.join(':')}:$PATH`)
+    dockerArgs.push('-e', `"PATH=${args.prependPath.join(':')}:$PATH"`)
   }
 
   dockerArgs.push(containerId)