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

New data source: aws_ec2_managed_prefix_list #16738

Merged

bflad merged 2 commits into hashicorp:master from roberth-k:f-d-aws_ec2_managed_prefix_list

Dec 17, 2020

Contributor

roberth-k commented Dec 13, 2020

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Relates #13986

Tests should be expanded when the aws_ec2_managed_prefix_list resource in #14068 has been merged.

Release note for CHANGELOG:

FEATURES:
- **New Data Source:** `aws_ec2_managed_prefix_list`

Output from acceptance testing:

$ make testacc TEST=./aws TESTARGS=-run=TestAccDataSourceAwsEc2ManagedPrefixList
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccDataSourceAwsEc2ManagedPrefixList -timeout 120m
--- PASS: TestAccDataSourceAwsEc2ManagedPrefixList_matchesTooMany (3.60s)
--- PASS: TestAccDataSourceAwsEc2ManagedPrefixList_basic (18.21s)
--- PASS: TestAccDataSourceAwsEc2ManagedPrefixList_filter (18.28s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       19.913s


          d/aws_ec2_managed_prefix_list: new data source

9a33321

roberth-k requested a review from a team as a code owner

December 13, 2020 16:22

ghost added size/XL documentation provider service/ec2 tests labels

github-actions bot added the needs-triage label

This was referenced Dec 13, 2020

d/aws_prefix_list: add support for managed prefix lists #14110

Closed

d/aws_prefix_list: fixes and enhancements #14109

Closed

bflad added new-data-source and removed needs-triage labels

bflad suggested changes

View reviewed changes

Contributor

bflad left a comment

Thank you for splitting this out @roberth-k, just a few things then we should be able to get this in. 👍

aws/data_source_aws_ec2_managed_prefix_list.go Outdated

Comment on lines 10 to 11


		"github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags"

Contributor

bflad Dec 15, 2020

This should fix the importlint error:

Suggested change

      
            	"github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags"
          
            	"github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags"

aws/data_source_aws_ec2_managed_prefix_list.go

+              func dataSourceAwsEc2ManagedPrefixList() *schema.Resource {
+              	return &schema.Resource{
+              		ReadContext: dataSourceAwsEc2ManagedPrefixListRead,

Contributor

bflad Dec 15, 2020

We're not quite ready to switch to the Context versions of the functions everywhere due to some known issues, but this is probably okay.

aws/data_source_aws_ec2_managed_prefix_list.go Outdated

+              	return &schema.Resource{
+              		ReadContext: dataSourceAwsEc2ManagedPrefixListRead,
+              		Schema: map[string]*schema.Schema{
+              			"id": {

Contributor

bflad Dec 15, 2020

Nit: It would be easier to scan these attributes if they were sorted alphabetically and we may eventually enable tfproviderlint XS002

aws/data_source_aws_ec2_managed_prefix_list.go Outdated

Comment on lines 77 to 81

+              	filters, filtersOk := d.GetOk("filter")
+              	input := ec2.DescribeManagedPrefixListsInput{}
+              	if filtersOk {

Contributor

bflad Dec 15, 2020

Nit: Simplification:

Suggested change

      
            	filters, filtersOk := d.GetOk("filter")
          
            	input := ec2.DescribeManagedPrefixListsInput{}
          
            	if filtersOk {
          
            	input := ec2.DescribeManagedPrefixListsInput{}
          
            	if filters, ok := d.GetOk("filter"); ok {

aws/data_source_aws_ec2_managed_prefix_list.go Outdated

+              		input.PrefixListIds = aws.StringSlice([]string{prefixListId.(string)})
+              	}
+              	if prefixListName := d.Get("name"); prefixListName.(string) != "" {

Contributor

bflad Dec 15, 2020

Nit: Simplification:

Suggested change

      
            	if prefixListName := d.Get("name"); prefixListName.(string) != "" {
          
            	if prefixListName, ok := d.GetOk("name"); ok {

aws/data_source_aws_ec2_managed_prefix_list.go Outdated

+              	out, err := conn.DescribeManagedPrefixListsWithContext(ctx, &input)
+              	if err != nil {
+              		return diag.FromErr(err)

Contributor

bflad Dec 15, 2020

Errors should return context for operators and code maintainers. Eventually this should get caught by: #15892

Suggested change

      
            		return diag.FromErr(err)
          
            		return diag.Errorf("error describing EC2 Managed Prefix Lists: %s", err)

aws/data_source_aws_ec2_managed_prefix_list.go Outdated

Comment on lines 124 to 126

+              	entries := &schema.Set{
+              		F: schema.HashResource(dataSourceAwsEc2ManagedPrefixListEntrySchema()),
+              	}

Contributor

bflad Dec 15, 2020

We can bypass schema.Set usage on flattening and just use []interface{} with append() 👍

aws/data_source_aws_ec2_managed_prefix_list.go Outdated

+              	)
+              	if err != nil {
+              		return diag.FromErr(err)

Contributor

bflad Dec 15, 2020

Error context:

Suggested change

      
            		return diag.FromErr(err)
          
            		return diag.Errorf("error getting EC2 Managed Prefix List (%s) Entries: %s", d.Id(), err)

website/docs/d/ec2_managed_prefix_list.html.markdown Outdated

Comment on lines 29 to 44

+              resource "aws_ec2_managed_prefix_list" "example" {
+                name           = "example"
+                max_entries    = 5
+                address_family = "IPv4"
+                entry {
+                  cidr_block = "1.0.0.0/8"
+                }
+              }
+              data "aws_ec2_managed_prefix_list" "example" {
+                filter {
+                  name   = "prefix-list-name"
+                  values = [aws_ec2_managed_prefix_list.example.name]
+                }
+              }

Contributor

bflad Dec 15, 2020

Resource does not exist yet and we prefer not showing the anti-pattern of having a data source reference a managed resource:

Suggested change

      
            resource "aws_ec2_managed_prefix_list" "example" {
          
              name           = "example"
          
              max_entries    = 5
          
              address_family = "IPv4"
          
              entry {
          
                cidr_block = "1.0.0.0/8"
          
              }
          
            }
          
            data "aws_ec2_managed_prefix_list" "example" {
          
              filter {
          
                name   = "prefix-list-name"
          
                values = [aws_ec2_managed_prefix_list.example.name]
          
              }
          
            }
          
            data "aws_ec2_managed_prefix_list" "example" {
          
              filter {
          
                name   = "prefix-list-name"
          
                values = ["example"]
          
              }
          
            }

bflad added this to the v3.22.0 milestone

Contributor

bflad commented Dec 15, 2020

Please note that I've updated the milestone for this pull request to version 3.22.0, which we expect to release on Thursday as our last release before the new year so its not lingering over the long break. If we do not see the updates prior to Wednesday, we will add commits on top of the existing ones to ensure this is tested and merged in time. 👍


          d/aws_ec2_managed_prefix_list: code review updates

roberth-k force-pushed the f-d-aws_ec2_managed_prefix_list branch from b9107da to 9785852 Compare

December 16, 2020 15:16

bflad approved these changes

View reviewed changes

Contributor

bflad left a comment

Looks great, thanks @roberth-k 🚀

Output from acceptance testing in AWS Commercial:

--- PASS: TestAccDataSourceAwsEc2ManagedPrefixList_matchesTooMany (2.82s)
--- PASS: TestAccDataSourceAwsEc2ManagedPrefixList_basic (14.10s)
--- PASS: TestAccDataSourceAwsEc2ManagedPrefixList_filter (15.01s)

Output from acceptance testing in AWS GovCloud (US):

=== CONT  TestAccDataSourceAwsEc2ManagedPrefixList_filter
    data_source_aws_ec2_managed_prefix_list_test.go:100: Step 1/1 error: Error running pre-apply refresh:
        Error: error describing EC2 Managed Prefix Lists: InvalidAction: The action DescribeManagedPrefixLists is not valid for this web service.
        	status code: 400, request id: 7216f4c9-169f-4097-95ac-1bd62806bd30


=== CONT  TestAccDataSourceAwsEc2ManagedPrefixList_basic
    data_source_aws_ec2_managed_prefix_list_test.go:46: Step 1/1 error: Error running pre-apply refresh:
        Error: error describing EC2 Managed Prefix Lists: InvalidAction: The action DescribeManagedPrefixLists is not valid for this web service.
        	status code: 400, request id: bf128192-6ee8-424b-ace3-09349efa30d2


=== CONT  TestAccDataSourceAwsEc2ManagedPrefixList_matchesTooMany
    data_source_aws_ec2_managed_prefix_list_test.go:151: Step 1/1, expected an error with pattern, no match on: Error running pre-apply refresh:
        Error: error describing EC2 Managed Prefix Lists: InvalidAction: The action DescribeManagedPrefixLists is not valid for this web service.
        	status code: 400, request id: e7e70397-1865-4b83-b569-c9800f1bf29f


--- FAIL: TestAccDataSourceAwsEc2ManagedPrefixList_filter (4.08s)
--- FAIL: TestAccDataSourceAwsEc2ManagedPrefixList_basic (4.11s)
--- FAIL: TestAccDataSourceAwsEc2ManagedPrefixList_matchesTooMany (4.11s)

Created followup issue for GovCloud testing: #16804

bflad merged commit ada2d7d into hashicorp:master

bflad added a commit that referenced this pull request


          Update CHANGELOG for #16738

cabe75f

jaredready commented Dec 17, 2020

Is this going to make it in the release today?

ghost commented Dec 18, 2020

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

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

ewbankkit mentioned this pull request

New Resource: data aws_ec2_managed_prefix_lists #17119

Closed

ghost commented Jan 16, 2021

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 locked as resolved and limited conversation to collaborators

roberth-k deleted the f-d-aws_ec2_managed_prefix_list branch

January 16, 2022 20:23

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

documentation new-data-source provider service/ec2 size/XL tests