Use Azure storage SDK to download cache (actions#497)

* Adds option to download using AzCopy * Bump version number and add release notes * Ensure we use at least v10 * Negate env var so it disables AzCopy * Use Azure storage SDK to download cache * Use same level of parallelism as AzCopy * Fix naming of variable * React to feedback * Bump Node types to Node 12 * Make linter happy * Pass options into restoreCache method * Fix tests * Restructure files and add tests * Add method to get the default download and upload options * Include breaking changes in RELEASES.md Co-authored-by: Josh Gross <[email protected]>
irinazheltisheva · Jul 10, 2020 · 4964b0c · 4964b0c
1 parent cee7d92
commit 4964b0c
Show file tree

Hide file tree

Showing 17 changed files with 968 additions and 290 deletions.
diff --git a/.github/workflows/cache-tests.yml b/.github/workflows/cache-tests.yml
@@ -58,17 +58,33 @@ jobs:
       run: |
         node -e "Promise.resolve(require('./packages/cache/lib/cache').saveCache(['test-cache','~/test-cache'],'test-${{ runner.os }}-${{ github.run_id }}'))"
 
-    - name: Delete cache folders prior to restore
+    - name: Delete cache folders before restoring
       shell: bash
       run: |
         rm -rf test-cache
         rm -rf ~/test-cache
 
-    - name: Restore cache using restoreCache()
+    - name: Restore cache using restoreCache() with http-client
+      run: |
+        node -e "Promise.resolve(require('./packages/cache/lib/cache').restoreCache(['test-cache','~/test-cache'],'test-${{ runner.os }}-${{ github.run_id }}',[],{useAzureSdk: false}))"
+
+    - name: Verify cache restored with http-client
+      shell: bash
+      run: |
+        packages/cache/__tests__/verify-cache-files.sh ${{ runner.os }} test-cache
+        packages/cache/__tests__/verify-cache-files.sh ${{ runner.os }} ~/test-cache
+
+    - name: Delete cache folders before restoring
+      shell: bash
+      run: |
+        rm -rf test-cache
+        rm -rf ~/test-cache
+
+    - name: Restore cache using restoreCache() with Azure SDK
       run: |
         node -e "Promise.resolve(require('./packages/cache/lib/cache').restoreCache(['test-cache','~/test-cache'],'test-${{ runner.os }}-${{ github.run_id }}'))"
 
-    - name: Verify cache 
+    - name: Verify cache restored with Azure SDK
       shell: bash
       run: |
         packages/cache/__tests__/verify-cache-files.sh ${{ runner.os }} test-cache

diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -14,7 +14,7 @@
   },
   "devDependencies": {
     "@types/jest": "^24.0.11",
-    "@types/node": "^11.13.5",
+    "@types/node": "^12.12.47",
     "@types/signale": "^1.2.1",
     "@typescript-eslint/parser": "^2.2.7",
     "concurrently": "^4.1.0",

diff --git a/packages/cache/RELEASES.md b/packages/cache/RELEASES.md
@@ -8,4 +8,9 @@
 - Fixes issues with the zstd compression algorithm on Windows and Ubuntu 16.04 [#469](https://github.com/actions/toolkit/pull/469)
 
 ### 0.2.1
-- Fix to await async function getCompressionMethod
+- Fix to await async function getCompressionMethod
+
+### 1.0.0
+- Downloads Azure-hosted caches using the Azure SDK for speed and reliability
+- Includes changes that break compatibility with earlier versions, including:
+  - `retry`, `retryTypedResponse`, and `retryHttpClientResponse` moved from `cacheHttpClient` to `requestUtils`
diff --git a/packages/cache/__tests__/cacheHttpClient.test.ts b/packages/cache/__tests__/cacheHttpClient.test.ts
@@ -1,5 +1,9 @@
-import {getCacheVersion, retry} from '../src/internal/cacheHttpClient'
+import {downloadCache, getCacheVersion} from '../src/internal/cacheHttpClient'
 import {CompressionMethod} from '../src/internal/constants'
+import * as downloadUtils from '../src/internal/downloadUtils'
+import {DownloadOptions, getDownloadOptions} from '../src/options'
+
+jest.mock('../src/internal/downloadUtils')
 
 test('getCacheVersion with one path returns version', async () => {
   const paths = ['node_modules']
@@ -35,141 +39,103 @@ test('getCacheVersion with gzip compression does not change vesion', async () =>
   )
 })
 
-interface TestResponse {
-  statusCode: number
-  result: string | null
-}
-
-async function handleResponse(
-  response: TestResponse | undefined
-): Promise<TestResponse> {
-  if (!response) {
-    // eslint-disable-next-line no-undef
-    fail('Retry method called too many times')
-  }
-
-  if (response.statusCode === 999) {
-    throw Error('Test Error')
-  } else {
-    return Promise.resolve(response)
-  }
-}
-
-async function testRetryExpectingResult(
-  responses: TestResponse[],
-  expectedResult: string | null
-): Promise<void> {
-  responses = responses.reverse() // Reverse responses since we pop from end
-
-  const actualResult = await retry(
-    'test',
-    async () => handleResponse(responses.pop()),
-    (response: TestResponse) => response.statusCode
-  )
-
-  expect(actualResult.result).toEqual(expectedResult)
-}
-
-async function testRetryExpectingError(
-  responses: TestResponse[]
-): Promise<void> {
-  responses = responses.reverse() // Reverse responses since we pop from end
-
-  expect(
-    retry(
-      'test',
-      async () => handleResponse(responses.pop()),
-      (response: TestResponse) => response.statusCode
-    )
-  ).rejects.toBeInstanceOf(Error)
-}
-
-test('retry works on successful response', async () => {
-  await testRetryExpectingResult(
-    [
-      {
-        statusCode: 200,
-        result: 'Ok'
-      }
-    ],
-    'Ok'
+test('downloadCache uses http-client for non-Azure URLs', async () => {
+  const downloadCacheHttpClientMock = jest.spyOn(
+    downloadUtils,
+    'downloadCacheHttpClient'
   )
-})
+  const downloadCacheStorageSDKMock = jest.spyOn(
+    downloadUtils,
+    'downloadCacheStorageSDK'
+  )
+
+  const archiveLocation = 'http://www.actionscache.test/download'
+  const archivePath = '/foo/bar'
 
-test('retry works after retryable status code', async () => {
-  await testRetryExpectingResult(
-    [
-      {
-        statusCode: 503,
-        result: null
-      },
-      {
-        statusCode: 200,
-        result: 'Ok'
-      }
-    ],
-    'Ok'
+  await downloadCache(archiveLocation, archivePath)
+
+  expect(downloadCacheHttpClientMock).toHaveBeenCalledTimes(1)
+  expect(downloadCacheHttpClientMock).toHaveBeenCalledWith(
+    archiveLocation,
+    archivePath
   )
-})
 
-test('retry fails after exhausting retries', async () => {
-  await testRetryExpectingError([
-    {
-      statusCode: 503,
-      result: null
-    },
-    {
-      statusCode: 503,
-      result: null
-    },
-    {
-      statusCode: 200,
-      result: 'Ok'
-    }
-  ])
+  expect(downloadCacheStorageSDKMock).toHaveBeenCalledTimes(0)
 })
 
-test('retry fails after non-retryable status code', async () => {
-  await testRetryExpectingError([
-    {
-      statusCode: 500,
-      result: null
-    },
-    {
-      statusCode: 200,
-      result: 'Ok'
-    }
-  ])
+test('downloadCache uses storage SDK for Azure storage URLs', async () => {
+  const downloadCacheHttpClientMock = jest.spyOn(
+    downloadUtils,
+    'downloadCacheHttpClient'
+  )
+  const downloadCacheStorageSDKMock = jest.spyOn(
+    downloadUtils,
+    'downloadCacheStorageSDK'
+  )
+
+  const archiveLocation = 'http://foo.blob.core.windows.net/bar/baz'
+  const archivePath = '/foo/bar'
+
+  await downloadCache(archiveLocation, archivePath)
+
+  expect(downloadCacheStorageSDKMock).toHaveBeenCalledTimes(1)
+  expect(downloadCacheStorageSDKMock).toHaveBeenCalledWith(
+    archiveLocation,
+    archivePath,
+    getDownloadOptions()
+  )
+
+  expect(downloadCacheHttpClientMock).toHaveBeenCalledTimes(0)
 })
 
-test('retry works after error', async () => {
-  await testRetryExpectingResult(
-    [
-      {
-        statusCode: 999,
-        result: null
-      },
-      {
-        statusCode: 200,
-        result: 'Ok'
-      }
-    ],
-    'Ok'
+test('downloadCache passes options to download methods', async () => {
+  const downloadCacheHttpClientMock = jest.spyOn(
+    downloadUtils,
+    'downloadCacheHttpClient'
   )
+  const downloadCacheStorageSDKMock = jest.spyOn(
+    downloadUtils,
+    'downloadCacheStorageSDK'
+  )
+
+  const archiveLocation = 'http://foo.blob.core.windows.net/bar/baz'
+  const archivePath = '/foo/bar'
+  const options: DownloadOptions = {downloadConcurrency: 4}
+
+  await downloadCache(archiveLocation, archivePath, options)
+
+  expect(downloadCacheStorageSDKMock).toHaveBeenCalledTimes(1)
+  expect(downloadCacheStorageSDKMock).toHaveBeenCalled()
+  expect(downloadCacheStorageSDKMock).toHaveBeenCalledWith(
+    archiveLocation,
+    archivePath,
+    getDownloadOptions(options)
+  )
+
+  expect(downloadCacheHttpClientMock).toHaveBeenCalledTimes(0)
 })
 
-test('retry returns after client error', async () => {
-  await testRetryExpectingResult(
-    [
-      {
-        statusCode: 400,
-        result: null
-      },
-      {
-        statusCode: 200,
-        result: 'Ok'
-      }
-    ],
-    null
+test('downloadCache uses http-client when overridden', async () => {
+  const downloadCacheHttpClientMock = jest.spyOn(
+    downloadUtils,
+    'downloadCacheHttpClient'
   )
+  const downloadCacheStorageSDKMock = jest.spyOn(
+    downloadUtils,
+    'downloadCacheStorageSDK'
+  )
+
+  const archiveLocation = 'http://foo.blob.core.windows.net/bar/baz'
+  const archivePath = '/foo/bar'
+  const options: DownloadOptions = {useAzureSdk: false}
+
+  await downloadCache(archiveLocation, archivePath, options)
+
+  expect(downloadCacheHttpClientMock).toHaveBeenCalledTimes(1)
+  expect(downloadCacheHttpClientMock).toHaveBeenCalledWith(
+    archiveLocation,
+    archivePath
+  )
+
+  expect(downloadCacheStorageSDKMock).toHaveBeenCalledTimes(0)
 })
diff --git a/packages/cache/__tests__/cacheUtils.test.ts b/packages/cache/__tests__/cacheUtils.test.ts
@@ -24,3 +24,11 @@ test('unlinkFile unlinks file', async () => {
 
   await fs.rmdir(testDirectory)
 })
+
+test('assertDefined throws if undefined', () => {
+  expect(() => cacheUtils.assertDefined('test', undefined)).toThrowError()
+})
+
+test('assertDefined returns value', () => {
+  expect(cacheUtils.assertDefined('test', 5)).toBe(5)
+})
diff --git a/packages/cache/__tests__/options.test.ts b/packages/cache/__tests__/options.test.ts
@@ -0,0 +1,54 @@
+import {
+  DownloadOptions,
+  UploadOptions,
+  getDownloadOptions,
+  getUploadOptions
+} from '../src/options'
+
+const useAzureSdk = true
+const downloadConcurrency = 8
+const timeoutInMs = 30000
+const uploadConcurrency = 4
+const uploadChunkSize = 32 * 1024 * 1024
+
+test('getDownloadOptions sets defaults', async () => {
+  const actualOptions = getDownloadOptions()
+
+  expect(actualOptions).toEqual({
+    useAzureSdk,
+    downloadConcurrency,
+    timeoutInMs
+  })
+})
+
+test('getDownloadOptions overrides all settings', async () => {
+  const expectedOptions: DownloadOptions = {
+    useAzureSdk: false,
+    downloadConcurrency: 14,
+    timeoutInMs: 20000
+  }
+
+  const actualOptions = getDownloadOptions(expectedOptions)
+
+  expect(actualOptions).toEqual(expectedOptions)
+})
+
+test('getUploadOptions sets defaults', async () => {
+  const actualOptions = getUploadOptions()
+
+  expect(actualOptions).toEqual({
+    uploadConcurrency,
+    uploadChunkSize
+  })
+})
+
+test('getUploadOptions overrides all settings', async () => {
+  const expectedOptions: UploadOptions = {
+    uploadConcurrency: 2,
+    uploadChunkSize: 16 * 1024 * 1024
+  }
+
+  const actualOptions = getUploadOptions(expectedOptions)
+
+  expect(actualOptions).toEqual(expectedOptions)
+})