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

feat: re-structure the CodePipeline Construct library API #1590

Merged
merged 1 commit into from
Feb 8, 2019

Conversation

skinny85
Copy link
Contributor

@skinny85 skinny85 commented Jan 22, 2019

This Pull Request re-structures the API of the CodePipeline Construct library.

There are 2 fundamental changes that are made here:

  1. Stage and Action are no longer Constructs - they're just regular value classes.
  2. Stages and Actions have to be explicitly added to a Pipeline / Stage, respectively, to be reflected in the resulting CloudFormation template. Before, they couldn't be created without referencing an existing Pipeline / Stage.

All of the changes are the just consequences of the above 2 points.

As a side effect, because of the late-binding of the Stages / Actions, we can't really automatically infer the artifacts anymore. So, this changes the API to have the input artifacts to Action always be provided explicitly, same as PR #1389 does for the "old" API.

This contains like 30 BREAKING CHANGES.

Previous experience:

const pipeline = new codepipeline.Pipeline(this, 'Pipeline');

const sourceStage = new codepipeline.Stage(this, 'Source', {
  pipeline,
});
const sourceAction = codeCommitRepo.addToPipeline(sourceStage, 'CodeCommit');

const buildStage = pipeline.addStage('Build');
const buildAction = new codebuild.PipelineBuildAction(this, 'CodeBuild', {
  stage: buildStage,
  project: codeBuildProject,
});

New experience:

const sourceAction = codeCommitRepo.toCodePipelineSourceAction({ actionName: 'CodeCommit' });

const pipeline = new codepipeline.Pipeline(this, 'Pipeline', {
  stages: [
    {
      name: 'Source',
      actions: [sourceAction],
    },
  ],
});
pipeline.addStage({
  name: 'Build',
  actions: [
    new codebuild.PipelineBuildAction({
      actionName: 'CodeBuild',
      project: codeBuildProject,
      inputArtifact: sourceAction.outputArtifact,
    }),
  ],
});

Pull Request Checklist

  • Testing
    • Unit test added
    • CLI change?: manually run integration tests and paste output as a PR comment
    • 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 skinny85 requested a review from a team as a code owner January 22, 2019 02:36
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.

Hey, would it be okay if I’d review this next week? I am traveling this week and won’t be able to dive deep.

There are few things that pop up from initial glance, but I’d like to take the time and provide a thorough feedback.

In the meantime, it will help if you provide some context and motivation behind this change in the PR description as well as a detailed list of changes for people to be able to migrate from the previous version. This library is heavily used and we should make sure the upgrade is not too painful.

@sam-goodwin
Copy link
Contributor

Your title prefix should be changed to feat(codepipeline)

@sam-goodwin sam-goodwin added the @aws-cdk/aws-codepipeline Related to AWS CodePipeline label Jan 22, 2019
@skinny85
Copy link
Contributor Author

Your title prefix should be changed to feat(codepipeline)

The changes are not limited to only the aws-codepipeline module.

@skinny85
Copy link
Contributor Author

In the meantime, it will help if you provide some context and motivation behind this change in the PR description as well as a detailed list of changes for people to be able to migrate from the previous version. This library is heavily used and we should make sure the upgrade is not too painful.

Updated the PR description, let me know if this is what you had in mind.

@aecollver
Copy link

aecollver commented Feb 1, 2019

Why not make the stage and action name a part of the props object instead of a separate parameter? This gives you the option to make the name an optional prop (and generate a name when omitted) in the future without breaking compatibility.

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.

This looks good. Definitely looks cleaner in certain areas.

Pre-bind References

There are a bunch of places that actions have public APIs that require that you first add the action to the stage before you can access them. Instead of making these properties optional, how about defining them as getters and throwing a runtime error in case people try to access them before the action has been added to the stage. This will at least not leak this implementation detail to the API, so that users don't have to constantly null-check when they use these properties.

Artifacts

I feel quite strongly about automatically wiring artifacts when it's clearly what the user would want. It's a pretty important design tenet that we try to maintain - don't ask the user to configure something if you can safely deduce it for them. This is even more important since the API for working with artifacts is clumsy and verbose, and the 90% use case it's basically pure boilerplate.

asCodePipelineAction

Generally asXxx methods are reserved for internal inversion of control (and we plan to actually rename them to _asXxx to indicate that). I am also wondering if this actually adds value since you will now need to add the action to a stage after you've retrieved it from the resource. Let's just get rid of these methods for now, and we can always add them later if we feel like they add value (the opposite will be a breaking change).

