artifact header cleanup (actions#441)

* Update NPM packages for @actions/artifact * Clarifications around headers * Revert NPM updates * Apply suggestions from code review Co-authored-by: Josh Gross <[email protected]> Co-authored-by: Josh Gross <[email protected]>
oflenake · May 12, 2020 · 0471ed4 · 0471ed4
1 parent d1b52e7
commit 0471ed4
Show file tree

Hide file tree

Showing 4 changed files with 47 additions and 52 deletions.
diff --git a/packages/artifact/__tests__/util.test.ts b/packages/artifact/__tests__/util.test.ts
@@ -120,55 +120,55 @@ describe('Utils', () => {
     const size = 24
     const uncompressedLength = 100
     const range = 'bytes 0-199/200'
-    const options = utils.getUploadRequestOptions(
+    const headers = utils.getUploadHeaders(
       contentType,
       true,
       true,
       uncompressedLength,
       size,
       range
     )
-    expect(Object.keys(options).length).toEqual(8)
-    expect(options['Accept']).toEqual(
+    expect(Object.keys(headers).length).toEqual(8)
+    expect(headers['Accept']).toEqual(
       `application/json;api-version=${utils.getApiVersion()}`
     )
-    expect(options['Content-Type']).toEqual(contentType)
-    expect(options['Connection']).toEqual('Keep-Alive')
-    expect(options['Keep-Alive']).toEqual('10')
-    expect(options['Content-Encoding']).toEqual('gzip')
-    expect(options['x-tfs-filelength']).toEqual(uncompressedLength)
-    expect(options['Content-Length']).toEqual(size)
-    expect(options['Content-Range']).toEqual(range)
+    expect(headers['Content-Type']).toEqual(contentType)
+    expect(headers['Connection']).toEqual('Keep-Alive')
+    expect(headers['Keep-Alive']).toEqual('10')
+    expect(headers['Content-Encoding']).toEqual('gzip')
+    expect(headers['x-tfs-filelength']).toEqual(uncompressedLength)
+    expect(headers['Content-Length']).toEqual(size)
+    expect(headers['Content-Range']).toEqual(range)
   })
 
   it('Test constructing upload headers with only required parameter', () => {
-    const options = utils.getUploadRequestOptions('application/octet-stream')
-    expect(Object.keys(options).length).toEqual(2)
-    expect(options['Accept']).toEqual(
+    const headers = utils.getUploadHeaders('application/octet-stream')
+    expect(Object.keys(headers).length).toEqual(2)
+    expect(headers['Accept']).toEqual(
       `application/json;api-version=${utils.getApiVersion()}`
     )
-    expect(options['Content-Type']).toEqual('application/octet-stream')
+    expect(headers['Content-Type']).toEqual('application/octet-stream')
   })
 
   it('Test constructing download headers with all optional parameters', () => {
     const contentType = 'application/json'
-    const options = utils.getDownloadRequestOptions(contentType, true, true)
-    expect(Object.keys(options).length).toEqual(5)
-    expect(options['Content-Type']).toEqual(contentType)
-    expect(options['Connection']).toEqual('Keep-Alive')
-    expect(options['Keep-Alive']).toEqual('10')
-    expect(options['Accept-Encoding']).toEqual('gzip')
-    expect(options['Accept']).toEqual(
+    const headers = utils.getDownloadHeaders(contentType, true, true)
+    expect(Object.keys(headers).length).toEqual(5)
+    expect(headers['Content-Type']).toEqual(contentType)
+    expect(headers['Connection']).toEqual('Keep-Alive')
+    expect(headers['Keep-Alive']).toEqual('10')
+    expect(headers['Accept-Encoding']).toEqual('gzip')
+    expect(headers['Accept']).toEqual(
       `application/octet-stream;api-version=${utils.getApiVersion()}`
     )
   })
 
   it('Test constructing download headers with only required parameter', () => {
-    const options = utils.getDownloadRequestOptions('application/octet-stream')
-    expect(Object.keys(options).length).toEqual(2)
-    expect(options['Content-Type']).toEqual('application/octet-stream')
+    const headers = utils.getDownloadHeaders('application/octet-stream')
+    expect(Object.keys(headers).length).toEqual(2)
+    expect(headers['Content-Type']).toEqual('application/octet-stream')
     // check for default accept type
-    expect(options['Accept']).toEqual(
+    expect(headers['Accept']).toEqual(
       `application/json;api-version=${utils.getApiVersion()}`
     )
   })

diff --git a/packages/artifact/src/internal/download-http-client.ts b/packages/artifact/src/internal/download-http-client.ts
@@ -3,7 +3,7 @@ import * as core from '@actions/core'
 import * as zlib from 'zlib'
 import {
   getArtifactUrl,
-  getDownloadRequestOptions,
+  getDownloadHeaders,
   isSuccessStatusCode,
   isRetryableStatusCode,
   isThrottledStatusCode,
@@ -40,8 +40,8 @@ export class DownloadHttpClient {
 
     // use the first client from the httpManager, `keep-alive` is not used so the connection will close immediately
     const client = this.downloadHttpManager.getClient(0)
-    const requestOptions = getDownloadRequestOptions('application/json')
-    const response = await client.get(artifactUrl, requestOptions)
+    const headers = getDownloadHeaders('application/json')
+    const response = await client.get(artifactUrl, headers)
     const body: string = await response.readBody()
 
     if (isSuccessStatusCode(response.message.statusCode) && body) {
@@ -68,8 +68,8 @@ export class DownloadHttpClient {
 
     // use the first client from the httpManager, `keep-alive` is not used so the connection will close immediately
     const client = this.downloadHttpManager.getClient(0)
-    const requestOptions = getDownloadRequestOptions('application/json')
-    const response = await client.get(resourceUrl.toString(), requestOptions)
+    const headers = getDownloadHeaders('application/json')
+    const response = await client.get(resourceUrl.toString(), headers)
     const body: string = await response.readBody()
 
     if (isSuccessStatusCode(response.message.statusCode) && body) {
@@ -149,22 +149,19 @@ export class DownloadHttpClient {
     let retryCount = 0
     const retryLimit = getRetryLimit()
     const destinationStream = fs.createWriteStream(downloadPath)
-    const requestOptions = getDownloadRequestOptions(
-      'application/json',
-      true,
-      true
-    )
+    const headers = getDownloadHeaders('application/json', true, true)
 
     // a single GET request is used to download a file
     const makeDownloadRequest = async (): Promise<IHttpClientResponse> => {
       const client = this.downloadHttpManager.getClient(httpClientIndex)
-      return await client.get(artifactLocation, requestOptions)
+      return await client.get(artifactLocation, headers)
     }
 
     // check the response headers to determine if the file was compressed using gzip
-    const isGzip = (headers: IncomingHttpHeaders): boolean => {
+    const isGzip = (incomingHeaders: IncomingHttpHeaders): boolean => {
       return (
-        'content-encoding' in headers && headers['content-encoding'] === 'gzip'
+        'content-encoding' in incomingHeaders &&
+        incomingHeaders['content-encoding'] === 'gzip'
       )
     }
 

diff --git a/packages/artifact/src/internal/upload-http-client.ts b/packages/artifact/src/internal/upload-http-client.ts
@@ -11,7 +11,7 @@ import {
 import {
   getArtifactUrl,
   getContentRange,
-  getUploadRequestOptions,
+  getUploadHeaders,
   isRetryableStatusCode,
   isSuccessStatusCode,
   isThrottledStatusCode,
@@ -63,8 +63,8 @@ export class UploadHttpClient {
 
     // use the first client from the httpManager, `keep-alive` is not used so the connection will close immediately
     const client = this.uploadHttpManager.getClient(0)
-    const requestOptions = getUploadRequestOptions('application/json', false)
-    const rawResponse = await client.post(artifactUrl, data, requestOptions)
+    const headers = getUploadHeaders('application/json', false)
+    const rawResponse = await client.post(artifactUrl, data, headers)
     const body: string = await rawResponse.readBody()
 
     if (isSuccessStatusCode(rawResponse.message.statusCode) && body) {
@@ -354,7 +354,7 @@ export class UploadHttpClient {
     totalFileSize: number
   ): Promise<boolean> {
     // prepare all the necessary headers before making any http call
-    const requestOptions = getUploadRequestOptions(
+    const headers = getUploadHeaders(
       'application/octet-stream',
       true,
       isGzip,
@@ -365,7 +365,7 @@ export class UploadHttpClient {
 
     const uploadChunkRequest = async (): Promise<IHttpClientResponse> => {
       const client = this.uploadHttpManager.getClient(httpClientIndex)
-      return await client.sendStream('PUT', resourceUrl, data, requestOptions)
+      return await client.sendStream('PUT', resourceUrl, data, headers)
     }
 
     let retryCount = 0
@@ -464,7 +464,7 @@ export class UploadHttpClient {
    * Updating the size indicates that we are done uploading all the contents of the artifact
    */
   async patchArtifactSize(size: number, artifactName: string): Promise<void> {
-    const requestOptions = getUploadRequestOptions('application/json', false)
+    const headers = getUploadHeaders('application/json', false)
     const resourceUrl = new URL(getArtifactUrl())
     resourceUrl.searchParams.append('artifactName', artifactName)
 
@@ -477,7 +477,7 @@ export class UploadHttpClient {
     const response: HttpClientResponse = await client.patch(
       resourceUrl.toString(),
       data,
-      requestOptions
+      headers
     )
     const body: string = await response.readBody()
     if (isSuccessStatusCode(response.message.statusCode)) {

diff --git a/packages/artifact/src/internal/utils.ts b/packages/artifact/src/internal/utils.ts
@@ -62,7 +62,6 @@ export function isForbiddenStatusCode(statusCode?: number): boolean {
   if (!statusCode) {
     return false
   }
-
   return statusCode === HttpCodes.Forbidden
 }
 
@@ -84,7 +83,6 @@ export function isThrottledStatusCode(statusCode?: number): boolean {
   if (!statusCode) {
     return false
   }
-
   return statusCode === HttpCodes.TooManyRequests
 }
 
@@ -133,9 +131,9 @@ export function getContentRange(
  * @param {boolean} isKeepAlive is the same connection being used to make multiple calls
  * @param {boolean} acceptGzip can we accept a gzip encoded response
  * @param {string} acceptType the type of content that we can accept
- * @returns appropriate request options to make a specific http call during artifact download
+ * @returns appropriate headers to make a specific http call during artifact download
  */
-export function getDownloadRequestOptions(
+export function getDownloadHeaders(
   contentType: string,
   isKeepAlive?: boolean,
   acceptGzip?: boolean
@@ -172,9 +170,9 @@ export function getDownloadRequestOptions(
  * @param {number} uncompressedLength the original size of the content if something is being uploaded that has been compressed
  * @param {number} contentLength the length of the content that is being uploaded
  * @param {string} contentRange the range of the content that is being uploaded
- * @returns appropriate request options to make a specific http call during artifact upload
+ * @returns appropriate headers to make a specific http call during artifact upload
  */
-export function getUploadRequestOptions(
+export function getUploadHeaders(
   contentType: string,
   isKeepAlive?: boolean,
   isGzip?: boolean,
@@ -207,7 +205,7 @@ export function getUploadRequestOptions(
 }
 
 export function createHttpClient(): HttpClient {
-  return new HttpClient('action/artifact', [
+  return new HttpClient('actions/artifact', [
     new BearerCredentialHandler(getRuntimeToken())
   ])
 }