provider/aws: CloudWatch Logs Subscription Filter #4813
Conversation
|
gah, I'll fix tests tomorrow and rebase |
joshmyers
force-pushed
the
cwlogs_subscription
branch
from
26940ac
to
aad9289
Compare
radeksimko
changed the title
joshmyers
force-pushed
the
cwlogs_subscription
branch
from
aad9289
to
c6b0f2b
Compare
|
@radeksimko I think this is ready for review 👍 |
joshmyers
force-pushed
the
cwlogs_subscription
branch
2 times, most recently
from
0f3543e
to
32f67f2
Compare
|
@radeksimko any idea if / when this will get merged? I keep rebasing against master and don't want more merge conflicts. If it helps, we are using it 👍 |
| function_name := lambda_arn_sliced[len(lambda_arn_sliced)-1] | ||
| statement_id := lambdaPermissionStatementId(log_group, destination) | ||
|
|
||
| if !permissionExists(function_name, statement_id, lambda_conn) { |
There was a problem hiding this comment.
I'm not sure it's a good idea to manage some resources "under the hood" and hide those away from the user. I think the user should be fully in control of all resources (and all parameters), where possible.
This is also bringing an unexpected behaviour - imagine a destructive update of the Lambda function (e.g. runtime) you rely on here - destroying the function would also destroy all lambda permissions and it would not trigger recreation of the log subscription here (because the ARN would remain the same).
I will hopefully soon finish #4826 which should solve this problem in cleaner way - i.e. I think we should leave it up to the user to setup the Lambda Permission.
|
Hi @joshmyers I left you some comments there. The most important one is I think management of related resources - Lambda Permissions & Kinesis streams. I'd rather see Lambda Permissions being 1st class citizen (i.e. resource) before we merge this in. Would you mind updating the PR according to those comments? The linked PR works for most simple use-cases, I'm now trying to get around some inconsistency on the API level. See details in the PR. I'd be more than happy for you to help me testing that PR. |
radeksimko
added
the
waiting-response
|
@radeksimko Thanks for the review and comments! I'll try and have a look at this ASAP. |
joshmyers
force-pushed
the
cwlogs_subscription
branch
3 times, most recently
from
302e83b
to
6948b5a
Compare
Fixes issue hashicorp#4985 by correcting copy/paste error that caused the max_utilization attribute to be read from the max_rate_per_instance attribute. N.B. There is still no test coverage for this issue.
joshmyers
force-pushed
the
cwlogs_subscription
branch
from
6948b5a
to
de82990
Compare
|
@joshmyers FYI Lambda Permission is now a 1st class resource in Terraform (as #4826 was merged). Do you think you'll find some time to finish this? Let me know if you need any help or if there's anything unclear in the feedback I provided. |
|
Sorry this has taken me so long @radeksimko This branch has fallen way behind master. Closing this PR and have opened #5996 |
|
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
locked and limited conversation to collaborators
I have a requirement for a cloudwatch_logs_subscription_filter provider ASAP. This is shameless theft of @hngkr PR #3134 which has been closed and no action taken in a while.