packages/@aws-cdk/aws-codepipeline/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-codepipeline/README.md Show resolved Hide resolved
packages/@aws-cdk/aws-codepipeline-api/lib/action.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-codepipeline-api/lib/action.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-codepipeline/lib/stage.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-codepipeline/lib/stage.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-codepipeline/lib/stage.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-codepipeline/lib/stage.ts Outdated Show resolved Hide resolved
@eladb
Copy link
Contributor

eladb commented Feb 4, 2019

Rethinking about asCodePipelineAction and seeing this pattern emerge from #1646. I can see the value in discoverability, so maybe toCodePipelineAction() makes sense... What do you think?

@aecollver
Copy link

aecollver commented Feb 4, 2019

I feel quite strongly about automatically wiring artifacts when it's clearly what the user would want. It's a pretty important design tenet that we try to maintain - don't ask the user to configure something if you can safely deduce it for them. This is even more important since the API for working with artifacts is clumsy and verbose, and the 90% use case it's basically pure boilerplate.

I'd guess it's more like 10% of use cases where wiring artifacts are boilerplate (e.g. there's a single artifact). As soon as you have two artifacts and add a new action, which artifact should you choose and why (it's not always the last produced artifact)? I'm happy to iterate on this more, but from a backward compatibility standpoint we can relax the "require artifacts to be wired up explicitly" later.

Perhaps one way to address the "clumsy and verbose" is to introduce an artifact object? This would let you generate artifact names if not specified, use the IDE to rename references to the variable (in contrast to strings today) and give you an object to attach a method for path references (e.g. the CloudFormation action configuration references files in an artifact).

Rethinking about asCodePipelineAction and seeing this pattern emerge from #1646. I can see the value in discoverability, so maybe toCodePipelineAction() makes sense... What do you think?

Or maybe createCodePipelineAction or newCodePipelineAction because it is a factory method?

Finally, bumping this comment so it doesn't get lost:

Why not make the stage and action name a part of the props object instead of a separate parameter? This gives you the option to make the name an optional prop (and generate a name when omitted) in the future without breaking compatibility.

@eladb
Copy link
Contributor

eladb commented Feb 4, 2019

I'd guess it's more like 10% of use cases where wiring artifacts are boilerplate (e.g. there's a single artifact). As soon as you have two artifacts and add a new action, which artifact should you choose and why (it's not always the last produced artifact)? I'm happy to iterate on this more, but from a backward compatibility standpoint we can relax the "require artifacts to be wired up explicitly" later.

Sure, as you have more artifacts you'll need to be explicit, but as I mentioned, we have a design tenet to provide "smart defaults" and this falls exactly in that category. If 90% of our users will ask themselves "why do I need to specify this" than we should do the right thing for them. Since we already have this capability I am not sure why we want to make our APIs less useful as we iterate...

Perhaps one way to address the "clumsy and verbose" is to introduce an artifact object? This would let you generate artifact names if not specified, use the IDE to rename references to the variable (in contract to strings today) and give you an object to attach a method for path references (e.g. the CloudFormation action configuration references files in an artifact).

I remember you suggested this idea and I like it better than strings. It will still feel "clumsy and verbose" in my mind when those artifacts can easily be deduced from the context, but for the explicit case, yes! How would that work for when you specify the stages/actions upon pipeline creation? Would you need to define your artifact objects before?

Or maybe createCodePipelineAction or newCodePipelineAction because it is a factory method?

See my followup comment. This seems like it's going to be a pattern (step functions are another example), and I am proposing maybe toCodePIpelineAction as a name.

Why not make the stage and action name a part of the props object instead of a separate parameter? This gives you the option to make the name an optional prop (and generate a name when omitted) in the future without breaking compatibility.

Yeah, why not?

@aecollver
Copy link

I'd guess it's more like 10% of use cases where wiring artifacts are boilerplate (e.g. there's a single artifact). As soon as you have two artifacts and add a new action, which artifact should you choose and why (it's not always the last produced artifact)? I'm happy to iterate on this more, but from a backward compatibility standpoint we can relax the "require artifacts to be wired up explicitly" later.

Sure, as you have more artifacts you'll need to be explicit, but as I mentioned, we have a design tenet to provide "smart defaults" and this falls exactly in that category. If 90% of our users will ask themselves "why do I need to specify this" than we should do the right thing for them. Since we already have this capability I am not sure why we want to make our APIs less useful as we iterate...

