Skip to content

Commit 6c467dd

Browse files
authored
feat: refactor uploading and ignore processing (#255)
* feat: refactor uploading and ignore processing This refactors the implementation for listing, ignoring, and uploading files. The previous implementation relied on the UploadHelper to process files and globs, and it led to a lot of translation between relative and absolute paths. It also added a lot of layers of indirection and a really tight coupling between the filesystem and the uploader. The new implementation pre-processes all files and ignores first, then delegates uploading on the complete list of files. This means the uploader does not need to behave differently when given a file versus a directory. This makes it easier to stub only the calls to storage.bucket.upload and truly exercise the entire pipeline in unit tests. As part of this refactor, I dropped the pMap dependency. We've been pinned at an old version of this dependency because the new version is incompatible. There's a new function, inParallel that executes Promises in parallel. I'll move it to actions-utils after more battle testing. * Address some, but not all, feedback * test
1 parent 9f24270 commit 6c467dd

14 files changed

Lines changed: 1267 additions & 1589 deletions

action.yml

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,8 +45,9 @@ inputs:
4545
required: false
4646
concurrency:
4747
description: |-
48-
Number of files to simultaneously upload, defaults to 100.
48+
Number of files to simultaneously upload.
4949
required: false
50+
default: 100
5051
headers:
5152
description: |-
5253
Set headers (object metadata).

dist/index.js

Lines changed: 20 additions & 20 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

package-lock.json

Lines changed: 197 additions & 319 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

package.json

Lines changed: 13 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,8 @@
55
"main": "dist/index.js",
66
"scripts": {
77
"build": "ncc build -m src/main.ts",
8-
"lint": "eslint . --ext .ts,.tsx --max-warnings 0",
9-
"format": "eslint . --fix --ext .ts,.tsx",
8+
"lint": "eslint src/ tests/ --ext .ts,.tsx",
9+
"format": "eslint src/ tests/ --ext .ts,.tsx --fix",
1010
"test": "mocha -r ts-node/register -t 600s 'tests/**/*.test.ts'"
1111
},
1212
"repository": {
@@ -24,29 +24,26 @@
2424
"license": "Apache-2.0",
2525
"dependencies": {
2626
"@actions/core": "^1.6.0",
27-
"@google-cloud/storage": "^5.19.0",
28-
"@google-github-actions/actions-utils": "^0.1.6",
27+
"@google-cloud/storage": "^5.19.3",
28+
"@google-github-actions/actions-utils": "^0.3.0",
2929
"globby": "^11.0.4",
30-
"ignore": "^5.2.0",
31-
"p-map": "^4.0.0"
30+
"ignore": "^5.2.0"
3231
},
3332
"devDependencies": {
34-
"@types/chai": "^4.3.0",
35-
"@types/mocha": "^9.1.0",
36-
"@types/node": "^17.0.23",
33+
"@types/chai": "^4.3.1",
34+
"@types/mocha": "^9.1.1",
35+
"@types/node": "^17.0.25",
3736
"@types/sinon": "^10.0.11",
38-
"@types/tmp": "^0.2.3",
39-
"@typescript-eslint/eslint-plugin": "^5.18.0",
40-
"@typescript-eslint/parser": "^5.18.0",
41-
"@vercel/ncc": "^0.33.3",
37+
"@typescript-eslint/eslint-plugin": "^5.20.0",
38+
"@typescript-eslint/parser": "^5.20.0",
39+
"@vercel/ncc": "^0.33.4",
4240
"chai": "^4.3.6",
4341
"eslint-config-prettier": "^8.5.0",
4442
"eslint-plugin-prettier": "^4.0.0",
45-
"eslint": "^8.12.0",
43+
"eslint": "^8.13.0",
4644
"mocha": "^9.2.2",
4745
"prettier": "^2.6.2",
48-
"sinon": "^13.0.1",
49-
"tmp": "^0.2.1",
46+
"sinon": "^13.0.2",
5047
"ts-node": "^10.7.0",
5148
"typescript": "^4.6.3"
5249
}

src/client.ts

Lines changed: 127 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -14,15 +14,18 @@
1414
* limitations under the License.
1515
*/
1616

17-
import * as fs from 'fs';
1817
import * as path from 'path';
1918

20-
import { Storage, UploadResponse, StorageOptions, PredefinedAcl } from '@google-cloud/storage';
21-
import { parseCredential } from '@google-github-actions/actions-utils';
22-
import { Ignore } from 'ignore';
19+
import { Storage, StorageOptions, PredefinedAcl } from '@google-cloud/storage';
20+
import {
21+
parseCredential,
22+
randomFilepath,
23+
inParallel,
24+
toPlatformPath,
25+
} from '@google-github-actions/actions-utils';
2326

24-
import { UploadHelper } from './upload-helper';
2527
import { Metadata } from './headers';
28+
import { parseBucketNameAndPrefix } from './util';
2629

2730
// Do not listen to the linter - this can NOT be rewritten as an ES6 import statement.
2831
// eslint-disable-next-line @typescript-eslint/no-var-requires
@@ -36,11 +39,81 @@ const userAgent = `google-github-actions:upload-cloud-storage/${appVersion}`;
3639
*
3740
* @param credentials GCP JSON credentials (default uses ADC).
3841
*/
39-
4042
type ClientOptions = {
4143
credentials?: string;
44+
projectID?: string;
4245
};
4346

47+
/**
48+
* ClientUploadOptions is the list of available options during file upload.
49+
*/
50+
export interface ClientUploadOptions {
51+
/**
52+
* destination is the name of the bucket and optionally the path within the
53+
* bucket in which to upload. This value is split on the first instance of a
54+
* slash character. Everything preceeding of the first slash is the bucket
55+
* name, everything following the first slash is the path.
56+
*/
57+
destination: string;
58+
59+
/**
60+
* root is the parent directory from which all files originated on local disk.
61+
* This must be the platform-specific path separators.
62+
*/
63+
root: string;
64+
65+
/**
66+
* files is the list of absolute file paths on local disk to upload. This list
67+
* must use posix path separators for files.
68+
*/
69+
files: string[];
70+
71+
/**
72+
* concurrency is the maximum number of parallel upload operations that will
73+
* take place.
74+
*/
75+
concurrency?: number;
76+
77+
/**
78+
* includeParent indicates whether the local directory parent name (dirname of
79+
* root) should be included in the destination path in the bucket.
80+
*/
81+
includeParent?: boolean;
82+
83+
/**
84+
* metadata is object metadata to set. These are usually populated from
85+
* headers.
86+
*/
87+
metadata?: Metadata;
88+
89+
/**
90+
* gzip indicates whether to gzip the object when uploading.
91+
*/
92+
gzip?: boolean;
93+
94+
/**
95+
* resumable indicates whether the upload should be resumable after interrupt.
96+
*/
97+
resumable?: boolean;
98+
99+
/**
100+
* predefinedAcl defines the default ACL to apply to new objects.
101+
*/
102+
predefinedAcl?: PredefinedAcl;
103+
104+
/**
105+
* onUploadObject is called each time an object upload begins.
106+
**/
107+
onUploadObject?: FOnUploadObject;
108+
}
109+
110+
/**
111+
* FOnUploadObject is the function interface for the upload callback signature.
112+
*/
113+
interface FOnUploadObject {
114+
(source: string, destination: string, opts?: Record<string, unknown>): void;
115+
}
116+
44117
/**
45118
* Handles credential lookup, registration and wraps interactions with the GCS
46119
* Helper.
@@ -51,7 +124,10 @@ export class Client {
51124
readonly storage: Storage;
52125

53126
constructor(opts?: ClientOptions) {
54-
const options: StorageOptions = { userAgent: userAgent };
127+
const options: StorageOptions = {
128+
projectId: opts?.projectID,
129+
userAgent: userAgent,
130+
};
55131

56132
if (opts?.credentials) {
57133
options.credentials = parseCredential(opts.credentials);
@@ -60,76 +136,52 @@ export class Client {
60136
}
61137

62138
/**
63-
* Invokes GCS Helper for uploading file or directory.
64-
* @param destination Name of bucket and optional prefix to upload file/dir.
65-
* @param filePath FilePath of the file/dir to upload.
66-
* @param glob Glob pattern if any.
67-
* @param gzip Gzip files on upload.
68-
* @param resumable Allow resuming uploads.
69-
* @param parent Flag to enable parent dir in destination path.
70-
* @param predefinedAcl Predefined ACL config.
71-
* @param concurrency Number of files to simultaneously upload.
72-
* @returns List of uploaded file(s).
139+
* upload puts the given collection of files into the bucket. It will
140+
* overwrite any existing objects with the same name and create any new
141+
* objects. It does not delete any existing objects.
142+
*
143+
* @param opts ClientUploadOptions
144+
*
145+
* @return The list of files uploaded.
73146
*/
74-
async upload(
75-
destination: string,
76-
filePath: string,
77-
glob = '',
78-
gzip = true,
79-
resumable = true,
80-
parent = true,
81-
predefinedAcl?: PredefinedAcl,
82-
concurrency = 100,
83-
metadata?: Metadata,
84-
ignores?: Ignore,
85-
): Promise<UploadResponse[]> {
86-
let bucketName = destination;
87-
let prefix = '';
88-
// If destination of the form my-bucket/subfolder get bucket and prefix.
89-
const idx = destination.indexOf('/');
90-
if (idx > -1) {
91-
bucketName = destination.substring(0, idx);
92-
prefix = destination.substring(idx + 1);
93-
}
147+
async upload(opts: ClientUploadOptions): Promise<string[]> {
148+
const [bucket, prefix] = parseBucketNameAndPrefix(opts.destination);
94149

95-
const stat = await fs.promises.stat(filePath);
96-
const uploader = new UploadHelper(this.storage);
97-
if (stat.isFile()) {
98-
destination = '';
99-
// If obj prefix is set, then extract filename and append to prefix to create destination
100-
if (prefix) {
101-
destination = path.posix.join(prefix, path.posix.basename(filePath));
102-
}
103-
const uploadedFile = await uploader.uploadFile(
104-
bucketName,
105-
filePath,
106-
gzip,
107-
resumable,
108-
destination,
109-
predefinedAcl,
110-
metadata,
111-
ignores,
112-
);
113-
114-
if (uploadedFile) {
115-
return [uploadedFile];
150+
const storageBucket = this.storage.bucket(bucket);
151+
152+
const uploadOne = async (file: string): Promise<string> => {
153+
// Calculate destination by joining the prefix (if one exists), the parent
154+
// directory name (if includeParent is true), and the file name. path.join
155+
// ignores empty strings.
156+
const base = opts.includeParent ? path.basename(opts.root) : '';
157+
const destination = path.posix.join(prefix, base, file);
158+
159+
// Build options
160+
const abs = path.resolve(opts.root, toPlatformPath(file));
161+
const uploadOpts = {
162+
destination: destination,
163+
metadata: opts.metadata || {},
164+
gzip: opts.gzip,
165+
predefinedAcl: opts.predefinedAcl,
166+
resumable: opts.resumable,
167+
configPath: randomFilepath(),
168+
};
169+
170+
// Execute callback if defined
171+
if (opts.onUploadObject) {
172+
opts.onUploadObject(abs, path.posix.join(bucket, destination), uploadOpts);
116173
}
117-
return [];
118-
} else {
119-
const uploadedFiles = await uploader.uploadDirectory(
120-
bucketName,
121-
filePath,
122-
glob,
123-
gzip,
124-
resumable,
125-
prefix,
126-
parent,
127-
predefinedAcl,
128-
concurrency,
129-
metadata,
130-
ignores,
131-
);
132-
return uploadedFiles;
133-
}
174+
175+
// Do the upload
176+
const response = await storageBucket.upload(abs, uploadOpts);
177+
const name = response[0].name;
178+
return name;
179+
};
180+
181+
const args: [file: string][] = opts.files.map((file) => [file]);
182+
const results = await inParallel(uploadOne, args, {
183+
concurrency: opts.concurrency,
184+
});
185+
return results;
134186
}
135187
}

0 commit comments

Comments
 (0)