Merge pull request #108 from github/brrygrdn/reintroduce-improved-err…

…or-handling

[Manual Revert] Fix the relaying of container errors to Dependabot API

.github/workflows/test.yml

@@ -21,7 +21,7 @@ jobs:
           node-version: ${{ steps.nvm.outputs.NVMRC }}
       - run: npm ci
       - run: npm run all
-      - run: git diff --quiet
+      - run: git diff --quiet dist/
   test: # make sure the action works on a clean machine without building
     runs-on: ubuntu-latest
     steps:

__fixtures__/output/happy_path/output.json

@@ -0,0 +1 @@
+{"base64_dependency_files":[{"name":"go.mod","content":"bW9kdWxlIGdpdGh1Yi5jb20vZGVwZW5kYWJvdC92Z290ZXN0CgpnbyAxLjEy\nCgpyZXF1aXJlIHJzYy5pby9xciB2MC4xLjAKCg==\n","directory":"/","type":"file","support_file":false,"content_encoding":"utf-8","deleted":false,"operation":"update"},{"name":"go.sum","content":"cnNjLmlvL3FyIHYwLjEuMCBoMTpNL3NBeHNVMko1bWxRNFc4NEJ4Z2EyRWdk\nUXFPYUFsaWlwY2pQbU1VTTVRPQpyc2MuaW8vcXIgdjAuMS4wL2dvLm1vZCBo\nMTpJRit1WmprYjlmcXllRi80dGxCb3lucW1ReFVvUGZXRUtoOTIxY29PdVhz\nPQo=\n","directory":"/","type":"file","support_file":false,"content_encoding":"utf-8","deleted":false,"operation":"update"}],"base_commit_sha":"818a3756444cb0d997a1e11820563105db1c24c4"}

__fixtures__/output/malformed/output.json

@@ -0,0 +1 @@
+C'est ne pas un json

__tests__/container-service.test.ts

