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

provider/aws: Add some tests for the Import for aws_eip #9009

Merged
merged 1 commit into from
Oct 7, 2016

Conversation

stack72
Copy link
Contributor

@stack72 stack72 commented Sep 23, 2016

% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSEIP_'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/09/23 09:28:32 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSEIP_ -timeout
120m
=== RUN   TestAccAWSEIP_importEc2Classic
--- PASS: TestAccAWSEIP_importEc2Classic (116.16s)
=== RUN   TestAccAWSEIP_importVpc
--- PASS: TestAccAWSEIP_importVpc (61.89s)
=== RUN   TestAccAWSEIP_basic
--- PASS: TestAccAWSEIP_basic (18.86s)
=== RUN   TestAccAWSEIP_instance
--- PASS: TestAccAWSEIP_instance (185.95s)
=== RUN   TestAccAWSEIP_network_interface
--- PASS: TestAccAWSEIP_network_interface (63.20s)
=== RUN   TestAccAWSEIP_twoEIPsOneNetworkInterface
--- PASS: TestAccAWSEIP_twoEIPsOneNetworkInterface (65.64s)
=== RUN   TestAccAWSEIP_associated_user_private_ip
--- PASS: TestAccAWSEIP_associated_user_private_ip (201.34s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    713.072s

ImportState: true,
ImportStateVerify: true,
ImportStateVerifyIgnore: []string{
"vpc",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious why we're ignoring the import of vpc, I believe it's setting has an impact to how the resource behaves.

@stack72 stack72 changed the title provider/aws: Add some tests for the Import for aws_eip [WIP] provider/aws: Add some tests for the Import for aws_eip Sep 30, 2016
@stack72
Copy link
Contributor Author

stack72 commented Sep 30, 2016

@catsby for the purposes of import, the ignoring of vpc isn't important. When we import, we determine if we are in a VPC using the domain parameter. Setting of the vpc to the bool just happens at the end of the func

this is the actual code that does the importing:

req := &ec2.DescribeAddressesInput{}

    if domain == "vpc" {
        req.AllocationIds = []*string{aws.String(id)}
    } else {
        req.PublicIps = []*string{aws.String(id)}
    }

vpc is just something we use internally - it is not exposed from the API

@stack72 stack72 changed the title [WIP] provider/aws: Add some tests for the Import for aws_eip provider/aws: Add some tests for the Import for aws_eip Sep 30, 2016
@catsby
Copy link
Contributor

catsby commented Sep 30, 2016

vpc is just something we use internally - it is not exposed from the API

which is why I was questioning why we omit it in the import; it can possibly affect how the resource behaves (by way of the domain attribute), so I feared that not caring how it gets imported could have unintended consequences.

I tried this out, and the domain attribute seems to be getting set correctly, which I believe is ultimately the thing that controls the behavior of aws_eip, so that part seems OK.

That said, I applied the below config and then removed and imported the aws_eip, and got a diff on the next plan:

#config 
resource "aws_instance" "foo" {
  # us-west-2
  ami = "ami-4fccb37f"
  # us-east-1
  #ami = "ami-8c6ea9e4"

  instance_type = "m1.small"
}

resource "aws_eip" "bar" {
  instance = "${aws_instance.foo.id}"
}

import works fine, but a follow up plan shows a diff:

~ aws_eip.bar
    vpc: "true" => "false"

vpc is getting set to true on import (correctly, I think), however vpc being an Optional field in the schema, we're now picking up a diff because it's not specified in the config. This does not seem to be a bug in the import itself, but I think that by ignoring the vpc here in the import we've unintentionally covered up a bug with import (or the schema, I suppose).

Thoughts?

(fwiw the above config works as expected in classic 👌)

@stack72 stack72 changed the title provider/aws: Add some tests for the Import for aws_eip [WIP] provider/aws: Add some tests for the Import for aws_eip Oct 3, 2016
@stack72 stack72 changed the title [WIP] provider/aws: Add some tests for the Import for aws_eip provider/aws: Add some tests for the Import for aws_eip Oct 7, 2016
@stack72
Copy link
Contributor Author

stack72 commented Oct 7, 2016

% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSEIP_'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/10/07 12:32:10 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSEIP_ -timeout 120m
=== RUN   TestAccAWSEIP_importEc2Classic
--- PASS: TestAccAWSEIP_importEc2Classic (113.29s)
=== RUN   TestAccAWSEIP_importVpc
--- PASS: TestAccAWSEIP_importVpc (71.80s)
=== RUN   TestAccAWSEIP_basic
--- PASS: TestAccAWSEIP_basic (21.25s)
=== RUN   TestAccAWSEIP_instance
--- PASS: TestAccAWSEIP_instance (198.77s)
=== RUN   TestAccAWSEIP_network_interface
--- PASS: TestAccAWSEIP_network_interface (71.90s)
=== RUN   TestAccAWSEIP_twoEIPsOneNetworkInterface
--- PASS: TestAccAWSEIP_twoEIPsOneNetworkInterface (75.54s)
=== RUN   TestAccAWSEIP_associated_user_private_ip
2016/10/07 12:44:30 Request body type has been overwritten. May cause race conditions
2016/10/07 12:44:34 Request body type has been overwritten. May cause race conditions
2016/10/07 12:44:36 Request body type has been overwritten. May cause race conditions
2016/10/07 12:44:39 Request body type has been overwritten. May cause race conditions
2016/10/07 12:44:43 Request body type has been overwritten. May cause race conditions
--- PASS: TestAccAWSEIP_associated_user_private_ip (209.33s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    761.909s

The Read func of the EIP has changed to set the `vpc` boolean value on
the response object having an Address. This is required as an EIP that
was specified, without a domain and then imported, would cause a
perpetual plan.

```
% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSEIP_'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/09/23 09:28:32 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSEIP_ -timeout
120m
=== RUN   TestAccAWSEIP_importEc2Classic
--- PASS: TestAccAWSEIP_importEc2Classic (116.16s)
=== RUN   TestAccAWSEIP_importVpc
--- PASS: TestAccAWSEIP_importVpc (61.89s)
=== RUN   TestAccAWSEIP_basic
--- PASS: TestAccAWSEIP_basic (18.86s)
=== RUN   TestAccAWSEIP_instance
--- PASS: TestAccAWSEIP_instance (185.95s)
=== RUN   TestAccAWSEIP_network_interface
--- PASS: TestAccAWSEIP_network_interface (63.20s)
=== RUN   TestAccAWSEIP_twoEIPsOneNetworkInterface
--- PASS: TestAccAWSEIP_twoEIPsOneNetworkInterface (65.64s)
=== RUN   TestAccAWSEIP_associated_user_private_ip
--- PASS: TestAccAWSEIP_associated_user_private_ip (201.34s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    713.072s
```
Copy link
Contributor

@catsby catsby left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@stack72 stack72 merged commit 181fd25 into master Oct 7, 2016
@stack72 stack72 deleted the tests-aws-import-eip branch October 7, 2016 15:25
@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.

2 participants