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

New AWS resource: aws_lambda_event_source_mapping #4093

Merged
merged 1 commit into from
Dec 1, 2015

Conversation

vancluever
Copy link
Contributor

This PR adds a new resource: aws_lambda_event_source_mapping, allowing for the creation of event sources for Lambda functions from either Kinesis or DynamoDB.

This satisfies #3897, and by proxy #4089.

PR includes the resource, the test, and the doc page for the website.

Issues

This resource suffers from the same IAM-related issues that currently other resources are suffering from, manifesting itself through issues like:

Error creating Lambda event source mapping: InvalidParameterValueException: Cannot access
stream arn:aws:kinesis:us-east-1:123456789012:stream/stream_name. Please ensure the role can
perform the GetRecords, GetShardIterator, DescribeStream, and ListStreams Actions on your stream
in IAM.

Any advice on how the best way to fix this would be welcome. Adding the provisioner "local-exec" { command = "sleep SECONDS" } on IAM resources seems to help, but I think long term a better strategy needs to be devised and I'm not too sure if this is the best place to be adding it.

Otherwise, if there's anything else that is required to push this PR thru, let me know.

--Chris

@stack72
Copy link
Contributor

stack72 commented Nov 28, 2015

@vancluever this is very nice :) Just ran the tests and go the following:

make testacc TEST=./builtin/providers/aws TESTARGS='-run=EventSourceMapping' 2>~/tf.log
go generate ./...
TF_ACC=1 go test ./builtin/providers/aws -v -run=EventSourceMapping -timeout 90m
=== RUN   TestAccAWSLambdaEventSourceMapping_basic
--- PASS: TestAccAWSLambdaEventSourceMapping_basic (79.25s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    79.265s

Didn't have to worry about the IAM or anything. I am not quite sure about the update func - I'm guessing it would be really good to enable the update for:

  • batch size
  • enabled / not enabled
  • (possibly) function name?


// NOTE: Enabled is omitted here but defaults to true
params := &lambda.CreateEventSourceMappingInput{
FunctionName: aws.String(functionName),
Copy link
Contributor

Choose a reason for hiding this comment

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

There are some go fmt issues in this block.

@vancluever
Copy link
Contributor Author

Thanks for the review Paul! Will be working on your suggestions today. Will fix the fmt issues, I am new to Go and am still getting my tools up and running to help with this kind of thing. Will re-push in a bit.

@vancluever
Copy link
Contributor Author

OK these updates should address the issues - update should now work on all updatable attributes, go fmt is now automatically run on all the code (got my editor tooling all up to snuff), I added the ability to set the mapping to enabled/disabled, and the test has update coverage. Doc has been updated as well.

@stack72
Copy link
Contributor

stack72 commented Nov 29, 2015

@vancluever thanks for submitting the updates - will have a test of these this evening

@stack72
Copy link
Contributor

stack72 commented Nov 29, 2015

@vancluever this is very nice :) Tests pass as expected:

make testacc TEST=./builtin/providers/aws TESTARGS='-run=EventSourceMapping' 2>~/tf.log
go generate ./...
TF_ACC=1 go test ./builtin/providers/aws -v -run=EventSourceMapping -timeout 90m
=== RUN   TestAccAWSLambdaEventSourceMapping_basic
--- PASS: TestAccAWSLambdaEventSourceMapping_basic (93.17s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    93.181s

I think the Hashicorp team will maybe ask for the commits to be squashed but I'll leave that up to them

Paul

@jen20
Copy link
Contributor

jen20 commented Nov 30, 2015

👍 @vancluever this looks great. As @stack72 correctly observes though (!) it would be good if you could squash the commits here so we can maintain as clean a history as possible.

@vancluever
Copy link
Contributor Author

@jen20 roger that, and all done and pushed!

@stack72
Copy link
Contributor

stack72 commented Dec 1, 2015

This looks good - nice work @vancluever :)

@jen20
Copy link
Contributor

jen20 commented Dec 1, 2015

Thanks @vancluever! This looks good to me, I'll go ahead and merge.

jen20 added a commit that referenced this pull request Dec 1, 2015
provider/aws: New resource `aws_lambda_event_source_mapping`
@jen20 jen20 merged commit 9987f36 into hashicorp:master Dec 1, 2015
jen20 added a commit that referenced this pull request Dec 1, 2015
@vancluever
Copy link
Contributor Author

Thanks Paul and James!

--Chris

On Dec 1, 2015, at 5:36 AM, James Nugent notifications@github.com wrote:

Merged #4093.


Reply to this email directly or view it on GitHub.

@ghost
Copy link

ghost commented Apr 29, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants