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

feat: stage assets under .cdk.assets #2182

Merged
merged 9 commits into from
Apr 8, 2019
Merged

feat: stage assets under .cdk.assets #2182

merged 9 commits into from
Apr 8, 2019

Conversation

eladb
Copy link
Contributor

@eladb eladb commented Apr 4, 2019

To ensure that assets are available for the toolchain to deploy after the CDK app
exists, the CLI will, by default, request that the app will stage the assets under
the .cdk.assets directory (relative to working directory).

The CDK will then copy all assets from their source locations to this staging
directory and will refer to the staging location as the asset path. Assets will
be stored using their content fingerprint (md5 hash) so they will never be copied
twice unless they change.

Docker build context directories will also be staged.

Staging is disabled by default and in cdk-integ.

Added .cdk.staging to all .gitignore files in cdk init templates.

Fixes #1716
Fixes #2096


Pull Request Checklist

  • Testing
    • Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
    • CLI change?: coordinate update of integration tests with team
    • cdk-init template change?: coordinated update of integration tests with team
  • Docs
    • jsdocs: All public APIs documented
    • README: README and/or documentation topic updated
  • Title and Description
    • Change type: title prefixed with fix, feat will appear in changelog
    • Title: use lower-case and doesn't end with a period
    • Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
    • Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
  • Sensitive Modules (requires 2 PR approvers)
    • IAM Policy Document (in @aws-cdk/aws-iam)
    • EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
    • Grant APIs (only if not based on official documentation with a reference)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

@eladb eladb requested a review from a team as a code owner April 4, 2019 20:05
To ensure that assets are available for the toolchain to deploy after the CDK app
exists, the CLI will, by default, request that the app will stage the assets under
the `.cdk.assets` directory (relative to working directory).

The CDK will then *copy* all assets from their source locations to this staging
directory and will refer to the staging location as the asset path. Assets will
be stored using their content fingerprint (md5 hash) so they will never be copied
twice unless they change.

Docker build context directories will also be staged.

Staging is disabled by default and in cdk-integ.

Added .cdk.staging to all .gitignore files in cdk init templates.

Fixes #1716
@rix0rrr
Copy link
Contributor

rix0rrr commented Apr 8, 2019

Does that mean the tempdir will go in the user's app directory?

Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

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

I don't care for the default location of the staging directory. I think it's going to invite more questions and discussions. But it'll be easy enough to change, so okay.

By the way, this PR could be trivially changed to also fix #2096, by having assets.Staging output the relative file path and the assets handers in the toolkit resolving directories relative to the staging root.

@eladb
Copy link
Contributor Author

eladb commented Apr 8, 2019

I don't care for the default location of the staging directory. I think it's going to invite more questions and discussions. But it'll be easy enough to change, so okay.

Can you elaborate what you don’t care about?

By the way, this PR could be trivially changed to also fix #2096, by having assets.Staging output the relative file path and the assets handers in the toolkit resolving directories relative to the staging root.

It actually fixes this already. Added.

@rix0rrr
Copy link
Contributor

rix0rrr commented Apr 8, 2019

Can you elaborate what you don’t care about?

First, everyone who has a CDK project today will not have this directory in their .gitignore yet, and this change will start generating more files in their source directory, which a careless git add -A is going to commit to version control.

Second, the directory is not going to be as inconspicuous as you think on Windows.

I'd prefer for this directory to be off to the side somewhere, maybe in $TMP

@eladb
Copy link
Contributor Author

eladb commented Apr 8, 2019

I'd prefer for this directory to be off to the side somewhere, maybe in $TMP

How about $HOME? This way, assets are reused across CDK apps.

@rix0rrr
Copy link
Contributor

rix0rrr commented Apr 8, 2019

This way, assets are reused across CDK apps.

If we use a stable name under $TMP we can also reuse across CDK apps, and I think putting there covers the intent better. Or it would have to be $HOME/.cache, which I think is also an (emerging?) standard, but doesn't place so nicely on Windows. Windows would have to be %LOCALAPPDATA% if we wanted to be correct.

I think $TMP/cdk-staging covers all use cases well enough.

Big downside is that if we go for an out-of-scope directory, we're on the hook for periodically cleaning it out as well. Which in fact, we should be doing for an in-scope directory as well anyway.

@eladb
Copy link
Contributor Author

eladb commented Apr 8, 2019

We use stable names (based off of the content fingerprint). It is a short of cache.

I am leaning towards leaving this under the project directory though as it aligns better with our longer term Toolchain project where “synth” and “bundle” happen on different machines so we need to move all the asset sources between those machines.

@rix0rrr
Copy link
Contributor

rix0rrr commented Apr 8, 2019

In that case, are the default settings maybe optimizing for CI builds, thereby accidentally pessimizing local builds?

If so, we could switch the default off of the --ci flag.

Copy link
Contributor

@sam-goodwin sam-goodwin left a comment

Choose a reason for hiding this comment

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

LGTM. The hashing and copying logic will be super useful for the toolkit changes.

@eladb
Copy link
Contributor Author

eladb commented Apr 8, 2019

@rix0rrr decided for now to leave it under the project directory. @sam-goodwin is going to refactor this quite a bit in his work on the toolchain and I rather keep this simple for now.

Elad Ben-Israel and others added 3 commits April 9, 2019 00:39
…2027)

This Code type is helpful when there is no local file/directory to use Assets with,
and the Lambda is supposed to be deployed in a CodePipeline.
Until aws/jsii#442 is released
we must fail builds on compilation errors. This is a temporary
workaround for this issue (if there is something in stdout
we consider that a compilation error).
@eladb eladb merged commit 2f74eb4 into master Apr 8, 2019
@eladb eladb deleted the benisrae/stage-assets branch April 8, 2019 23:28
@NGL321 NGL321 added the contribution/core This is a PR that came from AWS. label Sep 23, 2019
@mergify
Copy link
Contributor

mergify bot commented Sep 23, 2019

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist
:

  • PR title type(scope): text
    • type: fix, feat, refactor go into CHANGELOG, chore is hidden
    • scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
    • text: use all lower-case, do not end with a period, do not include issue refs
  • PR Description
    • Rationale: describe rationale of change and approach taken
    • Issues: indicate issues fixed via: fixes #xxx or closes #xxx
    • Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
  • Testing
    • Unit test added. Prefer to add a new test rather than modify existing tests
    • CLI or init templates change? Re-run/add CLI integration tests
  • Documentation
    • README: update module README to describe new features
    • API docs: public APIs must be documented. Copy from official AWS docs when possible
    • Design: for significant features, follow design process

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
5 participants