Skip to content
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

Adding sample for ddb and kinesis streams #219

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

giograno
Copy link
Contributor

@giograno giograno commented Jun 7, 2023

Adding the sample we pitched today.

@giograno giograno requested a review from thrau June 7, 2023 17:40
@giograno giograno marked this pull request as ready for review June 7, 2023 17:41
@giograno giograno force-pushed the dynamodb-kinesis-stream branch from a7cb99b to 35ef51e Compare June 7, 2023 17:53
@thrau thrau requested a review from baermat June 9, 2023 08:56
@thrau
Copy link
Contributor

thrau commented Jun 9, 2023

I added @baermat as reviewer to give feedback on whether the PR satisfies all the criteria needed for the samples

Copy link
Contributor

@thrau thrau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool thanks for adding! Just a really minor nit: Could we format the python code with black maybe?

Copy link
Contributor

@baermat baermat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, and sorry for the late review, this one slipped through 😬
I have one small change request 🙂


run: ## Deploy and run the sample locally
@(test -d .terraform || tflocal init) && tflocal apply --auto-approve
pip install boto3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pip install should probably be in the install section. Also it would probably make sense to put this into a venv to not cause problems on the host (we probably should put even the "command" installs into a venv to make it cleaner, but that's how it is right now across the samples). see e.g. https://github.com/localstack/localstack-pro-samples/blob/master/iot-basics/Makefile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants