fix(codebuild): API cleanup #2745

skinny85 · 2019-06-04T21:43:08Z

Change the names of the source classes to be more consistent with other integrations. Make the abstract Source classes package-private. Add static factory methods to Source.

Change the names of artifacts classes to be more consistent with other integrations. Add static factory methods to Artifacts.

Make CodePipelineSource, CodePipelineArtifacts, NoSource, and NoArtifacts package-private.

Get rid of the SourceType enum.

BREAKING CHANGE:

codebuild: rename BuildSource to Source, S3BucketSource to S3Source, BuildArtifacts to Artifacts, S3BucketBuildArtifacts to S3Artifacts
codebuild: the classes CodePipelineBuildSource, CodePipelineBuildArtifacts, NoBuildSource, and NoBuildArtifacts have been removed
codebuild: rename buildScriptAsset and buildScriptAssetEntrypoint to buildScript and buildScriptEntrypoint, respectively

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
- Design: For significant features, design document added to design folder
Title and Description
- Change type: title prefixed with fix, feat and module name in parens, which 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.

rix0rrr

Does it make sense to put artifacts and sources in separate packages? Wouldn't a single package for both be just as effective (and less overhead)?

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

rix0rrr · 2019-06-05T08:00:01Z

Also, I would be fine with names a la S3Source, S3Artifacts.

In effect, service + integration type. This is what we've been settling on for most of the integration classes (except the SFN tasks one, where it's an action verb, WriteToBucket or similar).

eladb

+1 on merging artifacts into -sources (or leaving in @aws-cdk/codebuild).

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

skinny85 · 2019-06-11T02:20:02Z

+1 on merging artifacts into -sources (or leaving in @aws-cdk/codebuild).

Just so I understand - for the second option, are you saying to leave just Artifacts in @aws-cdk/aws-codebuild, but still extract Sources into @aws-cdk/aws-codebuild-sources, or are you saying that you'd rather leave both Sources and Artifacts in @aws-cdk/aws-codebuild?

eladb · 2019-06-11T10:30:57Z

+1 for leaving sources in codebuild

skinny85 · 2019-06-11T17:16:35Z

Any justification why do we want to make CodeBuild inconsistent with our other integration packages?

eladb · 2019-06-11T17:26:35Z

Are you asking about the sources? It seems to be codebuild sources are not really “integrations”, but if you feel strongly about it, I don’t have a strong opinion.

skinny85 · 2019-06-11T17:49:39Z

Whatever then.

Let's change this PR to finalize the names, in that case (and do small cleanups, like removing the SourceType enum, making the intermediate Source classes package-private, etc.).

Currently, the names are:

Sources:
- BuildSource
- NoSource
- S3BucketSource
- CodePipelineSource
- CodeCommitSource
- GitHubSource
- GitHubEnterpriseSource
- BitBucketSource
Artifacts:
- BuildArtifacts
- NoBuildArtifacts
- S3BucketBuildArtifacts
- CodePipelineBuildArtifacts

I'm thinking of removing the Build prefix, so leaving it just as Source / Artifacts. So:

Sources:
- Source
- S3Source
- CodePipelineSource
- CodeCommitSource
- GitHubSource
- GitHubEnterpriseSource
- BitBucketSource
Artifacts:
- Artifacts
- S3Artifacts
- CodePipelineArtifacts

I think I would also like to get rid of the NoSource / NoArtifacts classes (make them package-private). I don't see a reason why would a customer ever want to use them.

eladb · 2019-06-11T18:15:41Z

Names look good. Not sure why you think NoSource and NoArtifacts are not useful. It’s actually a very common use case in the CDK’s ops app (all our canaries are codebuild projects without a source)

skinny85 · 2019-06-11T18:20:00Z

Because NoSource / NoArtifacts is the default.

eladb · 2019-06-11T18:43:10Z

got it. Yeah I guess as long as users can use these features I guess you can hide those.

skinny85 · 2019-06-12T23:16:59Z

I've re-purposed the PR for general changes in the CodeBuild modules (see description).

skinny85 · 2019-06-17T00:53:19Z

Please re-review when you have a chance @eladb @rix0rrr .

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

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

packages/@aws-cdk/aws-codebuild/lib/codepipeline-artifacts.ts

packages/@aws-cdk/aws-codebuild/lib/codepipeline-source.ts

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

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

skinny85 · 2019-06-17T19:44:33Z

Changes taking into account the comments from the previous revision.

Change the names of the source classes to be more consistent with other integrations. Introduce an ISource interface. Make all Source subclasses package-private, and use static factory methods on Source (renamed from BuildSource) instead. Change the names of artifacts classes to be more consistent with other integrations. Introduce the IArtifacts interface. Make all Artifacts subclasses package-private, and use static factory methods on Artifacts (renamed from BuildArtifacts) instead. Get rid of the SourceType enum. Remove the buildScriptAsset and buildScriptAssetEntrypoint properties. Make Project implements ec2.IConnectable instead of exposing secuirtyGroups directly. BREAKING CHANGE: * codebuild: rename BuildSource to Source, BuildArtifacts to Artifacts * codebuild: all Source and Artifacts classes can only be created through static factory methods instead of constructors * codebuild: the construction properties buildScriptAsset and buildScriptAssetEntrypoint were removed * codebuild: the Projet.securityGroups property has been removed; Project now implements ec2.IConnectable instead