@@ -7,18 +7,36 @@ describe('ContainerService', () => {
   const docker = new Docker()
   let container: any
 
-  beforeAll(async () => {
-    await ImageService.pull('alpine')
-    container = await docker.createContainer({
-      Image: 'alpine',
-      AttachStdout: true,
-      AttachStderr: true,
-      Cmd: ['/bin/sh', '-c', 'echo $VAR'],
-      Env: ['VAR=env-var']
+  describe('when a container runs successfully', () => {
+    beforeEach(async () => {
+      await ImageService.pull('alpine')
+      container = await docker.createContainer({
+        Image: 'alpine',
+        AttachStdout: true,
+        AttachStderr: true,
+        Cmd: ['/bin/sh', '-c', 'echo $VAR'],
+        Env: ['VAR=env-var']
+      })
+    })
+
+    test('it returns true', async () => {
+      expect(await ContainerService.run(container)).toBe(true)
     })
   })
 
-  test('runs containers', async () => {
-    await ContainerService.run(container)
+  describe('when a container runs unsuccessfully', () => {
+    beforeEach(async () => {
+      await ImageService.pull('alpine')
+      container = await docker.createContainer({
+        Image: 'alpine',
+        AttachStdout: true,
+        AttachStderr: true,
+        Cmd: ['/bin/sh', '-c', 'nosuchccommand']
+      })
+    })
+
+    test('raises an exception', async () => {
+      await expect(ContainerService.run(container)).rejects.toThrow()
+    })
   })
 })

__tests__/main.test.ts

@@ -139,7 +139,9 @@ describe('run', () => {
 
       expect(reportJobErrorSpy).toHaveBeenCalledWith({
         'error-type': 'actions_workflow_unknown',
-        'error-detail': 'error getting job details'
+        'error-details': {
+          'action-error': 'error getting job details'
+        }
       })
       expect(markJobAsProcessedSpy).toHaveBeenCalled()
     })
@@ -173,7 +175,9 @@ describe('run', () => {
 
       expect(reportJobErrorSpy).toHaveBeenCalledWith({
         'error-type': 'actions_workflow_unknown',
-        'error-detail': 'error getting credentials'
+        'error-details': {
+          'action-error': 'error getting credentials'
+        }
       })
       expect(markJobAsProcessedSpy).toHaveBeenCalled()
     })
@@ -207,7 +211,9 @@ describe('run', () => {
 
       expect(reportJobErrorSpy).toHaveBeenCalledWith({
         'error-type': 'actions_workflow_image',
-        'error-detail': 'error pulling an image'
+        'error-details': {
+          'action-error': 'error pulling an image'
+        }
       })
       expect(markJobAsProcessedSpy).toHaveBeenCalled()
     })
@@ -241,7 +247,9 @@ describe('run', () => {
 
       expect(reportJobErrorSpy).toHaveBeenCalledWith({
         'error-type': 'actions_workflow_updater',
-        'error-detail': 'error running the update'
+        'error-details': {
+          'action-error': 'error running the update'
+        }
       })
       expect(markJobAsProcessedSpy).toHaveBeenCalled()
     })

__tests__/updater.test.ts

@@ -1,15 +1,22 @@
-import {UPDATER_IMAGE_NAME, PROXY_IMAGE_NAME} from '../src/main'
 import {Updater} from '../src/updater'
+import Docker from 'dockerode'
+import {ContainerService} from '../src/container-service'
+import {ProxyBuilder} from '../src/proxy'
+
+// We do not need to build actual containers or run updates for this test.)
+jest.mock('dockerode')
+jest.mock('../src/container-service')
+jest.mock('../src/proxy')
 
 describe('Updater', () => {
   const mockApiClient: any = {
     getJobDetails: jest.fn(),
     getCredentials: jest.fn(),
     params: {
       jobId: 1,
-      jobToken: process.env.JOB_TOKEN,
-      credentialsToken: process.env.CREDENTIALS_TOKEN,
-      dependabotApiUrl: 'http://host.docker.internal:3001'
+      jobToken: 'job-token',
+      credentialsToken: 'job-credentials-token',
+      dependabotApiUrl: 'http://localhost:3001'
     }
   }
 
@@ -23,18 +30,163 @@ describe('Updater', () => {
     'package-manager': 'npm-and-yarn'
   }
 
-  const updater = new Updater(
-    UPDATER_IMAGE_NAME,
-    PROXY_IMAGE_NAME,
-    mockApiClient,
-    mockJobDetails,
-    []
-  )
+  const mockProxy: any = {
+    container: {
+      start: jest.fn()
+    },
+    network: jest.fn(),
+    networkName: 'mockNetworkName',
+    url: 'http://localhost',
+    cert: 'mockCertificate',
+    shutdown: jest.fn()
+  }
+
+  const mockContainer: any = {
+    id: 1
+  }
+
+  afterEach(async () => {
+    jest.clearAllMocks() // Reset any mocked classes
+  })
+
+  describe('when there is a happy path update', () => {
+    const updater = new Updater(
+      'MOCK_UPDATER_IMAGE_NAME',
+      'MOCK_PROXY_IMAGE_NAME',
+      mockApiClient,
+      mockJobDetails,
+      [],
+      '../__fixtures__/output/happy_path/output.json'
+    )
+
+    beforeEach(async () => {
+      jest
+        .spyOn(Docker.prototype, 'createContainer')
+        .mockResolvedValue(mockContainer)
+
+      jest.spyOn(ProxyBuilder.prototype, 'run').mockResolvedValue(mockProxy)
+      jest.spyOn(ContainerService, 'run').mockImplementation(jest.fn())
+    })
+
+    it('should be successful', async () => {
+      expect(await updater.runUpdater()).toBe(true)
+    })
+  })
+
+  describe('when the file fetch container fails', () => {
+    const updater = new Updater(
+      'MOCK_UPDATER_IMAGE_NAME',
+      'MOCK_PROXY_IMAGE_NAME',
+      mockApiClient,
+      mockJobDetails,
+      [],
+      '../__fixtures__/output/happy_path/output.json'
+    )
+
+    beforeEach(async () => {
+      jest
+        .spyOn(Docker.prototype, 'createContainer')
+        .mockResolvedValue(mockContainer)
+
+      jest.spyOn(ProxyBuilder.prototype, 'run').mockResolvedValue(mockProxy)
+
+      jest
+        .spyOn(ContainerService, 'run')
+        .mockImplementationOnce(
+          jest.fn(async () =>
+            Promise.reject(new Error('First call to container service errored'))
+          )
+        )
+    })
+
+    it('should raise an error', async () => {
+      await expect(updater.runUpdater()).rejects.toThrow()
+    })
+  })
+
+  describe('when file updater container fails', () => {
+    const updater = new Updater(
+      'MOCK_UPDATER_IMAGE_NAME',
+      'MOCK_PROXY_IMAGE_NAME',
+      mockApiClient,
+      mockJobDetails,
+      [],
+      '../__fixtures__/output/happy_path/output.json'
+    )
+
+    beforeEach(async () => {
+      jest
+        .spyOn(Docker.prototype, 'createContainer')
+        .mockResolvedValue(mockContainer)
+
+      jest.spyOn(ProxyBuilder.prototype, 'run').mockResolvedValue(mockProxy)
+
+      jest
+        .spyOn(ContainerService, 'run')
+        .mockImplementationOnce(jest.fn())
+        .mockImplementationOnce(
+          jest.fn(async () =>
+            Promise.reject(
+              new Error('Second call to container service errored')
+            )
+          )
+        )
+    })
+
+    it('should raise an error', async () => {
+      await expect(updater.runUpdater()).rejects.toThrow()
+    })
+  })
+
+  describe('when the file fetch step results in empty output', () => {
+    const updater = new Updater(
+      'MOCK_UPDATER_IMAGE_NAME',
+      'MOCK_PROXY_IMAGE_NAME',
+      mockApiClient,
+      mockJobDetails,
+      [],
+      '../__fixtures__/output/empty/output.json'
+    )
+
+    beforeEach(async () => {
+      jest
+        .spyOn(Docker.prototype, 'createContainer')
+        .mockResolvedValue(mockContainer)
+
+      jest.spyOn(ProxyBuilder.prototype, 'run').mockResolvedValue(mockProxy)
+
+      jest.spyOn(ContainerService, 'run').mockImplementation(jest.fn())
+    })
+
+    it('should raise an error', async () => {
+      await expect(updater.runUpdater()).rejects.toThrow(
+        new Error('No output.json created by the fetcher container')
+      )
+    })
+  })
+
+  describe('when the file fetch step results in malformed output', () => {
+    const updater = new Updater(
+      'MOCK_UPDATER_IMAGE_NAME',
+      'MOCK_PROXY_IMAGE_NAME',
+      mockApiClient,
+      mockJobDetails,
+      [],
+      '../__fixtures__/output/malformed/output.json'
+    )
+
+    beforeEach(async () => {
+      jest
+        .spyOn(Docker.prototype, 'createContainer')
+        .mockResolvedValue(mockContainer)
+
+      jest.spyOn(ProxyBuilder.prototype, 'run').mockResolvedValue(mockProxy)
+
+      jest.spyOn(ContainerService, 'run').mockImplementation(jest.fn())
+    })
 
-  it('should fetch job details', async () => {
-    mockApiClient.getJobDetails.mockImplementation(() => {
-      throw new Error('kaboom')
+    it('should raise an error', async () => {
+      await expect(updater.runUpdater()).rejects.toThrow()
     })
-    updater.runUpdater()
   })
 })