refactor(codepipeline): introduce IAction and unify the Action.bind() signature #3012
Conversation
| @@ -12,3 +12,4 @@ export * from './lambda/invoke-action'; | |||
| export * from './manual-approval-action'; | |||
| export * from './s3/deploy-action'; | |||
| export * from './s3/source-action'; | |||
| export * from './action'; // for some reason, JSII fails building the module without exporting this class | |||
There was a problem hiding this comment.
I actually don't want to export this class - but if I don't, JSII fails building with an error. Any ideas how to sidestep that...?
There was a problem hiding this comment.
Corrected in the newest revision.
There was a problem hiding this comment.
My mistake - it is not in fact resolved. When I marked the class as @internal, the build for @aws-codepipeline succeeded, but then @app-delivery failed saying the CloudFormation Actions don't implement the IAction interface. So I reverted the change, and I'm back to exporting the class.
There was a problem hiding this comment.
Let's mark it as @experimental for now and we'll figure it out
skinny85
force-pushed
the
refactor/iaction
branch
from
f2b481f
to
123cd8f
Compare
…oduce the IAction interface.
skinny85
force-pushed
the
refactor/iaction
branch
from
123cd8f
to
43d5a0f
Compare
eladb
changed the title
| /** | ||
| * Low-level class for generic CodePipeline Actions. | ||
| */ | ||
| export abstract class Action implements codepipeline.IAction { |
There was a problem hiding this comment.
Minor: probably should be called ActionBase
| protected abstract bound(scope: Construct, stage: codepipeline.IStage, options: codepipeline.ActionBindOptions): | ||
| codepipeline.ActionConfig; | ||
|
|
||
| private get pipeline(): codepipeline.IPipeline { |
There was a problem hiding this comment.
Minor: seems like these are only used in onStateChange so I'd just move this code in there, and simplify (it's probably sufficient to check if this._pipeline is undefined).
|
|
||
| /** | ||
| * Low-level class for generic CodePipeline Actions. | ||
| */ |
There was a problem hiding this comment.
Minor: this is internal right? We are settling on a convention where the file name of internal classes will begin with _
| return this.bound(scope, stage, options); | ||
| } | ||
|
|
||
| public onStateChange(name: string, target?: events.IRuleTarget, options?: events.RuleProps) { |
There was a problem hiding this comment.
Something tells me onStateChange should not be here. Without it, we basically don't need ActionBase and it will greatly simplify things. But that's okay... Not that important.
| @@ -218,27 +236,12 @@ interface CloudFormationDeployActionProps extends CloudFormationActionProps { | |||
| */ | |||
| abstract class CloudFormationDeployAction extends CloudFormationAction { | |||
There was a problem hiding this comment.
Add a Base suffix (should be the convention for abstract base classes)
| @@ -12,3 +12,4 @@ export * from './lambda/invoke-action'; | |||
| export * from './manual-approval-action'; | |||
| export * from './s3/deploy-action'; | |||
| export * from './s3/source-action'; | |||
| export * from './action'; // for some reason, JSII fails building the module without exporting this class | |||
There was a problem hiding this comment.
Let's mark it as @experimental for now and we'll figure it out
This brings the Action
bind()API in line with our conventions.It also introduces an
IActioninterface for those who want to work with the low-level Action interface.The Action class has been moved from the aws-codepipeline to the aws-codepipeline-actions module.
This API is much more flexible, and I show the capabilities by changing the implementation of the
PipelineDeployStackActionfrom@app-delivery.BREAKING CHANGE:
PipelineDeployStackActionis now acodepipeline.IActioninstead of a Construct.Please read the contribution guidelines and follow the pull-request checklist.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license