Skip to content
This repository was archived by the owner on Mar 3, 2026. It is now read-only.

chore(deps): gcs-resumable-upload migration#1736

Merged
shaffeeullah merged 20 commits intomainfrom
shaffeeullah/gcsresumableuploadmigration
Jan 6, 2022
Merged

chore(deps): gcs-resumable-upload migration#1736
shaffeeullah merged 20 commits intomainfrom
shaffeeullah/gcsresumableuploadmigration

Conversation

@shaffeeullah
Copy link
Copy Markdown
Contributor

No description provided.

@product-auto-label product-auto-label Bot added the api: storage Issues related to the googleapis/nodejs-storage API. label Dec 22, 2021
@shaffeeullah shaffeeullah marked this pull request as ready for review December 29, 2021 18:20
Comment thread system-test/kitchen.ts Outdated
import {createURI, ErrorWithCode, upload} from '../src/gcs-resumable-upload';

const bucketName = process.env.BUCKET_NAME || 'gcs-resumable-upload-test';
const filePath = path.join(__dirname, '../../system-test/data/20MB.zip');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we can, let's remove this binary in favor of generating a temp file. See:

If we want a 20MB file of random bytes:

  • crypto.randomBytes(1024 * 1024 * 20)

This way we don't have to check in a test blob into the repo.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. I would agree as well, I'm not a huge fan of committing blobs.

@danielbankhead
Copy link
Copy Markdown
Contributor

After this is merged, we should consider a refactor:

  • to remove unnecessary deps like pumpify, abort-controller, is-stream, etc. in favor of native deps
  • more tightly integrate gcs-resumable-upload stream handling with File (shared type definitions, removing things that are optional that File wouldn't use)

Comment thread system-test/kitchen.ts Outdated
tmp.setGracefulCleanup();
const tmpFileContents = crypto.randomBytes(1024 * 1024 * 20);
const filePath = path.join(os.tmpdir(), '20MB.zip');
fs.createWriteStream(filePath).write(tmpFileContents);
Copy link
Copy Markdown
Contributor

@danielbankhead danielbankhead Jan 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danielbankhead
Copy link
Copy Markdown
Contributor

danielbankhead commented Jan 4, 2022

gcs-resumable-upload v4.0.0 has been released - should we include it in this PR?

@shaffeeullah shaffeeullah merged commit 437d400 into main Jan 6, 2022
@shaffeeullah shaffeeullah deleted the shaffeeullah/gcsresumableuploadmigration branch January 6, 2022 20:17
@danielduhh
Copy link
Copy Markdown
Contributor

late review here:
@danielbankhead do you mind adding issues for the follow up action items?
@shaffeeullah does the single index.ts file need to be nested in the gcs-resumable-media folder? We could just rename the file to resumable media and move under src/ with the rest of the package

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

api: storage Issues related to the googleapis/nodejs-storage API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants