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 Data Source: aws_security_groups #2947

Merged
merged 1 commit into from
Jun 27, 2018
Merged

New Data Source: aws_security_groups #2947

merged 1 commit into from
Jun 27, 2018

Conversation

ian-d
Copy link
Contributor

@ian-d ian-d commented Jan 11, 2018

Resolves #2740.

Using the data sources aws_instance/aws_instances naming convention, this creates an aws_security_groups data source for multiple security groups to complement the aws_security_group data source that fetches single security groups.

The example code in the documentation could use an example of creating a resource actually using the data source but I couldn't think of a concise AWS resource that uses a list of security groups that would make much sense in context.

Tests:

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

@radeksimko radeksimko changed the title d/security_groups: Adds aws_security_groups data source. New Data Source: aws_security_groups Jan 17, 2018
@radeksimko radeksimko added new-data-source Introduces a new data source. service/ec2 Issues and PRs that pertain to the ec2 service. labels Jan 17, 2018
@sdot257
Copy link

sdot257 commented Apr 24, 2018

Any idea as to when this would merge? We need this functionality.

I have no experience with Golang, but I was able to build the provider into a binary file. I'm not sure how to actually use it.

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.

Thanks for submitting this @ian-d! Overall it looks really good, just left some minor notes below which I can quickly fix up post-merge. 🚀

log.Printf("[DEBUG] Reading Security Groups with request: %s", req)

var ids, vpc_ids []string
nextToken := ""
Copy link
Contributor

Choose a reason for hiding this comment

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

This new variable and logic surrounding it is extraneous, you can directly map the new resp.NextToken onto req.NextToken, e.g.

for {
  resp, err := conn.DescribeSecurityGroups(req)
  if err != nil {
    return fmt.Errorf("error reading security groups: %s", err)
  }
  for _, sg := range resp.SecurityGroups {
    ids = append(ids, aws.StringValue(sg.GroupId))
    vpc_ids = append(vpc_ids, aws.StringValue(sg.VpcId))
  }
  if resp.NextToken == nil {
    break
  }
  req.NextToken = resp.NextToken
}

@@ -203,6 +203,9 @@
<li<%= sidebar_current("docs-aws-datasource-security-group") %>>
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't expect most folks to know this, but an odd side effect of having the singular and plural data sources is that both entries will be highlighted in the sidebar if you are on the singular data source page since the sidebar_current prefix matches both. Our current convention is to go back to the singular data source and append -x to the sidebar_current in both the sidebar and singular data source page.

@@ -203,6 +203,9 @@
<li<%= sidebar_current("docs-aws-datasource-security-group") %>>
<a href="/docs/providers/aws/d/security_group.html">aws_security_group</a>
</li>
<li<%= sidebar_current("docs-aws-datasource-security-groups") %>>
<a href="/docs/providers/aws/d/security_groups.html">aws_security_group</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo 😎 : missing s in data source name

@bflad bflad added this to the v1.25.0 milestone Jun 27, 2018
@bflad bflad merged commit 6821dba into hashicorp:master Jun 27, 2018
bflad added a commit that referenced this pull request Jun 27, 2018
make testacc TEST=./aws TESTARGS='-run=TestAccDataSourceAwsSecurityGroups'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccDataSourceAwsSecurityGroups -timeout 120m
=== RUN   TestAccDataSourceAwsSecurityGroups_tag
--- PASS: TestAccDataSourceAwsSecurityGroups_tag (31.31s)
=== RUN   TestAccDataSourceAwsSecurityGroups_filter
--- PASS: TestAccDataSourceAwsSecurityGroups_filter (28.44s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	59.788s
bflad added a commit that referenced this pull request Jun 27, 2018
@bflad
Copy link
Contributor

bflad commented Jun 27, 2018

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

@ghost
Copy link

ghost commented Apr 5, 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 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
new-data-source Introduces a new data source. service/ec2 Issues and PRs that pertain to the ec2 service.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws_security_group_ids data source
4 participants