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

provider/aws: Add `aws_elasticache_replication_groups` resource #8275

Merged
merged 1 commit into from Aug 18, 2016

Conversation

Projects
None yet
6 participants
@stack72
Contributor

stack72 commented Aug 17, 2016

NOTE: This is the first use case for Replication Groups. This will allow us to specify a set of characteristics for a replication group and a number of nodes in the cluster and Terraform will go and create them.

This doesn't include:

  • creating a replication group from an existing cluster
  • adding a new cluster to a replication group

These use cases will follow very shortly!


New PR to rework the ElastiCache Replication Groups work. This means we are now starting to create a common schema between ElastiCache Cluster and Replication Groups

The ElastiCache Cluster tests work as expected still after the Schema has been refactored:

% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSElasticacheCluster_'           ✹
==> Checking that code complies with gofmt requirements...
/Users/stacko/Code/go/bin/stringer
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/08/17 15:20:57 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSElasticacheCluster_ -timeout 120m
=== RUN   TestAccAWSElasticacheCluster_basic
--- PASS: TestAccAWSElasticacheCluster_basic (395.41s)
=== RUN   TestAccAWSElasticacheCluster_snapshotsWithUpdates
--- PASS: TestAccAWSElasticacheCluster_snapshotsWithUpdates (812.77s)
=== RUN   TestAccAWSElasticacheCluster_decreasingCacheNodes
--- PASS: TestAccAWSElasticacheCluster_decreasingCacheNodes (860.08s)
=== RUN   TestAccAWSElasticacheCluster_vpc
--- PASS: TestAccAWSElasticacheCluster_vpc (509.98s)
=== RUN   TestAccAWSElasticacheCluster_multiAZInVpc
--- PASS: TestAccAWSElasticacheCluster_multiAZInVpc (879.67s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    3457.932s
  • Schema (refactor common schema)
  • Resource Specific schema pieces
  • CRUD
  • Acceptance tests
  • Documentation

Final test run for the Replication Groups

% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSElasticacheReplicationGroup_'  ✹
==> Checking that code complies with gofmt requirements...
/Users/stacko/Code/go/bin/stringer
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/08/18 13:16:33 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSElasticacheReplicationGroup_ -timeout 120m
=== RUN   TestAccAWSElasticacheReplicationGroup_basic
--- PASS: TestAccAWSElasticacheReplicationGroup_basic (771.49s)
=== RUN   TestAccAWSElasticacheReplicationGroup_updateDescription
--- PASS: TestAccAWSElasticacheReplicationGroup_updateDescription (892.87s)
=== RUN   TestAccAWSElasticacheReplicationGroup_updateNodeSize
--- PASS: TestAccAWSElasticacheReplicationGroup_updateNodeSize (1541.97s)
=== RUN   TestAccAWSElasticacheReplicationGroup_vpc
--- PASS: TestAccAWSElasticacheReplicationGroup_vpc (756.61s)
=== RUN   TestAccAWSElasticacheReplicationGroup_multiAzInVpc
--- PASS: TestAccAWSElasticacheReplicationGroup_multiAzInVpc (1114.94s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    5077.898s

@stack72 stack72 force-pushed the aws-elasticache-replication_groups branch 5 times, most recently from c9d3e4a to 079ec05 Aug 18, 2016

resourceSchema["number_cache_clusters"] = &schema.Schema{
Type: schema.TypeInt,
Required: true,
ForceNew: true,

This comment has been minimized.

@elblivion

elblivion Aug 18, 2016

Contributor

:-/

You can add read replicas by calling CreateCacheCluster, maybe something for later on: https://docs.aws.amazon.com/AmazonElastiCache/latest/UserGuide/Replication.AddReadReplica.html#Replication.AddReadReplica.API

This comment has been minimized.

@stack72

stack72 Aug 18, 2016

Contributor

This is a limitation of the API - the number is not adjustable via the API. This is a decent first pass :) We will continue to make this resource better!

@stack72 stack72 force-pushed the aws-elasticache-replication_groups branch from 079ec05 to 86f64d0 Aug 18, 2016

@stack72 stack72 changed the title from [WIP] Resource `aws_elasticache_replication_groups` to provider/aws: Add `aws_elasticache_replication_groups` resource Aug 18, 2016

MinTimeout: 3 * time.Second,
Timeout: 40 * time.Minute,
MinTimeout: 10 * time.Second,
Delay: 30 * time.Second,

This comment has been minimized.

@phinze

phinze Aug 18, 2016

Member

@stack72 are these number changes based on observed slowness while developing?

This comment has been minimized.

@stack72

stack72 Aug 18, 2016

Contributor

yessir!

MinTimeout: 3 * time.Second,
Timeout: 80 * time.Minute,
MinTimeout: 10 * time.Second,
Delay: 30 * time.Second,

This comment has been minimized.

@phinze

phinze Aug 18, 2016

Member

Same Q as above - updates can take twice as long as creates? 🙊

This comment has been minimized.

@stack72

stack72 Aug 18, 2016

Contributor

Create:
=== RUN TestAccAWSElasticacheReplicationGroup_basic
--- PASS: TestAccAWSElasticacheReplicationGroup_basic (771.49s)

Update:
=== RUN TestAccAWSElasticacheReplicationGroup_updateNodeSize
--- PASS: TestAccAWSElasticacheReplicationGroup_updateNodeSize (1541.97s)

This comment has been minimized.

@phinze

phinze Aug 18, 2016

Member

🙈

if d.Get("engine").(string) != "redis" {
return fmt.Errorf("The only acceptable Engine type when using Replication Groups is Redis")
}

This comment has been minimized.

@phinze

phinze Aug 18, 2016

Member

Should be able to add this as a ValidateFunc for plan time validation:

resourceSchema["engine"].ValidateFunc =  // ...
availability_zones = ["us-west-2a","us-west-2b"]
automatic_failover_enabled = true
}
```

This comment has been minimized.

@phinze

phinze Aug 18, 2016

Member

Nit: hclfmt the example

@phinze

This comment has been minimized.

Member

phinze commented Aug 18, 2016

Other than the few super-minor nits inline this LGTM!

@stack72 stack72 force-pushed the aws-elasticache-replication_groups branch from 86f64d0 to 47b3c9e Aug 18, 2016

* `replication_group_id` – (Required) The replication group identifier. This parameter is stored as a lowercase string.
* `replication_group_description` – (Required) A user-created description for the replication group.
* `number_cache_clusters` - (Required) The number of cache clusters this replication group will initially have.
If Multi-AZ is enabled , the value of this parameter must be at least 2. Changing this number will force a new resource

This comment has been minimized.

@catsby

catsby Aug 18, 2016

Member

this replication group will initially have ... Changing this number will force a new resource

Let's remove initially here, unless there is some supported way of expanding it

This comment has been minimized.

@stack72

stack72 Aug 18, 2016

Contributor

Totally agree on this!

@stack72 stack72 merged commit 51f2163 into master Aug 18, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stack72 stack72 deleted the aws-elasticache-replication_groups branch Aug 18, 2016

* `number_cache_clusters` - (Required) The number of cache clusters this replication group will initially have.
If Multi-AZ is enabled , the value of this parameter must be at least 2. Changing this number will force a new resource
* `node_type` - (Required) The compute and memory capacity of the nodes in the node group.
* `engine` - (Required) The name of the cache engine to be used for the cache clusters in this replication group. The only valid value is Redis.

This comment has been minimized.

@catsby

catsby Aug 18, 2016

Member

Should we even have this then, if the only value accepted is redis? Seems like needless work for our users

This comment has been minimized.

@stack72

stack72 Aug 18, 2016

Contributor

Agreed - removed

* `parameter_group_name` - (Optional) The name of the parameter group to associate with this replication group. If this argument is omitted, the default cache parameter group for the specified engine is used.
* `subnet_group_name` - (Optional) The name of the cache subnet group to be used for the replication group.
* `security_group_names` - (Optional) A list of cache security group names to associate with this replication group.
* `security_group_ids` - (Optional) One or more Amazon VPC security groups associated with this replication group.

This comment has been minimized.

@catsby

catsby Aug 18, 2016

Member

Any opposition to calling this vpc_security_group_ids to be inline with aws_instance? Can an EC2 Classic user use security_group_ids?

This comment has been minimized.

@stack72

stack72 Aug 18, 2016

Contributor

This comes from the common schema - we call it security_group_ids in ElastiCache Cluster as well

* `engine_version` - (Optional) The version number of the cache engine to be used for the cache clusters in this replication group.
* `parameter_group_name` - (Optional) The name of the parameter group to associate with this replication group. If this argument is omitted, the default cache parameter group for the specified engine is used.
* `subnet_group_name` - (Optional) The name of the cache subnet group to be used for the replication group.
* `security_group_names` - (Optional) A list of cache security group names to associate with this replication group.

This comment has been minimized.

@catsby

catsby Aug 18, 2016

Member

I assume a VPC user cannot use security_group_names, correct? If so we should document that

This comment has been minimized.

@stack72

stack72 Aug 18, 2016

Contributor

Added a note that this should only be used when creating a Replication Group within a VPC

@brmcconn

This comment has been minimized.

brmcconn commented Sep 1, 2016

Any ETA on when this might be available in an upcoming release? I use Terraform for a lot of other AWS resources and it works great. Would love to be able to add ElastiCache to the list, but can't without being able to run replication groups/Redis.

@stack72

This comment has been minimized.

Contributor

stack72 commented Sep 3, 2016

Hi @brmcconn

This has already been released and is available in the 0.7.2 release

P.

@brmcconn

This comment has been minimized.

brmcconn commented Sep 6, 2016

Great, thanks! @stack72. I didn't realize that it was in that release. Just an fyi - the documentation on the website for elasticache hasn't been updated to include the new funcitonality - https://www.terraform.io/docs/providers/aws/index.html#, but I see the documentation posted here which should be plenty to get me up and running!

@stack72

This comment has been minimized.

Contributor

stack72 commented Sep 6, 2016

Hi @brmcconn

The documentation is linked to on the side bar https://www.terraform.io/docs/providers/aws/r/elasticache_replication_group.html - hope this helps

P.

@brmcconn

This comment has been minimized.

brmcconn commented Sep 7, 2016

Thanks for the link @stack72. It all works great. However, that's not showing up on the website for me just going to www.terraform.io > docs > providers > AWS (the URL is https://www.terraform.io/docs/providers/aws/index.html). You can see the missing link for aws_elasticache_replication_group in my screenshot below:
image

@eyalzek

This comment has been minimized.

eyalzek commented Sep 7, 2016

@brmcconn try clearing your browser cache

@brmcconn

This comment has been minimized.

brmcconn commented Sep 7, 2016

@eyalzek , I gave that a try, but no luck. However, if I go to the website on my phone I see the documentation there, so we'll just call this a client issue on my end.

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