feat(aws-codepipeline): Jenkins build and test Actions

[skinny85]

As Jenkins Actions require registering a custom Action, I've also added a skeleton of that functionality, right now not exported from the aws-codepipeline module.

The weird part is that I had to define CustomActionRegistrationProps in the aws-codepipeline-api module (as they should be usable from Action classes, that have access to _registerCustomAction(CustomActionRegistrationProps)), but at the same time I had to put CustomActionRegistration in aws-codepipeline (as it uses the CodePipeline L1 in its implementation). It works fine, it's just that we always put the construction props next to their classes - but I don't see a different way forward in this particular case.

---

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

[rix0rrr]

The weird part is that I had to define CustomActionRegistrationProps in the aws-codepipeline-api module (as they should be usable from Action classes, that have access to _registerCustomAction(CustomActionRegistrationProps)), but at the same time I had to put CustomActionRegistration in aws-codepipeline (as it uses the CodePipeline L1 in its implementation).

Seems to me there should actually be two interface declarations, that only accidentally have the same properties but are actually different objects.

One is:

• CustomActionRegistration + CustomActionRegistrationProps, the custom action registration construct.
• RegisterCustomActionOptions, the arguments to the _registerCustomAction function.

The implementor of _registerCustomAction turns a RegisterCustomActionOptions -> CustomActionRegistrationProps (which in TypeScript is fortunately very easy, because they happen to be structurally compatible).

This also makes it possible to have different arguments/defaults in the two, which might be relevant at some point.

As an aside, I've taken to this convention:

• XxxProps -> Arguments for creating a construct, a "thing", a noun.
• XxxOptions -> Arguments for performing an operation (a verb), additional keyword arguments to a method.

The XxxOptions name is generally used in other JavaScript libraries where they allow customization parameters and they call them "options".

But this naming convention has not been ratified by anyone :)

[skinny85]

In the newest revision, I've completely changed the approach. I've elevated JenkinsProvider to a top-level Construct, and then referenced it in the Jenkins Actions.

I'm curious what you think of this approach @rix0rrr.

[rix0rrr]

So you're now making the registration explicit huh?

I like it! I think the API could be simplified a little further, but I like the approach!

[skinny85]

@rix0rrr I've changed the API according to your suggestions. Let me know if this is what you had in mind!

[skinny85]

Rebased on top of the new master to get the builds going again (master was broken before).

@rix0rrr still waiting for your comments here!

[rix0rrr]

You can export forBuild and forTest here as well.

[skinny85]

I could, but I'm not sure it makes much sense. For example, then export would be affected whether addToPipeline / addToPipelineAsTest was called before, which I'm not sure is the correct behavior.

[rix0rrr]

What I'm trying to enable is the following 2 scenario's:

1. Define JenkinsProvider in the same stack as where it's used. Automatically works with a minimal amount of fuss.
2. Define JenkinsProvider in stack A. Enable "forBuild" upon construction (because it won't be done automatically). Export it and import it in the stack B. The imported one cannot be used for tests (it will throw an error because it knows it wasn't "set up" that way), but it can be used for builds.

The case that in a two-stack scenario "forBuild" may be implicitly switched on if you happen to use a build action in stack A is maybe not fantastic, but also doesn't hurt anything (imo), especially with runtime validation in stack B (because if you remove that code, the runtime error will prevent a broken template from from being synthesized).

The fact that in scenario (2) the imported provider in stack B automatically knows what actual providers have been registered on the real provider in stack A only makes it better. Without this automatic piggy-backing of configuration information you would still have had to manually switch on the correct registrations in stack A, and in stack B you would also have to either assume it was configured correctly with no way of checking, or you would have to pass this additional information on import.

[rix0rrr]

If you're concerned about ordering, fair enough. Not something we can avoid, really, in the paradigm that we've chosen. I'd hate to lose ergonomics because of it, and also not a blocker since you can always trigger the behavior you want with constructor arguments if the ordering happens to not work out in your favor.

[rix0rrr]

Do you like this better than a Build/Test/Both enum?

Also, docstrings.

[skinny85]

Do you like this better than a Build/Test/Both enum?

I do, but I won't insist if you really like the enum approach more :).

Also, docstrings.

Yep, done.

[rix0rrr]

Just wanted to have had the argument, and confirmation that you prefer this.

[rix0rrr]

If we make the forBuild and forTest arguments to the imported props, we can do a sanity check and throw a useful error here.

[skinny85]

Won't that make using the imported provider more irritating? I assume that forBuild and forTest will be false by default, so if you do:

const provider = codepipeline.JenkinsProvider.import(this, 'Provider', {
  providerName: 'Provider',
  serverUrl: 'http://my-jenkins.com:8080',
});

provider.addToPipeline(/* ... */);

and you would get an exception saying forBuild should be true when doing the import.

Not sure it's worth it...?

[rix0rrr]

I see your concern. My thoughts were that it would be used with the export()/import() combination instead of a manual import, in which case you wouldn't need to pass the additional argument yourself.

You seem to be optimizing for cross-app, while I'm optimizing for in-app, cross-stack. Is that fair to say?

[skinny85]

I'm not sure I'm optimizing for it, just pointing out a potential usability issue with that use case :)

[rix0rrr]

The way I'm looking at it, by deciding whether or not to add that parameter to the ImportProps, we're optimizing for usability of one of the two cases. We just have to decide which one we care more about.

[skinny85]

I think I prefer the current approach, for 2 reasons:

a) the link between specifying the forBuild: true parameter to import before calling provider.addToPipeline(...) seems very unintuitive to me. I understand we can have a nice error message, but still, I don't love this approach.
b) I can't see a good use case for creating the provider in one Stack, but adding it to the Pipeline in another. Even if someone tried to do it, it wouldn't actually work! The imported Provider would throw an exception saying it wasn't marked for build. Of course, you could pass forBuild: true when creating it in the original Stack, but again, I don't find that intuitive.

Personally, if you feel strongly about having this validation, then I would rather go back to having separate Providers for build & test. I don't think forcing passing forBuild/forTest: true, and in a call before you get the actual exception, is a great API.

[rix0rrr]

Alrighty then. That was the last point of discussion, right? So ship it?

[skinny85]

So ship it?

If you don't have any other comments, then yep :)

[rix0rrr]

Already approved on my part so go ahead

[skinny85]

Rebased to resolve conflict.

[skinny85]

New revision due to upstream changes (parent -> scope, aws-linter warnings).