New Pipeline#addStage convenience method

[skinny85]

Added to make creating Stage objects a little more concise.

See #265 for more discussion on this feature.

---

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

[skinny85]

Rebased and changed the PR base branch to master after merging #642.

[eladb]

Always use this (instead of this.parent) - the reason is that the parent construct is a lexical scope and the construct IDs must be unique amongst it's siblings. Since you don't control the parent's children, this will fail:

class MyConstruct extends cdk.Construct {
  constructor(...) {
    supre(...);

   new s3.Bucket(this, 'Hello');

    const pl = new codepipeline.Pipeline(this, 'MyPipeline');
    pl.addStage('Hello'); // BOOM! Two children with the same name ("Hello").
    
  }
}

Also, since construct ID must be unique, it means no two stages can have the same name. I don't believe this is a constraints in CodePipeline, but it will be a constraint here. Are we okay with this? If not, maybe you want to add an optional props here that will allow specifying stageName (or displayName) explicitly in case users do want this behavior.

[skinny85]

Sorry, I'm not following your example.

This will also fail:

  constructor(...) {
    super(...);

    new s3.Bucket(this, 'Hello');

    const pl = new codepipeline.Pipeline(this, 'MyPipeline');
    const stage = new codepipeline.Stage(this, 'Hello', { pipeline: pl }); // BOOM! Two children with the same name ("Hello").
  }

Which is exactly what we want - pipeline.addStage('x') should be exactly equivalent to new Stage(this, 'x', { pipeline }).

Also, since construct ID must be unique, it means no two stages can have the same name. I don't believe this is a constraints in CodePipeline, but it will be a constraint here.

It is in fact a constraint in CodePipeline (and one that we explicitly validate in the CodePipeline L2).

[eladb]

As a CDK developer, I expected the code new Stage(this, 'Hello') to fail (since I defined two constructs under the same parent) but not addStage(Hello) (which hides the fact that a stage is a construct).

Not sure I understand why for some reason you insist on pinning the inner stage construct to the parent's parent?

[skinny85]

I don't agree.

We keep saying that this addStage method is just 'syntactic sugar', a convenience that is equivalent to creating the Stage explicitly.

So, if as a CDK developer, I go from this code, which works:

cost stage = pipeline.addStage('Hello');

to this, which we claim is equivalent:

const stage = new codepipeline.Stage(this, 'Hello', { pipeline });

In your example, I will get an exception. So, they are clearly not equivalent, and that is quite surprising, and (in my mind) misleading.

Not to mention that if the Stage itself has any Constructs under it, going between the two forms will change the logical IDs of those children, and thus will re-create them in the Stack, and show up in the diff.

I argue that this is not what we want.

[eladb]

Following up on our chat conversation: I don't believe it is currently priority to make addStage and new Stage seamlessly interchangeable, and I do think there's value in maintaining the lexical and contextual scope when instantiating the inner Stage construct. If we could enforce this pattern by the compiler (and maybe at some point we will), we would have done that.

Let's just decide that instantiation of constructs always looks like this:

new MyConstruct(this, '<id>', props)

Since <id> is scoped by this, using a different scope (i.e. this.parent) can cause conflicts with that other scope, that users can't fix.

[eladb]

What's up with this?

[skinny85]

What's up with this?

I've responded to your comments above. In particular, I disagree with using this instead of this.parent.

[eladb]

Awaiting updates

[skinny85]

Awaiting updates

Changed to use this in the call to new Stage, and rebased.

[eladb]

Please add a unit test

[eladb]

I terms of the README narrative, I would recommend starting with this as the recommended way to add a stage to the pipeline and then mention that it is also possible to instantiate Stage directly (I don't even think it's worth providing an example unless there's a clear use case to demonstrate, such as defining a reusable subclass of Stage.

[skinny85]

Done.

[skinny85]

Please add a unit test

Done.