feat(assets): surface the CFN Parameters that Assets create #2057

skinny85 · 2019-03-20T01:53:42Z

This is needed in order to override them when deploying the Stack through CodePipeline.

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.

skinny85 · 2019-03-20T01:56:35Z

There is a pretty serious issue with this solution. Because Assets check whether the file/directory they point to exists, this forces the build in CodePipeline to happen sequentially - first the packaging of the Lambda source into a deployable artifact, and only then cdk synth NameOfLambdaStack. This is wasteful, as other than checking for that file existing, the CDK process does not use that artifact in any way, so the 2 builds should happen in parallel in the Pipeline.

rix0rrr · 2019-03-20T10:15:28Z

Because Assets check whether the file/directory they point to exists

Fair point. The Correct Solution (tm) is probably to add a checkForExistence: true parameter to the assets. If left undefined, we use the default, and the default can be based on whether or not we're running in CI.

The toolkit already has a --ci flag, with logic to autodetermine whether it's most likely running on CI. We can pass that value to the CDK app either via context or an ENV_VAR.

packages/@aws-cdk/aws-lambda/README.md

packages/@aws-cdk/assets/lib/asset.ts

rix0rrr · 2019-03-20T10:26:08Z

packages/@aws-cdk/aws-lambda/README.md

+const parameterOverrides: { [name: string]: any } = {};
+parameterOverrides[lambdaCode.asset.bucketNameParam.logicalId] = lambdaBuildAction.outputArtifact.bucketName;
+parameterOverrides[lambdaCode.asset.objectKeyParam.logicalId] = lambdaBuildAction.outputArtifact.objectKey;
+pipeline.addStage({


Can you build the next layer on top of this as well?

So that we basically do:

const stack = new MyStack(app, 'MyStack'); stack.addToPipeline(pipeline); // (*)

And make that work? What will we need to map assets used in the application to build artifacts? Can we even find a list of all assets, so that the addToPipeline() call can determine that we satisfied all requirements? Etc.

// (*) I know this exact method call will not work because the package dependency arrows will point the wrong way, but I'm mirroring the build.addToPipeline() etc. methods here.

We are actually planning to remove all the addTpPipeline methods since we are moving all the pipeline integration classes to a centralized package. But maybe something like an integration class would work.

But yeah, this is waaayy to verbose

Can you build the next layer on top of this as well?

So that we basically do:

const stack = new MyStack(app, 'MyStack'); stack.addToPipeline(pipeline); // (*)

And make that work? What will we need to map assets used in the application to build artifacts? Can we even find a list of all assets, so that the addToPipeline() call can determine that we satisfied all requirements? Etc.

// (*) I know this exact method call will not work because the package dependency arrows will point the wrong way, but I'm mirroring the build.addToPipeline() etc. methods here.

We can't be so high level, unfortunately. You wouldn't know which Pipeline Artifacts to use to override each Asset - the customer needs to specify this.

I know, but what I'm saying is trying to get as close to that as possible.

packages/@aws-cdk/assets/lib/asset.ts

packages/@aws-cdk/aws-lambda/README.md

eladb · 2019-03-20T12:32:49Z

packages/@aws-cdk/aws-lambda/README.md

+const parameterOverrides: { [name: string]: any } = {};
+parameterOverrides[lambdaCode.asset.bucketNameParam.logicalId] = lambdaBuildAction.outputArtifact.bucketName;
+parameterOverrides[lambdaCode.asset.objectKeyParam.logicalId] = lambdaBuildAction.outputArtifact.objectKey;
+pipeline.addStage({


We are actually planning to remove all the addTpPipeline methods since we are moving all the pipeline integration classes to a centralized package. But maybe something like an integration class would work.

eladb · 2019-03-20T12:36:35Z

packages/@aws-cdk/aws-lambda/README.md

+const parameterOverrides: { [name: string]: any } = {};
+parameterOverrides[lambdaCode.asset.bucketNameParam.logicalId] = lambdaBuildAction.outputArtifact.bucketName;
+parameterOverrides[lambdaCode.asset.objectKeyParam.logicalId] = lambdaBuildAction.outputArtifact.objectKey;
+pipeline.addStage({


But yeah, this is waaayy to verbose

skinny85 · 2019-03-21T07:18:39Z

Updated according to comments. Let me know if this is better @eladb and @rix0rrr !

skinny85 · 2019-03-21T09:50:05Z

Added mention of autoDeploy: false in the ReadMe of Lambda.

This is needed in order to override them when deploying the Stack through CodePipeline.

skinny85 · 2019-03-29T23:20:06Z

Rebased on top of all of the changes (in particular, #2098 , which allows us to depend on the aws-cdk/assets package from the module that contains the Artifact class used in CodePipeline).

@rix0rrr, @eladb , I would appreciate another run through the PR!

eladb · 2019-03-31T07:47:45Z

packages/@aws-cdk/aws-codepipeline-actions/README.md

@@ -324,9 +324,69 @@ This package defines the following actions:
  changes from the people (or system) applying the changes.
 * **CloudFormationExecuteChangeSetAction** - Execute a change set prepared previously.

+##### Lambda deployed through CodePipeline


This section makes me feel that maybe we should merge the README files of aws-codepipeline and aws-codepipeline-actions. It feels artificial to have to jump between the two... What do you think?

I don't have any philosophical objections to that, I do have one practical concern though: nesting. As you can see, we've reached H5 in the ReadMe for codepipeline-actions, and I assume we'll need to increase the nesting of the headings by at least one level if we move them to the codepipeline ReadMe.

eladb · 2019-03-31T07:48:24Z

packages/@aws-cdk/aws-codepipeline-actions/README.md

+
+```typescript
+const lambdaStack = new cdk.Stack(app, 'LambdaStack', {
+  autoDeploy: false, // to make working with the 2 Stacks easier in the toolkit


Change the comment to describe what this does

eladb · 2019-03-31T07:48:47Z

packages/@aws-cdk/aws-codepipeline-actions/README.md

+    cdkBuildAction,
+  ],
+});
+// finally, deploy your Lambda Stack


Style: new line before each "//" line

eladb · 2019-03-31T07:50:22Z

packages/@aws-cdk/aws-codepipeline-actions/README.md

+  runtime: lambda.Runtime.NodeJS810,
+});
+// used to make sure each CDK synthesis produces a different Version
+const version = func.newVersion();


This sounds weird and scary?... We can't have "cdk synth" produce a different result each time. Is this just temporary until we have #1400?

It's meant to be used in a Pipeline, so I believe that's fine.

eladb · 2019-03-31T07:53:06Z

packages/@aws-cdk/aws-codepipeline/lib/artifact.ts

@@ -51,6 +52,13 @@ export class Artifact {
  public toString() {
    return this.artifactName;
  }
+
+  public overrideAsset(asset: assets.Asset): { [name: string]: string } {


Missing documentation. I am not sure about this name... Reads weird: artifact.overrideAsset(asset). What does this mean? Maybe something like artifact.wireAsset(asset) or artifact.useAsset(asset)?

The word used is override because you're changing what the asset points to. Before, it was referencing something on the file system - now, it references something in the Pipeline. So I think it makes a lot of sense.

But if you prefer useAsset (wireAssetI don't think is great...), I won't fight you (too strongly ;p) on it.

skinny85 · 2019-04-01T23:34:49Z

Talked with Elad offline about this PR. We decided not to go through with it.

skinny85 requested a review from a team as a code owner March 20, 2019 01:53

skinny85 requested a review from eladb March 20, 2019 01:53

rix0rrr reviewed Mar 20, 2019

View reviewed changes

packages/@aws-cdk/aws-lambda/README.md Outdated Show resolved Hide resolved

rix0rrr reviewed Mar 20, 2019

View reviewed changes

packages/@aws-cdk/assets/lib/asset.ts Outdated Show resolved Hide resolved

rix0rrr reviewed Mar 20, 2019

View reviewed changes

rix0rrr assigned skinny85 Mar 20, 2019

eladb reviewed Mar 20, 2019

View reviewed changes

skinny85 force-pushed the feature/assets-cfn-params branch from 568c565 to 4db81c5 Compare March 20, 2019 23:44

skinny85 force-pushed the feature/assets-cfn-params branch from 4db81c5 to 7caf911 Compare March 21, 2019 09:49

skinny85 mentioned this pull request Mar 26, 2019

feat(codepipeline): move all of the Pipeline Actions to their dedicated package #2098

Merged

4 tasks

rix0rrr approved these changes Mar 28, 2019

View reviewed changes

feat(assets): surface the CFN Parameters that Assets create.

6bb1900

This is needed in order to override them when deploying the Stack through CodePipeline.

skinny85 force-pushed the feature/assets-cfn-params branch from 7caf911 to 6bb1900 Compare March 29, 2019 23:18

eladb reviewed Mar 31, 2019

View reviewed changes

skinny85 closed this Apr 1, 2019

NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(assets): surface the CFN Parameters that Assets create #2057

feat(assets): surface the CFN Parameters that Assets create #2057

skinny85 commented Mar 20, 2019

skinny85 commented Mar 20, 2019

rix0rrr commented Mar 20, 2019

rix0rrr Mar 20, 2019

eladb Mar 20, 2019

eladb Mar 20, 2019

skinny85 Mar 20, 2019

rix0rrr Mar 21, 2019

eladb Mar 20, 2019

eladb Mar 20, 2019

skinny85 commented Mar 21, 2019

skinny85 commented Mar 21, 2019

skinny85 commented Mar 29, 2019

eladb Mar 31, 2019

skinny85 Apr 1, 2019

eladb Mar 31, 2019

eladb Mar 31, 2019

eladb Mar 31, 2019

skinny85 Apr 1, 2019

eladb Mar 31, 2019

skinny85 Apr 1, 2019

skinny85 commented Apr 1, 2019

feat(assets): surface the CFN Parameters that Assets create #2057

feat(assets): surface the CFN Parameters that Assets create #2057

Conversation

skinny85 commented Mar 20, 2019

Pull Request Checklist

skinny85 commented Mar 20, 2019

rix0rrr commented Mar 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 commented Mar 21, 2019

skinny85 commented Mar 21, 2019

skinny85 commented Mar 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 commented Apr 1, 2019