Assets #371
Assets #371
Conversation
Assets represent local files or directories which can be bundled as part of CDK constructs. During deployment, the toolkit will upload assets to the "Toolkit Bucket", and use CloudFormation Parameters to reference the asset in deploy-time. Assets expose the following deploy-time attributes: * s3BucketName * s3ObjectKey * s3Url Furthermore, the `asset.grantRead(principal)` will add IAM read permissions for the asset to the desired principal.
| @@ -0,0 +1,187 @@ | |||
| import cdk = require('@aws-cdk/core'); | |||
There was a problem hiding this comment.
Why are we not using import * as cdk from '@aws-cdk/core'; and similar for our own libraries, but using it for node libraries? I find this confusing.
There was a problem hiding this comment.
I think we should standardize around import x = require('y') for star imports across the board. It's a clean and concise syntax.
| * Path refers to a directory on disk, the contents of the directory is | ||
| * archived into a .zip. | ||
| */ | ||
| ZipDirectory = 'zip', |
There was a problem hiding this comment.
Do we need the string value here?
There was a problem hiding this comment.
Also, there may be an interest for uploading a given directory (un-zipp'd). It can be painful to have to map every file individually when there are a lot...
Would expect the URL in this case to be that of where the files were uploaded - with the same structure (so I can just append path elements to get a particular file).
There was a problem hiding this comment.
We can always add additional packaging schemes, such as Directory for example, in which case the URL will point to the prefix. We will add as needed.
| throw new Error(`Cannot find asset at ${props.path}`); | ||
| } | ||
|
|
||
| if (props.packaging === AssetPackaging.ZipDirectory) { |
| "Fn::Join": [ | ||
| "", | ||
| [ | ||
| "arn", |
There was a problem hiding this comment.
Why? Why would you runtime-join up to 7 string literals, including empty strings, instead of pre-joining them at code-authoring time?
| export const ASSET_METADATA = 'aws:cdk:asset'; | ||
| export interface AssetMetadataEntry { | ||
| path: string; | ||
| packaging: 'zip' | 'file'; |
There was a problem hiding this comment.
You'll have to change the cxapi every time we add a new supported type. It feels unnecessary granted this could be String. Otherwise, why is it not an enum?
There was a problem hiding this comment.
But it makes sense that we'll have to invalidate cxapi if we add more supported types because the change needs to be supported by the toolkit, no?
packages/aws-cdk/lib/assets.ts
Outdated
|
|
||
| const s3url = `s3://${toolkitInfo.bucketName}/${key}`; | ||
| if (changed) { | ||
| success(` 👜 Asset ${asset.path} (directory zip) uploaded to ${s3url}`); |
packages/aws-cdk/lib/assets.ts
Outdated
| if (changed) { | ||
| success(` 👜 Asset ${asset.path} (directory zip) uploaded to ${s3url}`); | ||
| } else { | ||
| success(` 👜 Asset ${asset.path} (directory zip) already exists in ${s3url}`); |
There was a problem hiding this comment.
already exists
I may perceive this as an error/warning that the file wasn't uploaded because there's already a file there in S3; when what you mean to say is what's in S3 is equal to what you meant to upload.
There was a problem hiding this comment.
Right. Any recommended wording?
There was a problem hiding this comment.
s/already exists/is already up-to-date/
|
|
||
| const s3url = `s3://${toolkitInfo.bucketName}/${key}`; | ||
| if (changed) { | ||
| success(` 👜 Asset ${asset.path} uploaded to ${s3url}`); |
There was a problem hiding this comment.
I'm not sure the "distinctive" messaging here is worth that amount of duplication.
There was a problem hiding this comment.
I think it might be useful for people to know that an asset was changed, no?
There was a problem hiding this comment.
I'm referring to the two nearly identical code paths that differ mostly on what they pass to success. The code for zip versus file versions aren't that different... Feels needlessly duplicative.
| @@ -15,7 +15,9 @@ | |||
| "test": "cdk-test" | |||
| }, | |||
| "cdk-build": { | |||
| "pre": ["./generate.sh"] | |||
There was a problem hiding this comment.
Merge mishap. Will fix.
There was a problem hiding this comment.
Oh, it's just reformatting and sorting the JSON...
| "mockery": "^2.1.0", | ||
| "pkglint": "^0.7.3-beta", | ||
| "cdk-build-tools": "^0.7.3-beta" |
There was a problem hiding this comment.
Those should also not be deleted...
The `bucketUrl` returns the URL of the bucket and `urlForObject(key)` returns the URL of an object within the bucket. Furthermore: `iam.IIdentityResource` was soft-renamed to `iam.IPrincipal` (IIdentityResource is still supported).
Assets represent local files or directories which can be bundled as part
of CDK constructs. During deployment, the toolkit will upload assets
to the "Toolkit Bucket", and use CloudFormation Parameters to reference
the asset in deploy-time.
Assets expose the following deploy-time attributes:
Furthermore, the
asset.grantRead(principal)will add IAM read permissionsfor the asset to the desired principal.
See @aws-cdk/assets/README for more details and examples
Note that the actual mechanism to emit and deploy assets will likely change as we progress towards the app model (#233). This is also related to #85.