Moved the LambdaInvoke Action from the codepipeline module to the Lambda module #348

eladb · 2018-07-16T19:12:33Z

This continues the refactoring of moving out concrete Action types to modules containing the resource the Action integrates with.

By @skinny85

skinny85 · 2018-07-16T19:34:17Z

Thanks for the new PR @eladb ! I just rebased and updated the PR.

mpiroc · 2018-07-16T19:37:57Z

packages/@aws-cdk/lambda/test/test.pipeline-action.ts

@@ -0,0 +1,90 @@
+import { expect, haveResource } from '@aws-cdk/assert';
+import { Pipeline, Stage } from '@aws-cdk/codepipeline';


Nittiest of nits: When importing the same symbols (Stage and Pipeline in this case), sometimes we do import *, other times we do import { ... }. In both cases we only use Stage and Pipeline. We should try to be consistent unless there is a good reason for the exception.

@mpiroc yes, you got me :) The reason for that is that I'm only moving existing tests, that were written before the Big L1-L2 Unification™. I changed it to use import * as ... in the integ tests (which are meant to be similar to what a customer would write), but left the old way in the unit tests (in which it's less important I think). But, if you insist, I can change it in the unit tests as well :)

@mpiroc good point - I think we are standardizing around:

import foo = require('foo');

It's concise, namespaced, and fabulous 🎂

eladb · 2018-07-17T08:21:34Z

@skinny85 the build break is caused by a dependency cycle: lambda => codecommit => sns => lambda.

I believe codecommit is only used as a test in lambda. Let's just rewrite this test not to include codecommit.

Here's the output of find-cycle:

Cycle: @aws-cdk/iam => @aws-cdk/assert => aws-cdk => @aws-cdk/s3 => @aws-cdk/kms => @aws-cdk/iam
Cycle: @aws-cdk/assert => aws-cdk => @aws-cdk/s3 => @aws-cdk/kms => @aws-cdk/assert
Cycle: @aws-cdk/iam => @aws-cdk/assert => aws-cdk => @aws-cdk/s3 => @aws-cdk/iam
Cycle: @aws-cdk/assert => aws-cdk => @aws-cdk/s3 => @aws-cdk/assert
Cycle: @aws-cdk/assert => aws-cdk => @aws-cdk/cloudformation => @aws-cdk/assert
Cycle: @aws-cdk/iam => @aws-cdk/assert => aws-cdk => @aws-cdk/util => @aws-cdk/iam
Cycle: @aws-cdk/sns => @aws-cdk/lambda => @aws-cdk/codecommit => @aws-cdk/sns
Cycle: @aws-cdk/sns => @aws-cdk/lambda => @aws-cdk/codecommit => @aws-cdk/codepipeline => @aws-cdk/sns

P.S. the other cycles are related to #32

skinny85 · 2018-07-17T16:10:23Z

Great catch @eladb , I would not guess a cycle might be the root cause of that. Alright, I'll change the integ test to not use codecommit.

… to its dedicated, new lambda-codepipeline module. This continues the refactoring of moving out concrete Action types from the codepipeline module.

skinny85 · 2018-07-23T19:57:35Z

Completely changed the approach.

The dependencies right now look like this:

codepipeline -> sns -> lambda

So, lambda cannot depend on codepipeline, as that would be a cycle.

Because of that, I've extracted a separate lambda-codepipeline module. I think breaking out all of the integrations with codepipeline into their own submodules make a lot of sense - in that way, all of the modules in @aws-cdk are smaller, and have less dependencies. Also, it lets us break out the S3 Action out of the codepipeline module (codepipeline already depends on s3 for Bucket management).

Let me know if there are any objections to this approach.

eladb · 2018-07-24T02:48:05Z

packages/@aws-cdk/codepipeline/test/test.generalValidation.ts

@@ -1,4 +1,7 @@
+import { Stack } from '@aws-cdk/core';


rename to test.general-validation.ts

eladb · 2018-07-24T02:48:56Z

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

@@ -0,0 +1,21 @@
+## AWS Lambda Construct Library for AWS CodePipeline Actions


The title should be AWS CodePipline Actions for AWS Lambda 😄

eladb · 2018-07-24T02:49:39Z

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

