New Data Source: aws_elasticache_replication_group #2124
Conversation
|
atsushi-ishibashi
changed the title
Ninir
added
the
enhancement
|
|
||
| resp, err := conn.DescribeReplicationGroups(input) | ||
| if err != nil { | ||
| return errwrap.Wrapf("Error retrieving Elasticache Replication Group: {{err}}", err) |
There was a problem hiding this comment.
Nitpick: The errwrap IMO isn't necessary in this context as there's no code that would need to know the original error above CRUD. Pretty much all errors from CRUD end up as messages printed in the CLI, so there's not much value in using this over simple fmt.Errorf().
| } | ||
| } | ||
| if rg == nil { | ||
| return fmt.Errorf("[ERROR] Elasticache Replication Group (%s) not found", d.Get("replication_group_id").(string)) |
There was a problem hiding this comment.
Nitpick: It's already clear that this is an error and it will be raised/displayed as such, there's no need to add an explicit [ERROR] here. It would be only valid in a log message.
| } else { | ||
| if rg.NodeGroups == nil { | ||
| d.SetId("") | ||
| return fmt.Errorf("[ERROR] Elasticache Replication Group (%s) doesn't have node groups.", d.Get("replication_group_id").(string)) |
There was a problem hiding this comment.
Nitpick: As above re [ERROR] ^
| number_cache_clusters = 2 | ||
| port = 6379 | ||
| parameter_group_name = "default.redis3.2" | ||
| availability_zones = ["us-east-1a", "us-east-1b"] |
There was a problem hiding this comment.
This is currently failing in our AWS testing account:
=== RUN TestAccDataSourceAwsElasticacheReplicationGroup_basic
--- FAIL: TestAccDataSourceAwsElasticacheReplicationGroup_basic (21.10s)
testing.go:492: Step 0 error: Error applying: 1 error(s) occurred:
* aws_elasticache_replication_group.bar: 1 error(s) occurred:
* aws_elasticache_replication_group.bar: Error creating Elasticache Replication Group: InvalidParameterValue: us-east-1b is not a valid availability zone.
status code: 400, request id: d01f865d-c572-11e7-8067-e323f32a1242
and it's usually not a good practice to hard-code AZs.
Can you please make it reusable by using the AZ data source here? https://www.terraform.io/docs/providers/aws/d/availability_zones.html
radeksimko
added
the
waiting-response
|
@radeksimko You're welcome to review👍 |
radeksimko
removed
the
waiting-response
|
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
#1393