codebuild: merge "addBuildToPipeline" and "addTestToPipeline" APIs

[eladb]

Copy @skinny85

Let's consider normalizing this API together with the rest of the addToPipeline calls. I think we can just have an optional actionType property passed in, defaulting to build, and if someone wants to add the action as a test action, they can just set actionType to test.

[skinny85]

ACK 👌🏻

[skinny85]

Scratch that 👌🏻. I just remembered why we did it the way we did.

We can't have one method for this, as addBuildToPipeline and addTestToPipeline return different Action classes. The 2 Action types have slightly different APIs. While a Build Action always has an output (and hence its outputArtifact field is defined as a non-optional type Artifact), the Test one does not (and so its outputArtifact field is of an optional type Artifact | undefined).

How do you want to proceed here @eladb ?

[eladb]

Is it really true that all build actions always have an output? Reference says 0-5.

[skinny85]

In our model they do.

[eladb]

So maybe we should align to the service, and then we can make outputArtifact optional in both.

[skinny85]

But there's a reason why we modeled it as not optional. It seems highly unlikely that someone would have a build that doesn't produce an output. Making it optional would also force customers to give the output a string name, which they don't care about in most cases, as it's only needed by the Pipeline itself. Also, you would be forced to say buildAction.outputArtifact!, as the type would have to be outputArtifact?: Artifact. Finally, making it optional would make our automatic Action input detection produce surprising results.

I don't think changing all of those behaviors to unify addBuildToPipeline and addTestToPipeline is worth it.

[eladb]

We usually try not to be opinionated about the service capabilities in L2. If the service allows users to define a build action without outputs, we should enable that too. But I guess that's a different discussion (raise issue? bring up with the team?)

Here's a proposal for this conversation: I believe the most common case is to use a CodeBuild project as a "build" step, correct? How about we rename addBuildToPipeline to addToPipeline and rename addTestToPipeline to addToPipelineAsTest?

[skinny85]

Created issue: #1253.

Will submit a PR with the fix you mentioned @eladb.