Add AWS Log Lambda Integration #436
Conversation
ghost
added
|
@bkabrda would you happen to know who can help push this along? |
|
Hey @mhaley-miovision 👋. First of all, thanks for opening this PR, it looks really good! I think overall the approach makes sense. I'd propose adding the One more think I'd like to ask: we recently started recording the interactions with the DD API using "cassettes" to be able to run all tests fast and without internet connection - more details on that in [1]. Could you please rebase on latest master and generate cassette for the test you created with make cassettes TESTARGS="-run TestAccountAndLambdaArnFromID"? (Do note that this will actually interact with the DD API much like acceptance tests, so make sure you don't provide api/app keys for a production org). Alternatively, if you don't/can't do this, I'd be happy to add a commit to this PR updating the cassettes, assuming you give me access to your branch from which you did this PR. [1] https://github.com/terraform-providers/terraform-provider-datadog#developing-the-provider |
|
Hey @bkabrda I would be happy to give you access to the branch for you to create the cassettes. I sent you an invite now. As far as adding services to this resource, I don't think that would be a good idea. Services are a per-account resource and you could potentially have multiple lambdas per account. They are separate API calls so I think they should be separate resources. If you look here https://github.com/terraform-providers/terraform-provider-datadog/pull/437 I have added the ability to manage services through a different resource. |
|
@mhaley-miovision that's a great point about services, I agree that what you propose makes a lot of sense, so let's go with that. I'll push the cassette to your branch and then do a detailed review of the PR, I think there are perhaps some minor improvements that we might consider doing in the added docs. |
There was a problem hiding this comment.
So the code looks really good, I have no comments on that side. I did some suggestions for the format of some error strings and also for docs.
One thing I noticed is that you didn't add tests that would actually try to create/delete the resource - could you please work on that? I'll manage recording the cassette for it once that's done (assuming you don't have an account to use while creating the test, please let me know and I'll try to find some time to create it). Thanks!
Fix up comments and error strings. Co-Authored-By: Slavek Kabrda <[email protected]>
|
Hey @bkabrda, I got the changes you suggested merged in as well as getting an acceptance test made. I did not see any for the AWS integration which is why I did not add one before. I made one for the lambda arn and also added one for the aws integration itself. Let me know what you think. I am working on adding the cassettes now. |
|
@bkabrda I have finished up with the tests and cassettes now and also added them to the other PR. Let me know if there is anything else you would like to see! |
|
Hello? I see other action in this repository, is there something else I can do to help get this in? A response would be appreciated. |
|
Hi, sorry for the delay. We are looking at this PR and we are trying to make sure with the proper team that this implementation is the right way to do this resource moving forward. We will come back to you very very soon. |
|
@gzussa thanks for the update. |
There was a problem hiding this comment.
Great PR 💯. My feedback for this PR is really the same as the one I made in https://github.com/terraform-providers/terraform-provider-datadog/pull/437
- Use of
ProviderConfiguration. - Use of
translateClientErrormethod. - Using Helper methods in tests.
- Creating a build...Struct method.
After these changes in, we should be good to merge.
| accountID := d.Get("account_id").(string) | ||
| lambdaArn := d.Get("lambda_arn").(string) | ||
|
|
||
| attachLambdaArnRequest := datadog.IntegrationAWSLambdaARNRequest{ | ||
| AccountID: &accountID, | ||
| LambdaARN: &lambdaArn, | ||
| } |
There was a problem hiding this comment.
Let's create a buildDatadogIntegrationAwsLambdaArnStruct(d *schema.ResourceData) *datadog.IntegrationAWSLambdaARNRequest method here
|
Hey Gregory, thanks for taking a look, I appreciate it. I will get to these first thing Monday. |
Taking a stab at https://github.com/terraform-providers/terraform-provider-datadog/issues/318. Let me know if we are on the right track and what other things we would like to see. This just adds the lambda arn to the integration, it does not manage any services. I was going to add service management after we work through if this is the right path forward and if there was anything I missed.
I have tested this against my own datadog account and it works as expected.