fix (Change log group references to ILogGroup, don't create a second log group in aws-*-step-function) #211
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
| @@ -74,7 +74,7 @@ _Parameters_ | |||
| |:-------------|:----------------|-----------------| | |||
| |lambdaFunction|[`lambda.Function`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-lambda.Function.html)|Returns an instance of the Lambda function created by the pattern.| | |||
| |stateMachine|[`sfn.StateMachine`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-stepfunctions.StateMachine.html)|Returns an instance of StateMachine created by the construct.| | |||
| |stateMachineLogGroup|[`logs.LogGroup`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-logs.LogGroup.html)|Returns an instance of the LogGroup created by the construct for StateMachine| | |||
| |stateMachineLogGroup|[`logs.ILogGroup`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-logs.ILogGroup.html)|Returns an instance of the LogGroup created by the construct for StateMachine| | |||
There was a problem hiding this comment.
We should update the property description to reference ILogGroup here as well (i.e. "Returns an instance of the ILogGroup created by the construct for the StateMachine.").
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
There was a problem hiding this comment.
Reviewed all files in the pull request. Helper function code looks good. Test coverage appears to be under 80% for the helper however. Recommend updating/organizing the test suite for the helper to more explicitly assert the outcomes for each of the four possibilities here (logGroup provided in stateMachineProps, logGroupProps not provided, logGroupProps provided w/o logGroupName, and logGroupProps provided w/ logGroupName). Overall nice, concise fix!
| @@ -70,7 +70,7 @@ _Parameters_ | |||
| | **Name** | **Type** | **Description** | | |||
| |:-------------|:----------------|-----------------| | |||
| |stateMachine|[`sfn.StateMachine`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-stepfunctions.StateMachine.html)|Returns an instance of sfn.StateMachine created by the construct| | |||
| |stateMachineLogGroup|[`logs.LogGroup`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-logs.LogGroup.html)|Returns an instance of the LogGroup created by the construct for StateMachine| | |||
| |stateMachineLogGroup|[`logs.ILogGroup`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-logs.ILogGroup.html)|Returns an instance of the LogGroup created by the construct for StateMachine| | |||
There was a problem hiding this comment.
Recommend same description update as noted for aws-lambda-stepfunction.
| @@ -70,7 +70,7 @@ _Parameters_ | |||
| |:-------------|:----------------|-----------------| | |||
| |eventsRule|[`events.Rule`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-events.Rule.html)|Returns an instance of events.Rule created by the construct| | |||
| |stateMachine|[`sfn.StateMachine`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-stepfunctions.StateMachine.html)|Returns an instance of sfn.StateMachine created by the construct| | |||
| |stateMachineLogGroup|[`logs.LogGroup`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-logs.LogGroup.html)|Returns an instance of the LogGroup created by the construct for StateMachine| | |||
| |stateMachineLogGroup|[`logs.ILogGroup`](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-logs.ILogGroup.html)|Returns an instance of the LogGroup created by the construct for StateMachine| | |||
There was a problem hiding this comment.
We should update the property description to reference ILogGroup here as well (i.e. "Returns an instance of the ILogGroup created by the construct for the StateMachine.").
|
Approved by Ryan, Hitendra is OOTO. |
*Issue #, if available:*193
Description of changes:
This revealed a larger bug in the Step Functions constructs. The construct was not looking at stateMachineProps.logs to see if an existing log group had been provided and created a new log group every time. This means that there was no way to provide an existing log group. We decided to fix this even though it it may be a breaking change for outlying use cases.
The issue is that stateMachineProps.logs is ILogGroup, not LogGroup. Since ILogGroup cannot be upgraded to LogGroup, including stateMachineProps.logs as a source of log group meant that all references to Log Groups have to be converted to ILogGroup, including the properties of the constructs.
If you access the Log Group of a Step Functions construct, you may need to change the type to ILogGroup if you ever explicitly specify the type.
If you access the Log Group of a Step Functions construct and make any changes to the Log Group after the construct has been instantiated, your code will no longer work as ILogGroups cannot be altered. Your workaround will be to create a log group to your full specification before launching the Step Functions construct, the set the stateMachineProps.logs.destination to this new log group (a LogGroup object can be "downgraded" to an ILogGroup).
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.