This isn't solely a question of convenience. The current feature is potentially surprising and dangerous: I can accidentally publish source code instead of a compiled artifact or mistakenly deploy the wrong thing if I rearrange actions in my pipeline. I'd like to start by requiring artifacts to be wired up explicitly (so we trade "potentially surprising and dangerous" for "smart defaults" and get the breaking change out of the way) and then iterate on adding "smart defaults" for use cases where it's safe/makes sense.

Perhaps one way to address the "clumsy and verbose" is to introduce an artifact object? This would let you generate artifact names if not specified, use the IDE to rename references to the variable (in contract to strings today) and give you an object to attach a method for path references (e.g. the CloudFormation action configuration references files in an artifact).

I remember you suggested this idea and I like it better than strings. It will still feel "clumsy and verbose" in my mind when those artifacts can easily be deduced from the context, but for the explicit case, yes! How would that work for when you specify the stages/actions upon pipeline creation? Would you need to define your artifact objects before?

Or maybe createCodePipelineAction or newCodePipelineAction because it is a factory method?

See my followup comment. This seems like it's going to be a pattern (step functions are another example), and I am proposing maybe toCodePIpelineAction as a name.

I'm not opposed to toCodePIpelineAction - just throwing out some alternatives to consider.

We may want to include an optional props in the pattern. Example: users could use props to differentiate between instantiating a CodeBuild build or test action.

@eladb
Copy link
Contributor

eladb commented Feb 4, 2019

"potentially surprising and dangerous"

Hard to argue when you pull out this card...

We may want to include an optional props in the pattern. Example: users could use props to differentiate between instantiating a CodeBuild build or test action.

Absolutely.

@aecollver
Copy link

"potentially surprising and dangerous"

Hard to argue when you pull out this card...

I think we want the same thing ("smart defaults") and it's mostly a question of whether we start with them or incrementally add them. Stage, action and artifact names are some other examples where we can generate "smart defaults" and let users override them.

@skinny85 skinny85 force-pushed the feature/pipeline-restructuring branch from 12f1576 to 739da9d Compare February 6, 2019 02:44
@skinny85
Copy link
Contributor Author

skinny85 commented Feb 6, 2019

Iteration nr 2

I just pushed the second iteration of the PR. Major changes are:

  1. Different way to create Stages (StageProps interface passed to either the Pipeline when constructing it or Pipeline#addStage, which returns IStage), according to @eladb's suggestion.
  2. Made both the Stage and Action names properties of interfaces instead of being separate parameters, according to @aecollver's suggestion.
  3. Renamed asCodePipelineAction methods to toCodePipelineAction.

@aecollver and @eladb , I would appreciate another pass :). Thanks!

@skinny85 skinny85 force-pushed the feature/pipeline-restructuring branch from 739da9d to 2aa4d1d Compare February 7, 2019 00:45
@skinny85
Copy link
Contributor Author

skinny85 commented Feb 7, 2019

Iteration 3

Just submitted the third iteration, with @eladb's suggestions:

  1. Changing the bind API in Actions to take an IStage, which then has a pipeline: IPipeline property.
  2. Renaming all occurrences of parent to scope.
  3. Not using ! to access possibly undefined variables.
  4. Update the ReadMe files of each of the packages.

@aecollver and @eladb , hopefully we're getting close :)

@skinny85 skinny85 force-pushed the feature/pipeline-restructuring branch from 2aa4d1d to b971a49 Compare February 7, 2019 20:23
@skinny85
Copy link
Contributor Author

skinny85 commented Feb 7, 2019

Rebased on top of newest master (0.24.1).

@eladb
Copy link
Contributor

eladb commented Feb 8, 2019

Great stuff dude!

@skinny85 skinny85 force-pushed the feature/pipeline-restructuring branch from b971a49 to 2a3dfb5 Compare February 8, 2019 21:09
@skinny85
Copy link
Contributor Author

skinny85 commented Feb 8, 2019

One last rebase (a conflict in the ReadMe file for CodeBuild).

@skinny85 skinny85 merged commit 3c3db07 into aws:master Feb 8, 2019
@skinny85 skinny85 deleted the feature/pipeline-restructuring branch February 8, 2019 23:48
@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
@aws-cdk/aws-codepipeline Related to AWS CodePipeline contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants