New Data Source: aws_elb #2004
Merged
New Data Source: aws_elb #2004
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This is ready for review, sorry for the extra commits. |
|
I'd appreciate if this got merged soon. (Others who agree, please don't post +1, but add a +1 reaction by clicking at the top right of my post.) |
radeksimko
added
the
size/L
|
Can I please get an initial review of this PR? Thanks! |
radeksimko
reviewed
Dec 6, 2017
There was a problem hiding this comment.
Hey @bflad
thanks for the PR.
I left you some comments there, nothing too critical though.
aws/resource_aws_elb.go
Outdated
|
|
||
| // flattenAwsELbResource takes a *elbv2.LoadBalancer and populates all respective resource fields. | ||
| func flattenAwsELbResource(d *schema.ResourceData, meta interface{}, lb *elb.LoadBalancerDescription) error { | ||
| elbconn := meta.(*AWSClient).elbconn |
There was a problem hiding this comment.
Do you mind reducing the interface here to conn *elb.ELB? AFAICT we don't need connections to any other API, so we don't need the whole meta?
There was a problem hiding this comment.
Unfortunately sourceSGIdByName requires ec2conn, but I will switch this to the two clients it needs.
aws/data_source_aws_elb.go
Outdated
| log.Printf("[DEBUG] Reading ELBs: %#v", input) | ||
| resp, err := elbconn.DescribeLoadBalancers(input) | ||
| if err != nil { | ||
| return errwrap.Wrapf("Error retrieving LB: {{err}}", err) |
There was a problem hiding this comment.
errwrap is usually useful in places where we need access to the original error type. This isn't the case IMO. The only place this error will bubble up to is the user where only string-representation of it matters.
Is there a reason for using that over fmt.Errorf()?
There was a problem hiding this comment.
Honestly, just copypasta. I'll fix it up. 👍
aws/data_source_aws_elb_test.go
Outdated
| } | ||
|
|
||
| resource "aws_security_group" "elb_test" { | ||
| name = "allow_all_elb_test" |
There was a problem hiding this comment.
Can you please randomize the name here, so it's safe to run this test multiple times in parallel and leaks don't cause subsequent runs to fail?
aws/data_source_aws_elb_test.go
Outdated
| subnets = ["${aws_subnet.elb_test.*.id}"] | ||
|
|
||
| idle_timeout = 30 | ||
| enable_deletion_protection = false |
There was a problem hiding this comment.
This is an ALB/NLB specific field which needs to be removed. It's also the reason why the test is currently failing:
=== RUN TestAccDataSourceAWSELB_basic
--- FAIL: TestAccDataSourceAWSELB_basic (3.64s)
testing.go:503: Step 0 error: Configuration is invalid.
Warnings: []string(nil)
Errors: []string{"aws_elb.elb_test: : invalid or unknown key: enable_deletion_protection"}
aws/data_source_aws_elb_test.go
Outdated
| } | ||
|
|
||
| tags { | ||
| TestName = "TestAccAWSELB_basic" |
There was a problem hiding this comment.
Nitpick: it may be useful to make these tags in the test config data source specific.
There was a problem hiding this comment.
t.Name() to the rescue!
radeksimko
added
the
waiting-response
* Remove enable_deletion_protection from testAccDataSourceAWSELBConfigBasic * Replace unnecessary errwrap.Wrapf with fmt.Errorf * Reduce flattenAwsELbResource to ec2conn and elbconn instead of meta * Properly name TestAccDataSourceAWSELB_basic resources * Use t.Name() for description and TestName tags
…th new d/aws_elasticache_replication_group
|
Addressed comments and passes data source and resource testing for me! 🚀 |
radeksimko
removed
the
waiting-response
mdlavin
pushed a commit
to mdlavin/terraform-provider-aws
that referenced
this pull request
Dec 9, 2017
* Remove enable_deletion_protection from testAccDataSourceAWSELBConfigBasic * Replace unnecessary errwrap.Wrapf with fmt.Errorf * Reduce flattenAwsELbResource to ec2conn and elbconn instead of meta * Properly name TestAccDataSourceAWSELB_basic resources * Use t.Name() for description and TestName tags
psyvision
added a commit
to psyvision/terraform-provider-aws
that referenced
this pull request
Dec 12, 2017
* Add Data Source: aws_elb * Fix dataSourceAwsElb typo * Fix dataSourceAwsElb Schema name field to not include Computed * Remove dataSourceAwsElb schema defaults for computed fields * Remove dataSourceAwsElb schema defaults for nested computed fields too * Corrected depends_on entry for EIP depends_on = ["aws_internet_gateway.gw"] is the correct syntax * Add sweeper for IAM Server Certificates * test/aws_config_delivery_channel: Add missing dependencies * d/aws_elb r/aws_elb: hashicorp#2004 review comments * Remove enable_deletion_protection from testAccDataSourceAWSELBConfigBasic * Replace unnecessary errwrap.Wrapf with fmt.Errorf * Reduce flattenAwsELbResource to ec2conn and elbconn instead of meta * Properly name TestAccDataSourceAWSELB_basic resources * Use t.Name() for description and TestName tags * d/aws_elb: Fix documentation sidebar ordering after merging master with new d/aws_elasticache_replication_group * Makefile: Add sweep target * Update cognito_user_pool.markdown * Update CHANGELOG.md * r/aws_elasticache_security_group: add import support (hashicorp#2277) * r/aws_elasticache_security_group: add import support * r/aws_elasticache_security_group: hashicorp#2277 review updates * Use d.Id() instead of d.Get("name") on read, which allows using schema.ImportStatePassthrough * d.Set("security_group_names") on read * Set AWS_DEFAULT_REGION to us-east-1 on import testing * Update CHANGELOG.md * documentation: remove antislashes in page titles * Added missing WARN debug lines when reading a non-existing resource * Removed <wbr> from documentation titles * vendor: Bump aws-sdk-go to v.1.12.44 * Add logs for iam server certificate delete conflict (hashicorp#2533) * Query elb API for load balancer arn causing delete conflict - For IAM server certificate. * Use regex for lb name. * Edits for hashicorp#2533 * r/aws_sqs_queue_policy: Support import by queue URL (hashicorp#2544) * Update CHANGELOG.md * r/aws_elasticsearch_domain: Add LogPublishingOption (hashicorp#2285) * WIP * Add enabled * Use cwl policy * Reflect reviews * Update CHANGELOG.md * Add force_destroy field to aws_athena_database (hashicorp#2363) * Add force_destroy field to aws_athena_database. Fixes hashicorp#2362. * Remove unnecessary import. * Code review feedback * Update CHANGELOG.md * Add more example and missing field * New Resource: aws_media_store_container (hashicorp#2448) * New Resource: aws_media_store_container * Reflect reviews * remove policy * Update CHANGELOG.md * Add Redis AUTH, in-transit and at-rest encryption (hashicorp#2090) * add AUTH, at-rest and in-transit encryption to Elasticache replication groups * add _enabled to transit/at_rest encyrption parameters * added one more _enabled * move validateAwsElastiCacheReplicationGroupAuthToken to aws/validators.go, as well as tests * set auth_token to nil during Reads * update Replication Group encryption acceptance tests to use config functions instead of vars * Fix whitespacing (tabs -> spaces) * docs/elasticache_replication_group: Add missing fields * Update CHANGELOG.md * r/aws_dynamodb_table: Ensure ttl is properly read (hashicorp#2452) * r/aws_dynamodb_table: Ensure ttl is properly read * r/aws_dynamodb_table: hashicorp#2452 review updates * Add timeToLiveOutput.TimeToLiveDescription nil check * Simplify logic to d.Set ttl * Update CHANGELOG.md * Bump aws-sdk-go to v.1.12.45 * New Resource: PublicDnsNamespace (hashicorp#2569) * WIP * Add test, docs * Reflect reviews * Modify error handling * Update CHANGELOG.md * New Resource: ServiceDiscovery PrivateDNS Namespace (hashicorp#2589) * New Resource: service_discovery_private_dns_namespace * Reflect reviews * Update CHANGELOG.md
|
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 and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Similar to
aws_lbresource and data source functionality, splitaws_elbresource data management into its own "flatten" method and utilize it in this new data source.Closes #1864