+Example usage:
+
+```ts
+import * as codepipeline from '@aws-cdk/codepipeline';


Use import codepipline = require('@aws-cdk/codepipeline') (this is the style we eventually decided to use across the board).

eladb · 2018-07-24T02:50:05Z

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

+```ts
+import * as codepipeline from '@aws-cdk/codepipeline';
+import * as lambda from '@aws-cdk/lambda';
+import * as lambda_codepipeline from '@aws-cdk/lambda-codepipeline';


No underscores in variable names.

eladb · 2018-07-24T02:50:52Z

packages/@aws-cdk/lambda-codepipeline/package.json

+{
+  "name": "@aws-cdk/lambda-codepipeline",
+  "version": "0.7.3-beta",
+  "description": "CDK Constructs for AWS Lambda in AWS CodePipeline",


eladb · 2018-07-24T02:53:12Z

@skinny85 I think we should probably recreate this PR. I am currently the author which means that I can't approve the changes. Would you please create a new PR and I'll close this one?

skinny85 · 2018-07-24T16:31:56Z

@skinny85 I think we should probably recreate this PR. I am currently the author which means that I can't approve the changes. Would you please create a new PR and I'll close this one?

Will do.

skinny85 · 2018-07-24T16:33:46Z

@eladb what do you think of the name lambda-codepipeline? In a meeting today @RomainMuller suggested that perhaps codepipeline-lambda would be a better name (it would make these integration modules a little easier to discover).

Thoughts on this?

eladb · 2018-07-24T16:38:10Z

I think lambda-pipeline makes more sense because eventually it's a "feature" of Lambda and not pipeline

skinny85 · 2018-07-24T22:24:01Z

Moved to #401 , feel free to close this one @eladb

mpiroc · 2018-07-31T23:20:28Z

Closing--this was moved to #401.

eladb changed the title ~~Feature/lambda action move~~ Moved the LambdaInvoke Action from the codepipeline module to the Lambda module Jul 16, 2018

mpiroc approved these changes Jul 16, 2018

View reviewed changes

[BREAKING] Moved the LambdaInvoke Action from the codepipeline module…

26b5ffd

… to its dedicated, new lambda-codepipeline module. This continues the refactoring of moving out concrete Action types from the codepipeline module.

eladb commented Jul 24, 2018

View reviewed changes

skinny85 mentioned this pull request Jul 24, 2018

[BREAKING] Move the Lambda Invoke CodePipeline Action #401

Merged

skinny85 deleted the feature/lambda-action-move branch July 30, 2018 18:24

mpiroc closed this Jul 31, 2018

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

Moved the LambdaInvoke Action from the codepipeline module to the Lambda module #348

Moved the LambdaInvoke Action from the codepipeline module to the Lambda module #348

eladb commented Jul 16, 2018

skinny85 commented Jul 16, 2018

mpiroc Jul 16, 2018

skinny85 Jul 16, 2018

eladb Jul 17, 2018

eladb commented Jul 17, 2018 •

edited

Loading

skinny85 commented Jul 17, 2018

skinny85 commented Jul 23, 2018

eladb Jul 24, 2018

eladb Jul 24, 2018

eladb Jul 24, 2018

eladb Jul 24, 2018

eladb Jul 24, 2018

eladb commented Jul 24, 2018

skinny85 commented Jul 24, 2018

skinny85 commented Jul 24, 2018

eladb commented Jul 24, 2018

skinny85 commented Jul 24, 2018

mpiroc commented Jul 31, 2018

		@@ -0,0 +1,90 @@
		import { expect, haveResource } from '@aws-cdk/assert';
		import { Pipeline, Stage } from '@aws-cdk/codepipeline';

		@@ -0,0 +1,21 @@
		## AWS Lambda Construct Library for AWS CodePipeline Actions

Moved the LambdaInvoke Action from the codepipeline module to the Lambda module #348

Moved the LambdaInvoke Action from the codepipeline module to the Lambda module #348

Conversation

eladb commented Jul 16, 2018

skinny85 commented Jul 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladb commented Jul 17, 2018 • edited Loading

skinny85 commented Jul 17, 2018

skinny85 commented Jul 23, 2018

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

eladb commented Jul 24, 2018

skinny85 commented Jul 24, 2018

skinny85 commented Jul 24, 2018

eladb commented Jul 24, 2018

skinny85 commented Jul 24, 2018

mpiroc commented Jul 31, 2018

eladb commented Jul 17, 2018 •

edited

Loading