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.

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

Aws elasticache cluster import #9010

Merged
merged 4 commits into from
Sep 28, 2016
Merged

Conversation

stack72
Copy link
Contributor

@stack72 stack72 commented Sep 23, 2016

Superceeds #8325

Initial tests were failing as follows:

=== RUN   TestAccAWSElasticacheCluster_importBasic
--- FAIL: TestAccAWSElasticacheCluster_importBasic (362.66s)
        testing.go:265: Step 1 error: ImportStateVerify attributes not
        equivalent. Difference is shown below. Top is actual, bottom is
        expected.

        (map[string]string) {

        }

(map[string]string) (len=2) {
             (string) (len=20) "parameter_group_name": (string) (len=20)
             "default.memcached1.4",
                             (string) (len=22) "security_group_names.#":
                             (string) (len=1) "0"

}

FAIL
exit status 1

The import of ElastiCache clusters helped to point out 3 things:

  1. Currently, we were trying to set the parameter_group_name as follows:
d.Set("parameter_group_name", c.CacheParameterGroup)

Unfortunately, c.CacheParameterGroup is a struct not a string. This was
causing the test import failure. So this had to be replaced as follows:

if c.CacheParameterGroup != nil {
      d.Set("parameter_group_name", c.CacheParameterGroup.CacheParameterGroupName)
}
  1. We were trying to set the security_group_names as follows:
d.Set("security_group_names", c.CacheSecurityGroups)

The CacheSecurityGroups was actually a []* so had to be changed to work
as follows:

if len(c.CacheSecurityGroups) > 0 {
            d.Set("security_group_names",
            flattenElastiCacheSecurityGroupNames(c.CacheSecurityGroups))

}
  1. We were trying to set the security_group_ids as follows:
d.Set("security_group_ids", c.SecurityGroups)

This is another []* and needs to be changed as follows:

if len(c.SecurityGroups) > 0 {
            d.Set("security_group_ids",
            flattenElastiCacheSecurityGroupIds(c.SecurityGroups))

}

This then allows the import test to pass as expected:

% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSElasticacheCluster_importBasic'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/09/23 10:59:01 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v
-run=TestAccAWSElasticacheCluster_importBasic -timeout 120m
=== RUN   TestAccAWSElasticacheCluster_importBasic
--- PASS: TestAccAWSElasticacheCluster_importBasic (351.96s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    351.981s

As a final test, I ran the basic ElastiCache cluster creation to make
sure all passed as expected:

% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSElasticacheCluster_basic'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/09/23 11:05:51 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v
-run=TestAccAWSElasticacheCluster_basic -timeout 120m
=== RUN   TestAccAWSElasticacheCluster_basic
--- PASS: TestAccAWSElasticacheCluster_basic (809.25s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    809.267s

}
if c.CacheParameterGroup != nil {
d.Set("parameter_group_name", c.CacheParameterGroup.CacheParameterGroupName)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be safe to simply do each of these set operations with the results of the respective flatten methods. By only doing so if len > 0, we may fail to detect drift. Ex. we should have a single SG, but an operator inadvertently removed it in the Web UI; TF would fail to detect that

port = 11211
parameter_group_name = "default.memcached1.4"
}
`, acctest.RandString(10))
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: if we ever re-use testAccAWSElasticacheClusterConfigBasic, it will have the same cluster_id as the accent.RandString is only ran on initialization. For future the configs should be methods that receive the random thing and insert it, similar to https://github.com/hashicorp/terraform/blob/master/builtin/providers/aws/resource_aws_elastic_beanstalk_environment_test.go#L88

Initial tests were failing as follows:

```
=== RUN   TestAccAWSElasticacheCluster_importBasic
--- FAIL: TestAccAWSElasticacheCluster_importBasic (362.66s)
        testing.go:265: Step 1 error: ImportStateVerify attributes not
        equivalent. Difference is shown below. Top is actual, bottom is
        expected.

        (map[string]string) {

        }

(map[string]string) (len=2) {
             (string) (len=20) "parameter_group_name": (string) (len=20)
             "default.memcached1.4",
                             (string) (len=22) "security_group_names.#":
                             (string) (len=1) "0"

}

FAIL
exit status 1
```

The import of ElastiCache clusters helped to point out 3 things:

1. Currently, we were trying to set the parameter_group_name as follows:

```
d.Set("parameter_group_name", c.CacheParameterGroup)
```

Unfortunately, c.CacheParameterGroup is a struct not a string. This was
causing the test import failure. So this had to be replaced as follows:

```
if c.CacheParameterGroup != nil {
      d.Set("parameter_group_name", c.CacheParameterGroup.CacheParameterGroupName)
}
```

2. We were trying to set the security_group_names as follows:

```
d.Set("security_group_names", c.CacheSecurityGroups)
```

The CacheSecurityGroups was actually a []* so had to be changed to work
as follows:

```
if len(c.CacheSecurityGroups) > 0 {
            d.Set("security_group_names",
            flattenElastiCacheSecurityGroupNames(c.CacheSecurityGroups))

}
```

3. We were trying to set the security_group_ids as follows:

```
d.Set("security_group_ids", c.SecurityGroups)
```

This is another []* and needs to be changed as follows:

```
if len(c.SecurityGroups) > 0 {
            d.Set("security_group_ids",
            flattenElastiCacheSecurityGroupIds(c.SecurityGroups))

}
```

This then allows the import test to pass as expected:

```
% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSElasticacheCluster_importBasic'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/09/23 10:59:01 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v
-run=TestAccAWSElasticacheCluster_importBasic -timeout 120m
=== RUN   TestAccAWSElasticacheCluster_importBasic
--- PASS: TestAccAWSElasticacheCluster_importBasic (351.96s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    351.981s
```

As a final test, I ran the basic ElastiCache cluster creation to make
sure all passed as expected:

```
% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSElasticacheCluster_basic'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/09/23 11:05:51 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v
-run=TestAccAWSElasticacheCluster_basic -timeout 120m
=== RUN   TestAccAWSElasticacheCluster_basic
--- PASS: TestAccAWSElasticacheCluster_basic (809.25s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    809.267s
```
@stack72 stack72 force-pushed the aws-elasticache-cluster-import branch from b6694bc to b02a5c4 Compare September 28, 2016 11:29
@stack72
Copy link
Contributor Author

stack72 commented Sep 28, 2016

@catsby made the changes as suggested and the tests still work as expected :)

% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSElasticacheCluster_importBasic'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/09/28 12:30:00 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSElasticacheCluster_importBasic -timeout 120m
=== RUN   TestAccAWSElasticacheCluster_importBasic
--- PASS: TestAccAWSElasticacheCluster_importBasic (342.01s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    342.061s
% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSElasticacheCluster_basic'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/09/28 12:36:46 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSElasticacheCluster_basic -timeout 120m
=== RUN   TestAccAWSElasticacheCluster_basic
--- PASS: TestAccAWSElasticacheCluster_basic (469.74s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    469.757s

I'll follow up with some other work in this area to refactor the tests as they are all using the same pattern as you commented on

P.

@stack72 stack72 merged commit b6718de into master Sep 28, 2016
@stack72 stack72 deleted the aws-elasticache-cluster-import branch September 28, 2016 18:49
@ghost
Copy link

ghost commented Apr 21, 2020

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants