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.

Sign up for GitHub

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

Jump to bottom

Added cloudwatch log group data source #4167

Merged
merged 3 commits into from
Apr 12, 2018
Merged

Added cloudwatch log group data source #4167

merged 3 commits into from
Apr 12, 2018

Conversation

sjauld
Copy link
Contributor

@sjauld sjauld commented Apr 11, 2018 •
edited
Loading

Ref #3247

% aws-vault exec development -- make testacc TESTARGS="-run TestAccAWSCloudwatchLogGroupDataSource"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run TestAccAWSCloudwatchLogGroupDataSource -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSCloudwatchLogGroupDataSource
--- PASS: TestAccAWSCloudwatchLogGroupDataSource (73.74s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	73.781s

@ghost ghost added the size/M Managed by automation to categorize the size of a PR. label Apr 11, 2018
@sjauld sjauld changed the title added cloudwatch log group data source (#3247) [WIP] added cloudwatch log group data source (#3247) Apr 11, 2018
@sjauld
Copy link
Contributor Author

sjauld commented Apr 11, 2018

Derp forgot to add docs.

@ghost ghost added size/L Managed by automation to categorize the size of a PR. and removed size/M Managed by automation to categorize the size of a PR. labels Apr 11, 2018
@sjauld sjauld changed the title [WIP] added cloudwatch log group data source (#3247) Added cloudwatch log group data source (#3247) Apr 11, 2018
@bflad bflad added new-data-source Introduces a new data source. service/cloudwatchlogs labels Apr 11, 2018
bflad
bflad reviewed Apr 11, 2018
View reviewed changes
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Hi @sjauld thanks for this contribution. Left some initial comments below. Please let me know if you have any questions!

aws/data_source_aws_cloudwatch_log_group.go Outdated
Schema: map[string]*schema.Schema{
"name": {
Type: schema.TypeString,
Optional: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

This field should be Required to match the documentation and desired behavior 😉

aws/data_source_aws_cloudwatch_log_group.go Outdated
}

func dataSourceAwsCloudwatchLogGroupRead(d *schema.ResourceData, meta interface{}) error {
name := aws.String(d.Get("name").(string))
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nitpick: usually its easier to work with the name := d.Get("name").(string) and only wrap it with aws.String(name) when creating the AWS SDK structs so you don't need to dereference the pointer every time. 👍

aws/data_source_aws_cloudwatch_log_group.go Outdated
var logGroup *cloudwatchlogs.LogGroup

for _, lg := range resp.LogGroups {
if *lg.LogGroupName == *name {
Copy link
Contributor

Choose a reason for hiding this comment

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

To prevent potential crashes, we should wrap this with aws.StringValue(lg.LogGroupName) in the unlikely chance that it comes back as nil

aws/data_source_aws_cloudwatch_log_group.go Outdated
LogGroupNamePrefix: name,
}

resp, err := conn.DescribeLogGroups(input)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use the SDK provided paginator to handle cases where over 50 results are returned: https://docs.aws.amazon.com/sdk-for-go/api/service/cloudwatchlogs/#CloudWatchLogs.DescribeLogGroupsPages

@bflad bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Apr 11, 2018
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Apr 11, 2018
@sjauld sjauld changed the title Added cloudwatch log group data source (#3247) Added cloudwatch log group data source Apr 11, 2018
@sjauld
Copy link
Contributor Author

sjauld commented Apr 11, 2018

Thanks for the feedback @bflad!

@sjauld
Copy link
Contributor Author

sjauld commented Apr 11, 2018

Tests rerun.

% aws-vault exec development -- make testacc TESTARGS="-run TestAccAWSCloudwatchLogGroupDataSource"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run TestAccAWSCloudwatchLogGroupDataSource -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSCloudwatchLogGroupDataSource
--- PASS: TestAccAWSCloudwatchLogGroupDataSource (47.90s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	47.947s

bflad
bflad reviewed Apr 12, 2018
View reviewed changes
aws/data_source_aws_cloudwatch_log_group_test.go

var testAccCheckAWSCloudwatchLogGroupDataSourceConfig = fmt.Sprintf(`
resource aws_cloudwatch_log_group "test" {
name = "Test"
Copy link
Contributor

Choose a reason for hiding this comment

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

We should randomize the naming here in case of AWS/testing failures 😄 I will fix this during merge to be %s, make testAccCheckAWSCloudwatchLogGroupDataSourceConfig a function with rName string parameter, and generate the random name via something like rName := acctest.RandomWithPrefix("tf-acc-test")

@bflad bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label Apr 12, 2018
@bflad bflad added this to the v1.15.0 milestone Apr 12, 2018
bflad
bflad approved these changes Apr 12, 2018
View reviewed changes
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Awesome job, LGTM! 🚀

make testacc TEST=./aws TESTARGS='-run=TestAccAWSCloudwatchLogGroupDataSource'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSCloudwatchLogGroupDataSource -timeout 120m
=== RUN   TestAccAWSCloudwatchLogGroupDataSource
--- PASS: TestAccAWSCloudwatchLogGroupDataSource (14.64s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	14.679s

@bflad bflad merged commit 73baf32 into hashicorp:master Apr 12, 2018
bflad added a commit that referenced this pull request Apr 12, 2018
@bflad
Update CHANGELOG for #4167
abc9dbf
@bflad
Copy link
Contributor

bflad commented Apr 18, 2018

This has been released in version 1.15.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

@sjauld sjauld deleted the f/data-source-cloudwatch-log-group branch November 1, 2018 22:15
@ghost
Copy link

ghost commented Apr 2, 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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Apr 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bflad bflad bflad approved these changes

Assignees
No one assigned
Labels
new-data-source Introduces a new data source. size/L Managed by automation to categorize the size of a PR.
Projects
None yet
Milestone
v1.15.0
Development

Successfully merging this pull request may close these issues.
2 participants
@sjauld @bflad