-
Notifications
You must be signed in to change notification settings - Fork 59
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
add sample for stepfunctions using lambdas and localstack #148
Conversation
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.
Added some comments. I feel like this sample is generally missing a proper scenario/intent. It's not very clear what kind of process this tries to model and why that would be particularly useful for a user.
Since it's just a sample this wouldn't block a merge though 👍
@@ -26,6 +26,7 @@ jobs: | |||
- name: Install Dependencies | |||
run: | | |||
pip install virtualenv | |||
pip install --upgrade pyopenssl |
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 needs to happen before installing localstack, i.e. before line 24
localstack start -d | ||
|
||
stop: | ||
@echo |
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.
?
--input '{"adam": "LocalStack", "cole": "Stack"}' | ||
``` | ||
|
||
This creates and invokes the flow between the three Lambda functions we created using LocalStack earlier. |
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.
Feels like this is missing some context about what this sample actually does and why. It's really hard to see the intent here. E.g. why is adam
set to LocalStack
and cole
to Stack
?
|
||
```sh | ||
awslocal stepfunctions create-state-machine --name step-demo \ | ||
--definition "$(cat step-definition.json)" \ |
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.
Why is this not using the same pattern as above in the Makefile? (file://....
)
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.
Its not needed here
stepfunctions-lambda/Makefile
Outdated
@@ -0,0 +1,69 @@ | |||
export AWS_ACCESS_KEY_ID ?= test | |||
export AWS_SECRET_ACCESS_KEY ?= test | |||
export AWS_DEFAULT_REGION = us-east-1 |
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.
Do we intentionally overwrite the default region here or should it be ?=
?
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.
On standard template, its export AWS_DEFAULT_REGION=us-east-1
0aadaec
to
70c68ee
Compare
No description provided.