skinny85 · 2019-06-17T23:31:53Z

Latest batch of changes.

eladb

See some comments for future improvements.

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

packages/@aws-cdk/aws-codebuild/lib/codepipeline-artifacts.ts

packages/@aws-cdk/aws-codebuild/lib/codepipeline-source.ts

skinny85 requested review from RomainMuller and rix0rrr June 4, 2019 21:43

skinny85 requested a review from a team as a code owner June 4, 2019 21:43

skinny85 self-assigned this Jun 4, 2019

rix0rrr reviewed Jun 5, 2019

View reviewed changes

packages/@aws-cdk/aws-codebuild-artifacts/lib/artifacts.ts Outdated Show resolved Hide resolved

eladb reviewed Jun 5, 2019

View reviewed changes

packages/@aws-cdk/aws-codebuild-artifacts/lib/artifacts.ts Outdated Show resolved Hide resolved

skinny85 force-pushed the refactor/codebuild-integrations branch from d1b5f7d to a39a748 Compare June 12, 2019 23:16

skinny85 changed the title ~~refactor(codebuild): move sources and artifacts into their own separa…~~ fix(codebuild): API cleanup Jun 12, 2019

skinny85 force-pushed the refactor/codebuild-integrations branch from a39a748 to f88d482 Compare June 16, 2019 22:19

eladb reviewed Jun 17, 2019

View reviewed changes

skinny85 force-pushed the refactor/codebuild-integrations branch from f88d482 to e753892 Compare June 17, 2019 19:29

skinny85 force-pushed the refactor/codebuild-integrations branch from e753892 to d76083f Compare June 17, 2019 20:01

skinny85 force-pushed the refactor/codebuild-integrations branch from d76083f to 6403b4f Compare June 17, 2019 23:29

skinny85 mentioned this pull request Jun 17, 2019

Add a new CodeBuild Source, AssetSource #2897

Closed

eladb approved these changes Jun 18, 2019

View reviewed changes

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

packages/@aws-cdk/aws-codebuild/lib/codepipeline-artifacts.ts Show resolved Hide resolved

packages/@aws-cdk/aws-codebuild/lib/codepipeline-source.ts Show resolved Hide resolved

eladb merged commit c3667d7 into aws:master Jun 18, 2019

snyk-bot mentioned this pull request Aug 22, 2019

[Snyk] Upgrade @aws-cdk/aws-ecs from 0.18.1 to 0.39.0 udinachmany/trivia-api#5

Open

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

This was referenced Jan 20, 2020

[Snyk] Upgrade @aws-cdk/aws-route53 from 0.22.0 to 0.39.0 MechanicalRock/tech-radar#20

Closed

[Snyk] Upgrade @aws-cdk/aws-s3 from 0.22.0 to 0.39.0 MechanicalRock/tech-radar#21

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(codebuild): API cleanup #2745

fix(codebuild): API cleanup #2745

skinny85 commented Jun 4, 2019 •

edited

Loading

rix0rrr left a comment

rix0rrr commented Jun 5, 2019 •

edited

Loading

eladb left a comment

skinny85 commented Jun 11, 2019

eladb commented Jun 11, 2019

skinny85 commented Jun 11, 2019

eladb commented Jun 11, 2019

skinny85 commented Jun 11, 2019 •

edited

Loading

eladb commented Jun 11, 2019

skinny85 commented Jun 11, 2019

eladb commented Jun 11, 2019

skinny85 commented Jun 12, 2019

skinny85 commented Jun 17, 2019

skinny85 commented Jun 17, 2019

skinny85 commented Jun 17, 2019

eladb left a comment

fix(codebuild): API cleanup #2745

fix(codebuild): API cleanup #2745

Conversation

skinny85 commented Jun 4, 2019 • edited Loading

Pull Request Checklist

rix0rrr left a comment

Choose a reason for hiding this comment

rix0rrr commented Jun 5, 2019 • edited Loading

eladb left a comment

Choose a reason for hiding this comment

skinny85 commented Jun 11, 2019

eladb commented Jun 11, 2019

skinny85 commented Jun 11, 2019

eladb commented Jun 11, 2019

skinny85 commented Jun 11, 2019 • edited Loading

eladb commented Jun 11, 2019

skinny85 commented Jun 11, 2019

eladb commented Jun 11, 2019

skinny85 commented Jun 12, 2019

skinny85 commented Jun 17, 2019

skinny85 commented Jun 17, 2019

skinny85 commented Jun 17, 2019

eladb left a comment

Choose a reason for hiding this comment

skinny85 commented Jun 4, 2019 •

edited

Loading

rix0rrr commented Jun 5, 2019 •

edited

Loading

skinny85 commented Jun 11, 2019 •

edited

Loading