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(toolkit): Produce an Asset manifest document #951

Closed
wants to merge 1 commit into from

Conversation

RomainMuller
Copy link
Contributor

@RomainMuller RomainMuller commented Oct 17, 2018

Consumes the metadata produced by Assets into the SynthesizedStacks,
and builds an asset manifest document and "internalizes" the supporting files.
The supporting files are de-duplicated using a sha-256, so that the same
file(s) referenced from multiple assets or stacks will be presented only once in
the output.

The asset document maps an asset logical ID to the "internalized" file(s) base
path in the output directory, the construct path where the metadata was created,
and the output parameters specified by the asset (currently always an S3
bucket and key).

TODO

  • Refactor into cdk.App
  • Integration test

Consumes the metadata produced by `Asset`s into the `SynthesizedStack`s,
and builds an asset manifest document and "internalizes" the supporting files.
The supporting files are de-duplicated using a `sha-256`, so that the same
file(s) referenced from multiple assets or stacks will be presented only once in
the output.

The asset document maps an asset logical ID to the "internalized" file(s) base
path in the output directory, the construct path where the metadata was created,
and the output parameters specified by the asset (currently always an S3
bucket and key).
@RomainMuller RomainMuller requested review from rix0rrr and eladb October 17, 2018 09:22
@eladb
Copy link
Contributor

eladb commented Oct 17, 2018

I understand this is a step towards CI/CD support, but would be nice to have a picture of where this is going. See my comment inline, but I wonder if we should move most of this logic (including, the asset copying) to cdk.App, so that the output of the App (now, files) would practically be "The" output.

Also, please add an integration test for this. Let's stop adding code to the toolkit without tests.

* @param dir the directory under which to stage the assets (an assets sub-directory will be used).
* @param cache a cache for asset files hashes, improving performance when the same assets are used across multiple stacks
*/
async function prepareManifest(stack: cxapi.SynthesizedStack, dir: string, cache: { [path: string]: string }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting... My intuition was that all the assets and asset manifest should be emitted to the working directory by the App and not by the toolkit. Now that the cx protocol enables this, I wonder if it doesn't make more sense to move all this logic to App so that we'll have less magic in the toolkit.

What do you think?

I am okay if you feel this can be done in two steps, but it would be nice to have some agreement on where we want this to go, so we'll not take too many redundant steps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That actually sounds like it'd be better. I had started this before you made the changes to the App API, and I missed the fact I could refactor this away to the other side of the wall.

const basePath = path.join('assets', await hash(asset.path), path.basename(asset.path));
const fullPath = path.join(dir, basePath);
if (!await fs.pathExists(fullPath)) {
await fs.copy(asset.path, fullPath);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use symlinks to make things faster, or would this break CI/CD?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The CodeBuild documentation doesn't really state anything with respects to symlinks behavior in output artifacts, so I'd rather avoid assuming they are replaced by the referent file... But on the other hand this should be relatively easy to test out and verify. That'd also save disk space on a user machine, which is not a bad thing.

@RomainMuller
Copy link
Contributor Author

Also, please add an integration test for this. Let's stop adding code to the toolkit without tests.

That was planned indeed. I wanted this out sooner rather than later so that other people can start growing how I have this working out if they need to prepare a downstream integration...

@RomainMuller
Copy link
Contributor Author

Issue #956 attempts to define a standard.

@rix0rrr
Copy link
Contributor

rix0rrr commented Nov 13, 2018

Do we still need this PR?

@RomainMuller
Copy link
Contributor Author

Let's drop this.

@RomainMuller RomainMuller deleted the rmuller/asset-manifest branch November 13, 2018 16:42
@NGL321 NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019
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
Development

Successfully merging this pull request may close these issues.

4 participants