feat(aws-s3): Add DeployAction for codepipeline #1596
Conversation
There was a problem hiding this comment.
Thanks for the contribution @hoegertn!
I have some small comments inline, and 1 large one: can you add an integration test to the aws-codepipeline package, testing this functionality? You can take inspiration from one of the tests already there - they're the one with the integ prefix. We have some docs about how to use them here.
Thanks!
| @@ -36,7 +36,7 @@ export interface CommonPipelineSourceActionProps extends codepipeline.CommonActi | |||
| * Construction properties of the {@link PipelineSourceAction S3 source Action}. | |||
| */ | |||
| export interface PipelineSourceActionProps extends CommonPipelineSourceActionProps, | |||
| codepipeline.CommonActionConstructProps { | |||
| codepipeline.CommonActionConstructProps { | |||
There was a problem hiding this comment.
Could you keep the double indent here? Thanks!
| * Construction properties of the {@link PipelineDeployAction S3 deploy Action}. | ||
| */ | ||
| export interface PipelineDeployActionProps extends codepipeline.CommonActionProps, | ||
| codepipeline.CommonActionConstructProps { |
There was a problem hiding this comment.
Double indent here, as above.
| /** | ||
| * The Amazon S3 bucket that is the deploy target | ||
| */ | ||
| bucket: IBucket; |
There was a problem hiding this comment.
There should be an empty line after each property in this interface, to separate them more clearly.
| /** | ||
| * The inputArtifact to deploy to Amazon S3 | ||
| */ | ||
| inputArtifact: codepipeline.Artifact; |
There was a problem hiding this comment.
Input artifact can be optional here (inputArtifact?: codepipeline.Artifact). In that case, the CodePipeline Construct will attempt to wire the input automatically.
| super(scope, id, { | ||
| stage: props.stage, | ||
| runOrder: props.runOrder, | ||
| owner: 'AWS', |
There was a problem hiding this comment.
This is the default value for owner, so feel free to skip providing this parameter.
| runOrder: props.runOrder, | ||
| owner: 'AWS', | ||
| provider: 'S3', | ||
| inputArtifact: props.inputArtifact, |
There was a problem hiding this comment.
Instead of passing these from props, you can use TypeScript's spread operator:
super(scope, id, {
provider: 'S3',
artifactBounds: {
minInputs: 1,
maxInputs: 1,
minOutputs: 0,
maxOutputs: 0,
},
configuration: {
BucketName: props.bucket.bucketName,
Extract: props.extract || 'true',
ObjectKey: props.objectKey,
},
...props,
});| */ | ||
| bucket: IBucket; | ||
| /** | ||
| * Should the deploy action extract the artifact before deploying to Amazon S3. Defaults to true |
There was a problem hiding this comment.
Use the @default JSDoc tag here:
/**
* Should the deploy Action extract the artifact before deploying to S3.
*
* @default true
*/| */ | ||
| extract?: boolean; | ||
| /** | ||
| * The key of the target object. This is required if extract is false. |
There was a problem hiding this comment.
It would be great if the construct validated that is true, and throw an error with a nice message if not.
| */ | ||
| objectKey?: string; | ||
| /** | ||
| * The inputArtifact to deploy to Amazon S3 |
There was a problem hiding this comment.
Missed a period at the end :)
| }, | ||
| configuration: { | ||
| BucketName: props.bucket.bucketName, | ||
| Extract: props.extract || 'true', |
There was a problem hiding this comment.
So this is a boolean in props, but the || is a String. Do you know which one is it? It might make a difference on the API level.
| */ | ||
| bucket: IBucket; | ||
| /** | ||
| * Should the deploy action extract the artifact before deploying to Amazon S3. Defaults to true |
There was a problem hiding this comment.
Document the default as
@default true
| */ | ||
| extract?: boolean; | ||
| /** | ||
| * The key of the target object. This is required if extract is false. |
|
Thanks for the review. I will fix all the issues soon. Sorry, I did not find the correct place to put the test or to find an example. |
|
@rix0rrr ping |
|
LGTM to me too |
|
hey @hoegertn , I've updated your PR with a conflict resolution, and I've also updated the test expectation for the integ tests (something changed on master in the meantime). |
Pull Request Checklist
fixes #1585
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.