feat(aws-codecommit): New method addToPipelineStage on Repository

[skinny85]

See #265 for the discussion about this feature.

This is a trial PR - if we agree with the direction taken here, I'll follow up with more PRs for S3, CodeBuild, CodeDeploy, etc.

---

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

[rix0rrr]

If you want to go this way, I will ship this. But to me, I'd want to start from the pipeline when modifying it.

Have we considered something like this already?

pipeline.addSource(repository.asPipelineSource({ artifactName: 'hello' }));

And with generics support in jsii, that could even be shortened to:

pipeline.addSource(repository, { artifactName: 'hello' });

(Or, going a different way, doesn't need generics but this one is easier to forget to configure:

pipeline.addSource(repository).configure({ artifactName: 'hello' }));

)

[skinny85]

pipeline.addSource(repository.asPipelineSource({ artifactName: 'hello' }));

I'm not a huge fan of this, as it hides the Stage completely. What name will it have? This works fine for source Actions, but what if I have a Stage that needs more than one Action (an approval, manual or with integ tests)? Will we have another variant that allows you to give the stage? That might be too many ways, all similar but slightly different, of achieving the same thing...

[RomainMuller]

Also, we need a solution that doesn't suck without jsii support for generics. We may have other API entry points that leverage generics to provide "reciprocate" views on those (allowing discoverability from both sides is in parts what we are trying to aim for here).

One thing, though, is you mention the pipeline.addSource(repository).configure({...}) doesn't require generics, but I actually think it does... If I'm wrong, can you please elaborate on how this works out to actually be type-safe? I'm interested in learning.

[RomainMuller]

I like it (but I guess you knew this already since we had discussed it earlier). Kinda still interested to know if @rix0rrr has particularly hard feelings on the subject and seeing what the sticky points are.

[RomainMuller]

That actually feels pretty good to me.

[skinny85]

👍🏻

[RomainMuller]

FYI - the {@link}isn't going to render properly in our Sphinx documentation (not that this isn't something we should fix :D)

[skinny85]

This is actually just a moved comment, so it doesn't bother me that much... yet ;)

[RomainMuller]

I don't reckon you need two of the !s :)

[eladb]

Why attach it to that parent and not to this?

[skinny85]

I don't reckon you need two of the !s :)

Yep, my bad. Will fix.

[skinny85]

Why attach it to that parent and not to this?

Because I want to make this:

repository.addToPipeline(stage, 'CodeCommit', {
    artifactName: 'SourceOutput',
});

be equivalent to this:

new codecommit.PipelineSourceAction(this, 'CodeCommit', {
    stage: stage,
    artifactName: 'SourceOutput',
    repository: repository,
});

If we attach the Action to the Repository, instead of its parent, that first call above would be equivalent to:

new codecommit.PipelineSourceAction(repository, 'CodeCommit', {
    // rest is the same as above...
});

, which is not the same thing.

[eladb]

Why is it important that they will be attached to the same construct?

[skinny85]

It's not strictly required. But, imagine a world where we move to managing a Role per each Action (instead of having a big Pipeline Role with all of the permissions). If an Action will create that Role, then the code repository.addToPipeline(stage, 'CodeCommit', { ...}) and new codecommit.PipelineSourceAction(this, 'CodeCommit', { repository, ... }) will produce different CloudFormation templates (the paths will be different, and so the logical names assigned to the Role will be different). Using the parent here prevents that.

What's a good reason for not using the parent here?

[rix0rrr]

I'm not a huge fan of this, as it hides the Stage completely. What name will it have?

That could all be solved by changing the arguments slightly. Feel free to read my example as:

stage.addAction(repository.toSourceAction({ actionName: 'Hello' }));

Or whatever if that makes more sense. My point was more about the object model and the order of operations than the exact type of object and set of parameters involved.

Only thing I wanted to say was, I think element.addToContainer(container) is backwards, and container.addElement(element) makes more sense.

But I just wanted to provide a final alternative, to see if they were considered. In particular:

container.addElement(elementProvider.supplyArguments({ args }));

But if everyone else is on board with the current API, I'm not one to stand in the way of progress.

[rix0rrr]

One thing, though, is you mention the pipeline.addSource(repository).configure({...}) doesn't require generics, but I actually think it does.

You are correct.

[skinny85]

@rix0rrr can you write out the entire signature of Stage#addAction that you use in the code stage.addAction(repository.toSourceAction({ actionName: 'Hello' })) - including the arguments and return type? Remember that it needs to return a codecommit.PipelineSource Action.

I don't believe it can be expressed without generics.

[rix0rrr]

@rix0rrr can you write out the entire signature of Stage#addAction

// Generic
interface IAction {
    public readonly name: string;
    public readonly toJSON(): any;
}

class Stage {
    public addAction(action: IAction) {
        this.actions.push(action);
    }
}

// Specific
class RepositorySourceAction implements IAction { ...elided... }

class Repository {
    public toSourceAction(props: RepositorySourceActionProps): IAction {
        return new RepositorySourceAction({
            repository: this,
            name: props.name,
            ...
        });
    }
}

The combination of IAction+RepositorySourceAction could potentially even be a single concrete class Action if we just say that class is just a type tag for an arbitrary property bag.

Maybe I'm thinking too simply about it? Tell me why this wouldn't work?

[skinny85]

It won't work, because, like I said above, Stage#addAction for a CodeCommit Repository needs to return the type codecommit.PipelineSourceAction (or, at the very least, the abstract codepipelineApi.SourceAction class). That source Action will be used in later definitions of the subsequent Actions, like build and/or deploy (you need to call sourceAction.artifact when defining those).

[rix0rrr]

You are right of course.

[eladb]

Should this be in the codepipeline package and extend CommonActionProps?

[skinny85]

I don't think so - CommonPipelineSourceProps contains things like branch and pollForSourceChanges, which are CodeCommit-specific, so I don't think it belongs in the codepipeline package.

[eladb]

Avoid multiple inheritance if possible (I think CommonPiplineSourceProps should just extend CommonActionProps)

[skinny85]

CommonPiplineSourceProps cannot extend CommonActionProps, because CommonActionProps contain the required stage attribute, which we don't need to provide when calling Repository#addToPipeline(Stage, name: string, props: CommonPiplineSourceProps).

I don't believe it's possible to avoid multiple inheritance here.

[eladb]

The word Stage is redundant (since the type of the first argument tells this story). addToPipeline should be fine in my mind

[skinny85]

Hah, that's funny. addToPipeline is actually the name I proposed originally, and @RomainMuller asked to change it to addToPipelineStage :)

[eladb]

Why attach it to that parent and not to this?

[skinny85]

Rebased and incorporated Romain's and Elad's feedback.

[eladb]

Why is artifact name required? Can we get rid of them already (okay if we do that in a subsequent commit). But as far as I understand, they are only needed for referencing in the pipeline model, and in the CDK we can just use an artifact object instead and generate the Artifact name from eg the uniqueId for the Action construct.

[skinny85]

Yes. We have an issue opened for getting rid of (or at least defaulting) them.