Move the CodeCommit and CodeBuild Actions from the codepipeline module into their @aws-cdk modules #238

skinny85 · 2018-07-06T23:59:23Z

As agreed to with the CodePipeline team,
we want to move the concrete Action types from the codepipeline module
and into the modules of the resources they represent integrations with.
This is doing this refactoring for CodeCommit Repositories and CodeBuild Projects.

The most interesting part of this PR is where to put the tests that were in the codepipeline module. They can't be there anymore, as they relied on creating CodeCommit and CodeBuild Actions, and so that would result in a circular dependency. I moved them to the codebuild project, as that one already depended on codecommit, and now, of course, depends on codepipeline. But, I'm open to suggestions on a different approach (separate module for the tests maybe?).

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

eladb

Looks good. See some feedback about adding addToPipeline methods to ease discoverability and improve the ergonomics of using these resources within a pipeline. I am okay if you prefer to open an issue and implement this as a 2nd phase.

Regarding testing: I think the codepipeline library should test the "framework", and each integration (i.e. code-commit, code-build, etc) should have tests that verify the integration. In that regard, I would add another integration test to code-commit that verifies just the code-commit source behavior.

eladb · 2018-07-08T18:01:08Z

packages/@aws-cdk/codebuild/lib/codebuild_pipeline_action.ts

@@ -0,0 +1,59 @@
+import { Artifact, BuildAction, Stage } from '@aws-cdk/codepipeline';


File names should use hyphens (codebuild-pipeline-action.ts).

eladb · 2018-07-08T18:03:14Z

packages/@aws-cdk/codebuild/lib/codebuild_pipeline_action.ts

+        // allow codebuild to read and write artifacts to the pipline's artifact bucket.
+        parent.pipeline.artifactBucket.grantReadWrite(props.project.role);
+
+        // policy must be added as a dependency to the pipeline!!


If this is still relevant, can you please open an issue with some more context?

I'm actually not sure. This comment has been here before (I've only moved it). The Pipeline already depends on both the Pipeline Role and on the Policy Document that Role uses. Perhaps this comment is no longer relevant?

eladb · 2018-07-08T18:05:10Z

packages/@aws-cdk/codebuild/lib/codebuild_pipeline_action.ts

