Fixes #120 Artifacts can be removed even when new ones aren't provided

__tests__/Action.test.ts

@@ -4,6 +4,7 @@ import {Inputs} from "../src/Inputs";
 import {Releases} from "../src/Releases";
 import {ArtifactUploader} from "../src/ArtifactUploader";
 import {Outputs} from "../src/Outputs";
+import {ArtifactDestroyer} from "../src/ArtifactDestroyer";
 
 const applyReleaseDataMock = jest.fn()
 const createMock = jest.fn()
@@ -13,6 +14,7 @@ const listArtifactsMock = jest.fn()
 const listMock = jest.fn()
 const updateMock = jest.fn()
 const uploadMock = jest.fn()
+const artifactDestroyMock = jest.fn()
 
 const artifacts = [
     new Artifact('a/art1'),
@@ -29,7 +31,6 @@ const createPrerelease = true
 const updatePrerelease = false
 const releaseId = 101
 const replacesArtifacts = true
-const removeArtifacts = false
 const tag = 'tag'
 const token = 'token'
 const updateBody = 'updateBody'
@@ -95,6 +96,24 @@ describe("Action", () => {
         assertOutputApplied()
     })
 
+    it('removes all artifacts when artifact destroyer is enabled', async () => {
+        const action = createAction(false, true, true)
+
+        await action.perform()
+
+        expect(artifactDestroyMock).toBeCalledWith(releaseId)
+        assertOutputApplied()
+    })
+
+    it('removes no artifacts when artifact destroyer is disabled', async () => {
+        const action = createAction(false, true)
+
+        await action.perform()
+
+        expect(artifactDestroyMock).not.toBeCalled()
+        assertOutputApplied()
+    })
+
     it('throws error when create fails', async () => {
         const action = createAction(false, true)
         createMock.mockRejectedValue("error")
@@ -245,7 +264,7 @@ describe("Action", () => {
         expect(applyReleaseDataMock).toBeCalledWith({id: releaseId, upload_url: url})
     }
 
-    function createAction(allowUpdates: boolean, hasArtifact: boolean): Action {
+    function createAction(allowUpdates: boolean, hasArtifact: boolean, removeArtifacts: boolean = false): Action {
         let inputArtifact: Artifact[]
         if (hasArtifact) {
             inputArtifact = artifacts
@@ -318,12 +337,18 @@ describe("Action", () => {
                 uploadArtifacts: uploadMock
             }
         })
+        const MockArtifactDestroyer = jest.fn<ArtifactDestroyer, any>(() => {
+            return {
+                destroyArtifacts: artifactDestroyMock
+            }
+        })
 
         const inputs = new MockInputs()
         const outputs = new MockOutputs()
         const releases = new MockReleases()
         const uploader = new MockUploader()
+        const artifactDestroyer = new MockArtifactDestroyer()
 
-        return new Action(inputs, outputs, releases, uploader)
+        return new Action(inputs, outputs, releases, uploader, artifactDestroyer)
     }
 })

__tests__/ArtifactDestroyer.test.ts

@@ -0,0 +1,90 @@
+import {Artifact} from "../src/Artifact"
+import {GithubArtifactUploader} from "../src/ArtifactUploader"
+import {Releases} from "../src/Releases";
+import {RequestError} from '@octokit/request-error'
+import {GithubArtifactDestroyer} from "../src/ArtifactDestroyer";
+
+const releaseId = 100
+
+const deleteMock = jest.fn()
+const listArtifactsMock = jest.fn()
+
+
+describe('ArtifactDestroyer', () => {
+    beforeEach(() => {
+        deleteMock.mockClear()
+        listArtifactsMock.mockClear()
+    })
+
+    it('destroys all artifacts', async () => {
+        mockListWithAssets()
+        mockDeleteSuccess()
+        const destroyer = createDestroyer()
+
+        await destroyer.destroyArtifacts(releaseId)
+
+        expect(deleteMock).toBeCalledTimes(2)
+    })
+
+    it('destroys nothing when no artifacts found', async () => {
+        mockListWithoutAssets()
+        const destroyer = createDestroyer()
+
+        await destroyer.destroyArtifacts(releaseId)
+
+        expect(deleteMock).toBeCalledTimes(0)
+    })
+
+    it('throws when delete call fails', async () => {
+        mockListWithAssets()
+        mockDeleteError()
+        const destroyer = createDestroyer()
+        
+        expect.hasAssertions()
+        try {
+            await destroyer.destroyArtifacts(releaseId)
+        } catch (error) {
+            expect(error).toEqual("error")
+        }
+    })
+
+    function createDestroyer(): GithubArtifactDestroyer {
+        const MockReleases = jest.fn<Releases, any>(() => {
+            return {
+                create: jest.fn(),
+                deleteArtifact: deleteMock,
+                getByTag: jest.fn(),
+                listArtifactsForRelease: listArtifactsMock,
+                listReleases: jest.fn(),
+                update: jest.fn(),
+                uploadArtifact: jest.fn()
+            }
+        })
+        return new GithubArtifactDestroyer(new MockReleases())
+    }
+
+    function mockDeleteError(): any {
+        deleteMock.mockRejectedValue("error")
+    }
+
+    function mockDeleteSuccess(): any {
+        deleteMock.mockResolvedValue({})
+    }
+
+    function mockListWithAssets() {
+        listArtifactsMock.mockResolvedValue([
+            {
+                name: "art1",
+                id: 1
+            },
+            {
+                name: "art2",
+                id: 2
+            }
+        ])
+    }
+
+    function mockListWithoutAssets() {
+        listArtifactsMock.mockResolvedValue([])
+    }
+});

__tests__/ArtifactUploader.test.ts

@@ -89,25 +89,6 @@ describe('ArtifactUploader', () => {
         expect(deleteMock).toBeCalledWith(2)
     })
 
-    it('removes all artifacts', async () => {
-        mockDeleteSuccess()
-        mockListWithAssets()
-        mockUploadArtifact()
-        const uploader = createUploader(false, true)
-
-        await uploader.uploadArtifacts(artifacts, releaseId, url)
-
-        expect(uploadMock).toBeCalledTimes(2)
-        expect(uploadMock)
-            .toBeCalledWith(url, contentLength, 'raw', fileContents, 'art1', releaseId)
-        expect(uploadMock)
-            .toBeCalledWith(url, contentLength, 'raw', fileContents, 'art2', releaseId)
-
-        expect(deleteMock).toBeCalledTimes(2)
-        expect(deleteMock).toBeCalledWith(1)
-        expect(deleteMock).toBeCalledWith(2)
-    })
-
     it('replaces no artifacts when previous asset list empty', async () => {
         mockDeleteSuccess()
         mockListWithoutAssets()
@@ -148,7 +129,7 @@ describe('ArtifactUploader', () => {
     it('throws upload error when replacesExistingArtifacts is true', async () => {
         mockListWithoutAssets()
         mockUploadError()
-        const uploader = createUploader(true, false, true)
+        const uploader = createUploader(true, true)
 
         expect.hasAssertions()
         try {
@@ -189,7 +170,7 @@ describe('ArtifactUploader', () => {
         expect(deleteMock).toBeCalledTimes(0)
     })
 
-    function createUploader(replaces: boolean, removes: boolean = false, throws: boolean = false): GithubArtifactUploader {
+    function createUploader(replaces: boolean, throws: boolean = false): GithubArtifactUploader {
         const MockReleases = jest.fn<Releases, any>(() => {
             return {
                 create: jest.fn(),
@@ -201,7 +182,7 @@ describe('ArtifactUploader', () => {
                 uploadArtifact: uploadMock
             }
         })
-        return new GithubArtifactUploader(new MockReleases(), replaces, removes, throws)
+        return new GithubArtifactUploader(new MockReleases(), replaces, throws)
     }
 
     function mockDeleteError(): any {

__tests__/Integration.test.ts

@@ -6,6 +6,7 @@ import {GithubArtifactUploader} from "../src/ArtifactUploader";
 import * as path from "path";
 import {FileArtifactGlobber} from "../src/ArtifactGlobber";
 import {Outputs} from "../src/Outputs";
+import {GithubArtifactDestroyer} from "../src/ArtifactDestroyer";
 
 // This test is currently intended to be manually run during development. To run:
 // - Make sure you have an environment variable named GITHUB_TOKEN assigned to your token
@@ -23,10 +24,11 @@ describe.skip('Integration Test', () => {
         const uploader = new GithubArtifactUploader(
             releases,
             inputs.replacesArtifacts,
-            inputs.removeArtifacts,
             inputs.artifactErrorsFailBuild,
         )
-        action = new Action(inputs, outputs, releases, uploader)
+        const artifactDestroyer = new GithubArtifactDestroyer(releases)
+        
+        action = new Action(inputs, outputs, releases, uploader, artifactDestroyer)
     })
 
     it('Performs action', async () => {

src/Action.ts

@@ -9,18 +9,25 @@ import {
 import {ArtifactUploader} from "./ArtifactUploader";
 import {GithubError} from "./GithubError";
 import {Outputs} from "./Outputs";
+import {ArtifactDestroyer} from "./ArtifactDestroyer";
 
 export class Action {
     private inputs: Inputs
     private outputs: Outputs
     private releases: Releases
+    private artifactDestroyer: ArtifactDestroyer
     private uploader: ArtifactUploader
 
-    constructor(inputs: Inputs, outputs: Outputs, releases: Releases, uploader: ArtifactUploader) {
+    constructor(inputs: Inputs,
+                outputs: Outputs,
+                releases: Releases,
+                uploader: ArtifactUploader,
+                artifactDestroyer: ArtifactDestroyer) {
         this.inputs = inputs
         this.outputs = outputs
         this.releases = releases
         this.uploader = uploader
+        this.artifactDestroyer = artifactDestroyer
     }
 
     async perform() {
@@ -28,12 +35,16 @@ export class Action {
         const releaseData = releaseResponse.data
         const releaseId = releaseData.id
         const uploadUrl = releaseData.upload_url
-
+        
+        if (this.inputs.removeArtifacts) {
+            await this.artifactDestroyer.destroyArtifacts(releaseId)
+        }
+        
         const artifacts = this.inputs.artifacts
         if (artifacts.length > 0) {
             await this.uploader.uploadArtifacts(artifacts, releaseId, uploadUrl)
         }
-        
+
         this.outputs.applyReleaseData(releaseData)
     }
 

src/ArtifactDestroyer.ts

@@ -0,0 +1,20 @@
+import {Releases} from "./Releases";
+import * as core from "@actions/core";
+
+export interface ArtifactDestroyer {
+    destroyArtifacts(releaseId: number): Promise<void>
+}
+
+export class GithubArtifactDestroyer implements ArtifactDestroyer {
+    constructor(private releases: Releases) {
+    }
+
+    async destroyArtifacts(releaseId: number): Promise<void> {
+        const releaseAssets = await this.releases.listArtifactsForRelease(releaseId)
+        for (const artifact of releaseAssets) {
+            const asset = artifact
+            core.debug(`Deleting existing artifact ${artifact.name}...`)
+            await this.releases.deleteArtifact(asset.id)
+        }
+    }
+}

src/ArtifactUploader.ts

@@ -10,7 +10,6 @@ export class GithubArtifactUploader implements ArtifactUploader {
     constructor(
         private releases: Releases,
         private replacesExistingArtifacts: boolean = true,
-        private removeArtifacts: boolean = false,
         private throwsUploadErrors: boolean = false,
     ) {
     }
@@ -21,9 +20,6 @@ export class GithubArtifactUploader implements ArtifactUploader {
         if (this.replacesExistingArtifacts) {
             await this.deleteUpdatedArtifacts(artifacts, releaseId)
         }
-        if (this.removeArtifacts) {
-            await this.deleteUploadedArtifacts(releaseId)
-        }
         for (const artifact of artifacts) {
             await this.uploadArtifact(artifact, releaseId, uploadUrl)
         }
@@ -69,13 +65,4 @@ export class GithubArtifactUploader implements ArtifactUploader {
             }
         }
     }
-
-    private async deleteUploadedArtifacts(releaseId: number): Promise<void> {
-        const releaseAssets = await this.releases.listArtifactsForRelease(releaseId)
-        for (const artifact of releaseAssets) {
-            const asset = artifact
-            core.debug(`Deleting existing artifact ${artifact.name}...`)
-            await this.releases.deleteArtifact(asset.id)
-        }
-    }
 }

src/Main.ts

@@ -7,6 +7,7 @@ import {GithubArtifactUploader} from './ArtifactUploader';
 import {FileArtifactGlobber} from './ArtifactGlobber';
 import {GithubError} from './GithubError';
 import {CoreOutputs} from "./Outputs";
+import {GithubArtifactDestroyer} from "./ArtifactDestroyer";
 
 async function run() {
     try {
@@ -27,8 +28,10 @@ function createAction(): Action {
     const inputs = new CoreInputs(globber, context)
     const outputs = new CoreOutputs()
     const releases = new GithubReleases(inputs, git)
-    const uploader = new GithubArtifactUploader(releases, inputs.replacesArtifacts, inputs.removeArtifacts, inputs.artifactErrorsFailBuild)
-    return new Action(inputs, outputs, releases, uploader)
+    const uploader = new GithubArtifactUploader(releases, inputs.replacesArtifacts, inputs.artifactErrorsFailBuild)
+    const artifactDestroyer = new GithubArtifactDestroyer(releases)
+    
+    return new Action(inputs, outputs, releases, uploader, artifactDestroyer)
 }
 
 run();