-
Notifications
You must be signed in to change notification settings - Fork 247
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(aws-lambda-stepfunctions): aws-lambda-stepfunctions implementation #285
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
*/ | ||
export interface LambdaToStepFunctionProps { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want to change anything in the old interface, if we change the name of the props interface and the construct class we will break any existing customer code.
* @since 0.8.0 | ||
* @access public | ||
*/ | ||
constructor(scope: Construct, id: string, props: LambdaToStepFunctionProps) { | ||
constructor(scope: Construct, id: string, props: LambdaToStepFunctionsProps) { | ||
super(scope, id); | ||
defaults.CheckProps(props); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code in the constructor should all be deleted. A new LambdaToStepfunctionsProps object should be created and populated from the incoming LambdaToStepFunctionProps values. Then the constructor will make a call like:
const wrappedConstruct = new LambdaToStepfunctions(this, ${id}-wrapped
, newLambdaToStepfunctionsProps);
// After that, the exposed properties of the construct class will need to be populated from the exposed properties of wrapptedConstruct.
Here is a minimal deployable pattern definition in Typescript: | ||
|
||
``` typescript | ||
import { LambdaToStepFunctions } from '@aws-solutions-constructs/aws-lambda-stepfunctions'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Name needs to be LambdaToStepfunctions to match design guidelines (we only uppercase the first letter - while that seems odd here if you think of LambdaToSqs the reason for the pattern becomes clearer).
(same for Props interface)
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 |
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 |
![Stability: Experimental](https://img.shields.io/badge/stability-Experimental-important.svg?style=for-the-badge) | ||
![Stability: Deprecated](https://img.shields.io/badge/STABILITY-DEPRECATED-red?style=for-the-badge) | ||
|
||
> Construct will be deprecated in favor of aws-lambda-stepfunctions in v2.x, but will be supported in v1.x as long as v1.x is supported. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to write up a paragraph explaining what we are doing and why that I will send you to put here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's replace line 8 with:
Some of our early constructs don’t meet the naming standards that evolved for the library. We are releasing completely feature compatible versions with corrected names. The underlying implementation code is the same regardless of whether you deploy the construct using the old or new name. We will support both names for all 1.x releases, but in 2.x we will only publish the correctly named constructs.
@@ -25,7 +25,7 @@ import { LambdaToDynamoDB } from '@aws-solutions-constructs/aws-lambda-dynamodb' | |||
import { LambdaToS3 } from '@aws-solutions-constructs/aws-lambda-s3'; | |||
import { LambdaToSns } from '@aws-solutions-constructs/aws-lambda-sns'; | |||
import { EventsRuleToLambda } from '@aws-solutions-constructs/aws-events-rule-lambda'; | |||
import { LambdaToStepFunction } from '@aws-solutions-constructs/aws-lambda-step-function'; | |||
import { LambdaToStepfunctions } from '@aws-solutions-constructs/aws-lambda-stepfunctions'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow - nice catch.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Issue #271 , if available:
Description of changes:
Transitioning from aws-lambda-step-function to new construct aws-lambda-stepfunctions to adhere to consistent naming conventions.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.