+/**
+ * Build action that uses AWS CodeBuild
+ */
+export class CodeBuildAction extends BuildAction {


Since this is already in the codebuild library, wouldn't it make sense to make this discoverable directly from the project? I am envisioning something like buildProject.addToPipeline(stage, name, props). This could be a nice pattern to allow easy discovery of AWS resources that can be used as pipeline actions.

P.S. When we implement #181 we could use this pattern to identify pipeline actions and provide rich IDE and tools support for pipeline definitions.

Sounds good, but, like you said, I'd rather do this change separately (this one is only about moving files around). I've filed #265 to not lose track of this.

eladb · 2018-07-08T18:09:28Z

packages/@aws-cdk/codecommit/lib/index.ts

@@ -1 +1,2 @@
 export * from './codecommit';
+export * from './codecommit_pipeline_action';


hyphens in file name

eladb · 2018-07-08T18:09:54Z

packages/@aws-cdk/codecommit/lib/codecommit_pipeline_action.ts

+/**
+ * Source that is provided by an AWS CodeCommit repository
+ */
+export class CodeCommitSource extends Source {


add repository.addToPipeline(stage, name, props)

See above (#265 ).

skinny85 · 2018-07-09T22:17:24Z

Thank for the quick review @eladb ! I've submitted a 2nd version with the file names corrected, and an integ test in the codecommit module for its interactions with CodePipeline.

eladb · 2018-07-10T07:27:36Z

packages/@aws-cdk/codebuild/lib/codebuild-pipeline-action.ts

+/**
+ * Build action that uses AWS CodeBuild
+ */
+export class CodeBuildAction extends BuildAction {


I think this should be renamed to PipelineAction or CodeBuildPipelineAction (the word "pipeline" must be there somewhere 😄 )

There's actually quite a (stupid?) subtlety here. There are actually 2 CodeBuild Actions in the codepipeline module (the other is simply commented out right now). The difference between them is the category they use. The first one (this one) has category ActionCategory.Build, while the other one has ActionCategory.Test (it's used for approval steps). They are also constructed a little bit differently (the one with ActionCategory.Test doesn't produce any output artifacts, for example).

Given that, does it make sense to name this CodeBuildPipelineAction, and then the other class CodeBuildTestPipelineAction?

How about BuildPipelineAction and TestPipelineAction. We are in the CodeBuild module, so everything is namespaced by codebuild anyway.

P.S. how will this work with addToPipeline() will we need two addToPipeline or will it take a property indicating if this is a build or test action?

How about BuildPipelineAction and TestPipelineAction. We are in the CodeBuild module, so everything is namespaced by codebuild anyway.

That would be a departure from how we named those previously. In theory, a customer can import * from @aws-cdk/codebuild, right? In which case there would no context, as the L2s are in the default namespace.

However, if we're changing this convention, I'm fine with that.

P.S. how will this work with addToPipeline() will we need two addToPipeline or will it take a property indicating if this is a build or test action?

Not sure. My first instinct is having 2 addToPipeline()s, but I guess we can discuss it in more depth when we add those methods.

I went with PipelineBuildAction in the new version - it meshes better with codecommit's PipelineSource (SourcePipeline sounds wrong).

Can you make sure to add a note to #265 that emphasizes the fact that a CodeCommit project can serve two different roles in a pipeline?

Can you make sure to add a note to #265 that emphasizes the fact that a CodeCommit project can serve two different roles in a pipeline?

Done.

eladb · 2018-07-12T08:40:32Z

packages/@aws-cdk/codebuild/test/integ.pipeline-code-commit-build.ts

@@ -1,17 +1,18 @@
-import { BuildProject, CodePipelineSource } from '@aws-cdk/codebuild';
-import { Repository } from '@aws-cdk/codecommit';
+import { PipelineSource, Repository } from '@aws-cdk/codecommit';


We decided to move to "*" imports, which will make this code more readable, so import * as codecommit from '@aws-cdk/codecommit' and then codecommit.PipelineSource will make much more sense.

Changed in all of the integ tests.

eladb · 2018-07-12T08:40:53Z

packages/@aws-cdk/codecommit/lib/pipeline-action.ts

+}
+
+/**
+ * Source that is provided by an AWS CodeCommit repository


"CodePipeline Source that is provi..."

…eline module into their @aws-cdk modules. As agreed to with the CodePipeline team, we want to move the concrete Action types from the codepipeline module and into the modules of the resources they represent integrations with. This is doing this refactoring for CodeCommit Repositories and CodeBuild Projects.

skinny85 · 2018-07-16T18:51:38Z

Ping @eladb - if this looks fine, do you mind merging it in? I don't have permissions.

eladb approved these changes Jul 8, 2018

View reviewed changes

eladb mentioned this pull request Jul 8, 2018

AWS Construct Library: Construct Specs and API Linting #181

Closed

skinny85 changed the title ~~Move the CodeCommit and CodeBuild Actions from the codepipeline modules into their @aw-cdk modules~~ Move the CodeCommit and CodeBuild Actions from the codepipeline module into their @aws-cdk modules Jul 9, 2018

skinny85 mentioned this pull request Jul 9, 2018

Add an addToPipeline method for L2 Constructs like CodeCommit Repositories and CodeBuild Projects #265

Closed

eladb approved these changes Jul 10, 2018

View reviewed changes

eladb approved these changes Jul 12, 2018

View reviewed changes

eladb merged commit 248c503 into aws:master Jul 16, 2018

skinny85 deleted the feature/code-actions-move-backup branch July 16, 2018 19:23

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

Move the CodeCommit and CodeBuild Actions from the codepipeline module into their @aws-cdk modules #238

Move the CodeCommit and CodeBuild Actions from the codepipeline module into their @aws-cdk modules #238

skinny85 commented Jul 6, 2018

eladb left a comment

eladb Jul 8, 2018

skinny85 Jul 9, 2018

eladb Jul 8, 2018

skinny85 Jul 9, 2018

eladb Jul 8, 2018

skinny85 Jul 9, 2018

eladb Jul 8, 2018

skinny85 Jul 9, 2018

eladb Jul 8, 2018

skinny85 Jul 9, 2018

skinny85 commented Jul 9, 2018

eladb Jul 10, 2018

skinny85 Jul 10, 2018

eladb Jul 11, 2018

skinny85 Jul 11, 2018

skinny85 Jul 11, 2018 •

edited

Loading

eladb Jul 12, 2018

skinny85 Jul 12, 2018

eladb Jul 12, 2018

skinny85 Jul 12, 2018

eladb Jul 12, 2018

skinny85 Jul 12, 2018

skinny85 commented Jul 16, 2018

		@@ -0,0 +1,59 @@
		import { Artifact, BuildAction, Stage } from '@aws-cdk/codepipeline';

		@@ -1 +1,2 @@
		export * from './codecommit';
		export * from './codecommit_pipeline_action';

Move the CodeCommit and CodeBuild Actions from the codepipeline module into their @aws-cdk modules #238

Move the CodeCommit and CodeBuild Actions from the codepipeline module into their @aws-cdk modules #238

Conversation

skinny85 commented Jul 6, 2018

eladb left a 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

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 Jul 9, 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

skinny85 Jul 11, 2018 • edited Loading

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 Jul 16, 2018

skinny85 Jul 11, 2018 •

edited

Loading