Skip to content
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

Issue #459 - EBS information required during cluster config change #1131

Merged
merged 10 commits into from
Jul 31, 2017
Merged

Issue #459 - EBS information required during cluster config change #1131

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b0a8352
Issue #459 - EBS information required during cluster configuration ch…
Jul 12, 2017
40a82fe
fix formatting
Jul 12, 2017
7071e77
add test for #459
Jul 26, 2017
268a455
fix and adjust testing method
Jul 31, 2017
eaa9795
fix function name for as an entrypoint for Config
Jul 31, 2017
d94f173
test: Randomize name of the cluster
radeksimko Jul 31, 2017
b24b01a
test: Use even number of data nodes
radeksimko Jul 31, 2017
53ccd7c
test: Specify valid snapshot hours
radeksimko Jul 31, 2017
700bb88
test: Fix test functions
radeksimko Jul 31, 2017
9a41771
Fix tests
radeksimko Jul 31, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 12 additions & 11 deletions aws/resource_aws_elasticsearch_domain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,10 +358,10 @@ func resourceAwsElasticSearchDomainUpdate(d *schema.ResourceData, meta interface

if err := setTagsElasticsearchService(conn, d, d.Id()); err != nil {
return err
} else {
d.SetPartial("tags")
}

d.SetPartial("tags")

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What exactly is the reason for this change?

Copy link
Contributor Author

@dsalbert dsalbert Jul 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason is purely cosmetic. In case where there is an error, function will return from function so there is no need to keep else. From other side, by using 'else' we can show that there is a relation between setTagsElasticsearchService and d.SetPartial("tags"), so if there is any relation between them maybe we should keep else :>

btw. In documentation there is no else block -https://www.terraform.io/docs/plugins/provider.html

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case where there is an error, function will return from function so there is no need to keep else

Ah, you're right, good point. Early return FTW 👍

input := elasticsearch.UpdateElasticsearchDomainConfigInput{
DomainName: aws.String(d.Get("domain_name").(string)),
}
Expand All @@ -374,7 +374,7 @@ func resourceAwsElasticSearchDomainUpdate(d *schema.ResourceData, meta interface
input.AdvancedOptions = stringMapToPointers(d.Get("advanced_options").(map[string]interface{}))
}

if d.HasChange("ebs_options") {
if d.HasChange("ebs_options") || d.HasChange("cluster_config") {
options := d.Get("ebs_options").([]interface{})

if len(options) > 1 {
Expand All @@ -383,17 +383,18 @@ func resourceAwsElasticSearchDomainUpdate(d *schema.ResourceData, meta interface
s := options[0].(map[string]interface{})
input.EBSOptions = expandESEBSOptions(s)
}
}

if d.HasChange("cluster_config") {
config := d.Get("cluster_config").([]interface{})
if d.HasChange("cluster_config") {
config := d.Get("cluster_config").([]interface{})

if len(config) > 1 {
return fmt.Errorf("Only a single cluster_config block is expected")
} else if len(config) == 1 {
m := config[0].(map[string]interface{})
input.ElasticsearchClusterConfig = expandESClusterConfig(m)
if len(config) > 1 {
return fmt.Errorf("Only a single cluster_config block is expected")
} else if len(config) == 1 {
m := config[0].(map[string]interface{})
input.ElasticsearchClusterConfig = expandESClusterConfig(m)
}
}

}

if d.HasChange("snapshot_options") {
Expand Down
76 changes: 76 additions & 0 deletions aws/resource_aws_elasticsearch_domain_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,54 @@ func TestAccAWSElasticSearchDomain_tags(t *testing.T) {
})
}

func TestAccAWSElasticSearchDomain_update(t *testing.T) {
var input elasticsearch.ElasticsearchDomainStatus
ri := acctest.RandInt()

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckESDomainDestroy,
Steps: []resource.TestStep{
{
Config: testAccESDomainConfig_ClusterUpdate(ri, 2, 22),
Check: resource.ComposeTestCheckFunc(
testAccCheckESDomainExists("aws_elasticsearch_domain.example", &input),
testAccCheckESNumberOfInstances(2, &input),
testAccCheckESSnapshotHour(22, &input),
),
},
{
Config: testAccESDomainConfig_ClusterUpdate(ri, 4, 23),
Check: resource.ComposeTestCheckFunc(
testAccCheckESDomainExists("aws_elasticsearch_domain.example", &input),
testAccCheckESNumberOfInstances(4, &input),
testAccCheckESSnapshotHour(23, &input),
),
},
}})
}

func testAccCheckESSnapshotHour(snapshotHour int, status *elasticsearch.ElasticsearchDomainStatus) resource.TestCheckFunc {
return func(s *terraform.State) error {
conf := status.SnapshotOptions
if *conf.AutomatedSnapshotStartHour != int64(snapshotHour) {
return fmt.Errorf("Snapshots start hour differ. Given: %d, Expected: %d", *conf.AutomatedSnapshotStartHour, snapshotHour)
}
return nil
}
}

func testAccCheckESNumberOfInstances(numberOfInstances int, status *elasticsearch.ElasticsearchDomainStatus) resource.TestCheckFunc {
return func(s *terraform.State) error {
conf := status.ElasticsearchClusterConfig
if *conf.InstanceCount != int64(numberOfInstances) {
return fmt.Errorf("Number of instances differ. Given: %d, Expected: %d", *conf.InstanceCount, numberOfInstances)
}
return nil
}
}

func testAccLoadESTags(conf *elasticsearch.ElasticsearchDomainStatus, td *elasticsearch.ListTagsOutput) resource.TestCheckFunc {
return func(s *terraform.State) error {
conn := testAccProvider.Meta().(*AWSClient).esconn
Expand Down Expand Up @@ -224,6 +272,34 @@ resource "aws_elasticsearch_domain" "example" {
`, randInt)
}

func testAccESDomainConfig_ClusterUpdate(randInt, instanceInt, snapshotInt int) string {
return fmt.Sprintf(`
resource "aws_elasticsearch_domain" "example" {
domain_name = "tf-test-%d"

advanced_options {
"indices.fielddata.cache.size" = 80
}

ebs_options {
ebs_enabled = true
volume_size = 10

}

cluster_config {
instance_count = %d
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be certainly static number, we don't want to be spinning up an unbound random number of instances 💸 😃

Randomization is however certainly desired, but only in places that are unique, like domain_name see other tests in this file.

Copy link
Contributor Author

@dsalbert dsalbert Jul 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I've also changed instance type to t2.micro.elasticsearch.

zone_awareness_enabled = true
instance_type = "t2.micro.elasticsearch"
}

snapshot_options {
automated_snapshot_start_hour = %d
}
}
`, randInt, instanceInt, snapshotInt)
}

func testAccESDomainConfig_TagUpdate(randInt int) string {
return fmt.Sprintf(`
resource "aws_elasticsearch_domain" "example" {
Expand Down