feat: re-structure the CodePipeline Construct library API

[skinny85]

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.

[eladb]

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]

Your title prefix should be changed to feat(codepipeline)

[skinny85]

Your title prefix should be changed to feat(codepipeline)

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

[skinny85]

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]

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]

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).

[eladb]

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]

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]

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]

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]

"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]

"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]

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]

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]

Rebased on top of newest master (0.24.1).

[eladb]

Great stuff dude!

[skinny85]

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