Add rules packages datasource for AWS Inspector #3175

MattiasGees · 2018-01-29T10:30:49Z

This add the rules packages datasource to retrieve all the available AWS inspector rules packages for your region.

This is my first time creating a full data source for Terraform. All feedback is welcome on both code and test coverage.

I am also not sure about the name of the datasource. At this moment it is rules_packages but maybe inspector_rules_packages is a better name?

This add the rules packages datasource to retrieve all the available AWS inspector rules packages for your region

bflad · 2018-02-05T20:40:22Z

Since AWS is very segregated between services and the APIs, we would definitely recommend calling the data source something with the service name in it and would require that change on review so its not confused with many of the other services that offer "rules" like EC2 security groups. So in this case dataSourceAwsInspectorRulePackages and aws_inspector_rule_packages would be appropriate.

I'll provide a full review shortly.

bflad

@MattiasGees thanks for your contribution! Check out the notes below and let me know if you have any questions.

This will be pretty straightforward to enhance in the future if we want to by name, provider, etc. filtering with DescribeRulesPackages on the ARNs.

bflad · 2018-02-05T20:41:27Z

aws/data_source_aws_rules_packages.go

+	"github.com/hashicorp/terraform/helper/schema"
+)
+
+func dataSourceAwsRulesPackages() *schema.Resource {


Please change to dataSourceAwsInspectorRulesPackages() and update the Read function accordingly as well. 👍

bflad · 2018-02-05T20:54:54Z

aws/data_source_aws_rules_packages.go

+	log.Printf("[DEBUG] Reading Rules Packages.")
+	d.SetId(time.Now().UTC().String())
+
+	var results int64 = 300


We should probably use the SDK paginator instead of limiting the results or potentially missing some due to not checking NextToken.

var arns []string input := &inspector.ListRulesPackagesInput{} err := conn.ListRulesPackagesPages(input, func(page *ListRulesPackagesOutput, lastPage bool) bool { for _, arn := range page.RulesPackageArns { arns = append(arns, *arn) } return !lastPage }) if err != nil { return fmt.Errorf("Error fetching Rules Packages: %s", err) } if len(arns) == 0 { return errors.New("No rules packages found.") } sort.Strings(arns) d.Set("arns", arns)

bflad · 2018-02-05T20:56:14Z

aws/provider.go

@@ -214,6 +214,7 @@ func Provider() terraform.ResourceProvider {
 			"aws_region":                           dataSourceAwsRegion(),
 			"aws_route_table":                      dataSourceAwsRouteTable(),
 			"aws_route53_zone":                     dataSourceAwsRoute53Zone(),
+			"aws_rules_packages":                   dataSourceAwsRulesPackages(),


Please update to aws_inspector_rules_packages 👍

bflad · 2018-02-05T20:56:56Z

aws/data_source_aws_rules_packages_test.go

+	return func(s *terraform.State) error {
+		rs, ok := s.RootModule().Resources[n]
+		if !ok {
+			return fmt.Errorf("Can't find Tules Packages resource: %s", n)


Nitpick: typo

bflad · 2018-02-05T20:59:44Z

aws/data_source_aws_rules_packages_test.go

+				Config: testAccCheckAwsRulesPackagesConfig,
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckAwsRulesPackagesMeta("data.aws_rules_packages.aws_rules_packages"),
+				),


We can check attributes here with some of the resource helpers instead of more complicated acceptance testing logic in another function: e.g. resource.TestCheckResourceAttr("data.aws_inspector_rules_packages.test", "arns.#", "4"), or if indeterminate resource.TestCheckResourceAttrSet("data.aws_inspector_rules_packages.test", "arns.#")

bflad · 2018-02-05T21:01:15Z

aws/data_source_aws_rules_packages_test.go

+			return fmt.Errorf("Rules Packages resource ID not set.")
+		}
+
+		actual, err := testAccCheckAwsRulesPackagesBuildAvailable(rs.Primary.Attributes)


We don't need this function or its call, see above.

bflad · 2018-02-05T21:02:57Z

aws/data_source_aws_rules_packages_test.go

+			return err
+		}
+
+		expected := actual


I don't think we don't need to acceptance test sort.Strings().

This changes the name of the function to aws_inspector_rules_packages. Also a change to the pagination was made + simplified the tests

MattiasGees · 2018-02-12T07:55:35Z

@bflad I made the requested changes.

bflad

You're pretty close! 😄 Let us know if you don't have time to address the last items.

We definitely need to address the test failure and add documentation:

Add data source documentation in website/docs/d/inspector_rules_packages.html.markdown
Add link in data sources sidebar section of website/aws.erb e.g.

                        <li<%= sidebar_current("docs-aws-datasource-inspector-rules-packages") %>>
                          <a href="/docs/providers/aws/d/inspector_rules_packages.html">aws_inspector_rules_packages</a>
                        </li>

bflad · 2018-02-12T11:17:45Z

aws/data_source_aws_inspector_rules_packages_test.go

+		Steps: []resource.TestStep{
+			{
+				Config: testAccCheckAWSInspectorRulesPackagesConfig,
+				Check:  resource.TestCheckResourceAttrSet("data.aws_inspector_rules_packages.test", "arns.#"),


The .test here needs to either be updated to .rules_packages to match the name in testAccCheckAWSInspectorRulesPackagesConfig or the data source to be named test in the configuration

make testacc TEST=./aws TESTARGS='-run=TestAccAWSInspectorRulesPackages_basic' ==> Checking that code complies with gofmt requirements... TF_ACC=1 go test ./aws -v -run=TestAccAWSInspectorRulesPackages_basic -timeout 120m === RUN TestAccAWSInspectorRulesPackages_basic --- FAIL: TestAccAWSInspectorRulesPackages_basic (7.84s) testing.go:513: Step 0 error: Check failed: Not found: data.aws_inspector_rules_packages.test in [root] FAIL exit status 1 FAIL github.com/terraform-providers/terraform-provider-aws/aws 7.888s make: *** [testacc] Error 1

I renamed it to test

bflad · 2018-02-12T11:18:54Z

aws/provider.go

@@ -214,6 +214,7 @@ func Provider() terraform.ResourceProvider {
 			"aws_region":                           dataSourceAwsRegion(),
 			"aws_route_table":                      dataSourceAwsRouteTable(),
 			"aws_route53_zone":                     dataSourceAwsRoute53Zone(),
+			"aws_inspector_rules_packages":         dataSourceAwsInspectorRulesPackages(),


Minor nitpick: Can you lexicographically sort this please?

bflad

This looks great! Thanks for the contribution! 🚀

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

bflad · 2018-02-27T12:40:02Z

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

ghost · 2020-04-07T17:55:40Z

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!

MattiasGees added 2 commits January 29, 2018 11:17

Add rules packages datasource

85370aa

This add the rules packages datasource to retrieve all the available AWS inspector rules packages for your region

Fix little bug

5f8b98d

ghost added the size/L Managed by automation to categorize the size of a PR. label Jan 29, 2018

radeksimko added service/inspector Issues and PRs that pertain to the inspector service. new-data-source Introduces a new data source. labels Jan 29, 2018

bflad suggested changes Feb 5, 2018

View reviewed changes

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Feb 6, 2018

Modify code with PR comments

02835c9

This changes the name of the function to aws_inspector_rules_packages. Also a change to the pagination was made + simplified the tests

ghost added size/M Managed by automation to categorize the size of a PR. and removed size/L Managed by automation to categorize the size of a PR. labels Feb 12, 2018

bflad self-requested a review February 12, 2018 11:09

bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label Feb 12, 2018

bflad added this to the v1.10.0 milestone Feb 12, 2018

bflad suggested changes Feb 12, 2018

View reviewed changes

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Feb 12, 2018

Add docs

51063cd

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 Feb 12, 2018

MattiasGees added 2 commits February 12, 2018 20:49

Change order

907333e

Rename test datasource

f762880

bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label Feb 12, 2018

bflad approved these changes Feb 12, 2018

View reviewed changes

bflad merged commit fb9ae06 into hashicorp:master Feb 12, 2018

bflad added a commit that referenced this pull request Feb 12, 2018

Update CHANGELOG for #3175

8122967

MattiasGees deleted the feature/rules-packages branch February 13, 2018 07:31

ghost locked and limited conversation to collaborators Apr 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rules packages datasource for AWS Inspector #3175

Add rules packages datasource for AWS Inspector #3175

MattiasGees commented Jan 29, 2018 •

edited

Loading

bflad commented Feb 5, 2018

bflad left a comment

bflad Feb 5, 2018

bflad Feb 5, 2018

bflad Feb 5, 2018

bflad Feb 5, 2018

bflad Feb 5, 2018 •

edited

Loading

bflad Feb 5, 2018

bflad Feb 5, 2018

MattiasGees commented Feb 12, 2018

bflad left a comment

bflad Feb 12, 2018

MattiasGees Feb 12, 2018

bflad Feb 12, 2018

bflad left a comment

bflad commented Feb 27, 2018

ghost commented Apr 7, 2020

Add rules packages datasource for AWS Inspector #3175

Add rules packages datasource for AWS Inspector #3175

Conversation

MattiasGees commented Jan 29, 2018 • edited Loading

bflad commented Feb 5, 2018

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad Feb 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MattiasGees commented Feb 12, 2018

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

bflad commented Feb 27, 2018

ghost commented Apr 7, 2020

MattiasGees commented Jan 29, 2018 •

edited

Loading

bflad Feb 5, 2018 •

edited

Loading