Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[rush-lib] Add AWS support for build cache feature #2475

Merged
merged 8 commits into from
Mar 10, 2021

Conversation

raihle
Copy link
Contributor

@raihle raihle commented Feb 9, 2021

Summary

AWS adapter for #2393. Build output can be cached in and fetched from an S3 bucket.

Details

Uses aws-sdk-js-v3, which has some unfortunate characteristics:

  • It's not as big as v2, but still adds ~15 MB
  • Supports browsers and React Native, bringing in some otherwise unnecessary dependencies in order to do so.
    • I've overridden the worst offenders in pnpmfile.js.
    • I've set skipLibCheck in tsconfig.json, to work around the fact that AWS SDK types are often a union of Node, DOM, and React Native types, while we only want to know about Node types.
  • Relies on some TypeScript 4 features.
    • I've upgraded and made some minor fixes across the repo.

I haven't implemented the --interactive flag, except for providing a somewhat useful error message. I don't know of any flows (similar to the Azure implementation) a developer could use in order to authenticate rush with AWS.

How it was tested

Added the same basic set of unit tests as the Azure integration (no check for region names, as valid options are not exposed by AWS SDK).

Enabled the experiment and configured one of our in-house projects to use AWS S3 for build cache.

  • Ran rush build
  • Made changes
  • Ran rush build again
  • Deleted the local temp/build-cache folder
  • Changed back to the original code
  • Ran rush build and confirmed that it was using the uploaded cache-files

Repeated the same experiment, but deleted the uploaded cache-files before the final step to confirm that it wasn't magically finding the files somewhere else.

@ghost
Copy link

ghost commented Feb 9, 2021

CLA assistant check
All CLA requirements met.

@@ -18,6 +18,7 @@
},
"license": "MIT",
"dependencies": {
"@aws-sdk/client-s3": "~3.3.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

@iclanton these @aws-sdk dependencies bring in 3,371files (16MB uncompressed). The Azure stuff also brought in a bunch of dependencies. Maybe it's time to consider an install-on-demand facility for Rush.

@octogonz
Copy link
Collaborator

@raihle are you able to sign the CLA?

@iclanton
Copy link
Member

This is awesome! Thanks for putting this together.

I agree with @octogonz - I think we need to split some of these big-but-optional Rush features into "feature packages" that are only installed in repositories that require them.

apps/rush-lib/tsconfig.json Show resolved Hide resolved
common/config/rush/browser-approved-packages.json Outdated Show resolved Hide resolved
@@ -4,815 +4,815 @@
"packages": [
Copy link
Member

Choose a reason for hiding this comment

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

Can you revert the formatting changes to this file?

common/config/rush/pnpmfile.js Show resolved Hide resolved
@raihle
Copy link
Contributor Author

raihle commented Feb 15, 2021

@raihle are you able to sign the CLA?

I'd like to spend some time on Rush as part of my day-job, so I'm trying to get permission for that, but I'll sign it for myself otherwise.

@octogonz
Copy link
Collaborator

@iclanton Can we double-check:

  • Does this not significantly increase the npm install footprint for Rush?
  • Does it not bring in any new native dependencies (like keytar)?
  • Does it not impact the process load time? (We already validated this using rundown, but before your latest commits.)

@iclanton
Copy link
Member

Does this not significantly increase the npm install footprint for Rush?

This PR adds 73 AWS dependencies. That's unfortunate but not the end of the world. We should try to cut that down in a follow-up.

Does it not bring in any new native dependencies (like keytar)?

Doesn't seem to, no.

Does it not impact the process load time? (We already validated this using rundown, but before your latest commits.)

Rundown is the same after my changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants