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

[BREAKING] Usability improvements for the CodeBuild Construct library #412

Merged
merged 1 commit into from
Jul 29, 2018
Merged

[BREAKING] Usability improvements for the CodeBuild Construct library #412

merged 1 commit into from
Jul 29, 2018

Conversation

skinny85
Copy link
Contributor

  1. Rename 'BuildProject' to 'Project'.
  2. Allow setting the physical name of a Project, to make it consistent with other L2s.
  3. Introduce a BuildImage class that makes it more convenient to specify the used Docker image.
  4. Introduce a convenience PipelineProject class for use in CodePipeline that defaults the source and artifacts fields.

Continued from #3


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

Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

Looks great!


const pipeline = new codepipeline.Pipeline(this, 'MyPipeline');
const buildStage = new codepipeline.Stage(pipeline, 'Build');
new codebuildPipeline.PipelineBuildAction(buildStage, 'CodeBuild', {
project: project,
project: project
Copy link
Contributor

Choose a reason for hiding this comment

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

No need : project

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@@ -10,12 +10,26 @@ import codebuildPipeline = require('@aws-cdk/aws-codebuild-codepipeline');
import codepipeline = require('@aws-cdk/aws-codepipeline');

// see the @aws-cdk/aws-codebuild module for more documentation on how to create CodeBuild Projects
const project = new codebuild.BuildProject( // ...
);
const project = new codebuild.Project(this, 'MyProject', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think there's value in walking through the layers here? Why would anyone work with this library if they don't want to use PipelineProject?

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps you can switch the order of the narrative? Start with PipelineProject and then explain that under the hood it is just a sugar for Project

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 main draw to this module is the PipelineBuildAction (and soon the PipelineTestAction), PipelineProject is just a small convenience :). But sure, I've switched around the narrative to start with PipelineProject.

1. Rename 'BuildProject' to 'Project'.
2. Allow setting the physical name of a Project, to make it consistent with other L2s.
3. Introduce a BuildImage class that makes it more convenient to specify the used Docker image.
4. Introduce a convenience PipelineProject class for use in CodePipeline that defaults the source and artifacts fields.
@skinny85
Copy link
Contributor Author

Looks great!

Thanks :)

@skinny85
Copy link
Contributor Author

I would appreciate a merge if this looks good :)

@eladb eladb merged commit b8ebcbe into aws:master Jul 29, 2018
@skinny85 skinny85 deleted the feature/code-build-improvements2 branch July 30, 2018 16:52
@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.

3 participants