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

provider/aws: aws_cloudwatch_log_stream resource #8626

Merged
merged 1 commit into from
Sep 3, 2016

Conversation

stack72
Copy link
Contributor

@stack72 stack72 commented Sep 2, 2016

This is a requirement for enabling CloudWatch Logging on Kinesis Firehose

% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSCloudWatchLogStream_'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/09/02 16:19:14 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v
-run=TestAccAWSCloudWatchLogStream_ -timeout 120m
=== RUN   TestAccAWSCloudWatchLogStream_basic
--- PASS: TestAccAWSCloudWatchLogStream_basic (22.31s)
=== RUN   TestAccAWSCloudWatchLogStream_disappears
--- PASS: TestAccAWSCloudWatchLogStream_disappears (21.21s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    43.538s

@apparentlymart
Copy link
Contributor

Hmm... it feels weird to me for Terraform to make log streams. The design for Cloudwatch Logs is that each distinct writer makes its own stream, because each stream has a single "cursor" that is used for writing and only the client that wrote the most recent record has the cursor token for the next write.

Thus I think it makes sense for Terraform to make log groups, but individual log emitters (whether they be EC2 instances, Lambda functions, Kinesis Firehose streams, or whatever) should create one more more log streams themselves, on the fly.

In the case of Lambda and Kinesis Firehose, you don't even get to create the log group... they just make their own based on a predictable naming scheme.

@stack72 stack72 changed the title provider/aws: aws_cloudwatch_log_stream resource [WIP] provider/aws: aws_cloudwatch_log_stream resource Sep 2, 2016
@radeksimko
Copy link
Member

I think I know where @apparentlymart is coming from and it may does feel a bit weird based on how many AWS services deal with log streams (the services just manage those directly), but I don't see why we shouldn't allow the user to manage these themselves.

The design for Cloudwatch Logs is that each distinct writer makes its own stream

I'd argue this is design of many AWS services (including Lambda and API Gateway), but not necessarily CloudWatch itself.

CloudFormation does support this alongside CloudWatch Log Destinations.
I'm assuming there was a reason why Amazon CloudFormation engineers spent time on implementing these (?). 😉

@stack72 stack72 changed the title [WIP] provider/aws: aws_cloudwatch_log_stream resource provider/aws: aws_cloudwatch_log_stream resource Sep 3, 2016
"regexp"

"github.com/hashicorp/terraform/helper/schema"

Copy link
Contributor

Choose a reason for hiding this comment

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

Import formatting is screwed up

@jen20
Copy link
Contributor

jen20 commented Sep 3, 2016

Few minor nits but otherwise this LGTM. Looking at the design of it we do need to support this to allow people to configure CloudWatch/Firehose - and while I agree with @apparentlymart that it would be a nicer design for writers to create their own streams it doesn't seem to be the case, so I think we'll have to follow CloudFormation's lead here as pointed out by @radeksimko.

This is a requirement for enabling CloudWatch Logging on Kinesis
Firehost

% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSCloudWatchLogStream_'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/09/02 16:19:14 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v
-run=TestAccAWSCloudWatchLogStream_ -timeout 120m
=== RUN   TestAccAWSCloudWatchLogStream_basic
--- PASS: TestAccAWSCloudWatchLogStream_basic (22.31s)
=== RUN   TestAccAWSCloudWatchLogStream_disappears
--- PASS: TestAccAWSCloudWatchLogStream_disappears (21.21s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    43.538s
@stack72 stack72 force-pushed the aws-cloudwatch-log-stream branch from 062fb7e to 49b8568 Compare September 3, 2016 21:26
@stack72 stack72 merged commit ef33024 into master Sep 3, 2016
@stack72 stack72 deleted the aws-cloudwatch-log-stream branch September 3, 2016 21:31
@apparentlymart
Copy link
Contributor

I'm certainly fine with this... was just describing how I've observed CloudWatch behave in case it was rooted in a misunderstanding of how something works. Certainly no harm in having this here, as long as it doesn't cause confusion for other people trying to figure out how to use Cloudwatch Logs and thinking that they always need to create these things with Terraform (since for many services they would not need to).

@ghost
Copy link

ghost commented Apr 22, 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 22, 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.

4 participants