resource/aws_instance: Add computed field for volume_id of block device

[rowleyaj]

Should resolve #1485.

Thought I would take a stab at adding this feature. I'm not sure how to add tests if any are required for this kind of change, pointers in the right direction there would be appreciated.

Manually tested using the below config:

# Create a new instance of the latest Ubuntu 14.04 on an
# t2.micro node with an AWS Tag naming it "HelloWorld"
provider "aws" {
  region = "us-east-1"
}

variable "subnet_id" {}

data "aws_ami" "ubuntu" {
  most_recent = true

  filter {
    name   = "name"
    values = ["ubuntu/images/hvm-ssd/ubuntu-trusty-14.04-amd64-server-*"]
  }

  filter {
    name   = "virtualization-type"
    values = ["hvm"]
  }

  owners = ["099720109477"] # Canonical
}

resource "aws_instance" "web" {
  ami           = "${data.aws_ami.ubuntu.id}"
  instance_type = "t2.micro"
  subnet_id = "${var.subnet_id}"

  root_block_device {
    volume_type = "standard"
    volume_size = "8"
  }

  tags {
    Name = "HelloWorld"
  }
}

output "root_volume_id" {
  value = "${aws_instance.web.root_block_device.0.volume_id}"
}

cc @fields is this what you were looking for?

[fields]

Assuming it does what it seems to do, yes - that would be exactly what I'm looking for.

[fields]

There can only ever be one root block device, so making that an array seems a little weird to me, but that's not a huge problem.

[rowleyaj]

@fields I believe it's always been an array/list, set to a maximum of 1 item though. Thats in the code here: https://github.com/rowleyaj/terraform-provider-aws/blob/master/aws/resource_aws_instance.go#L372-L376

The other ebs volumes also have the ids exported in the same way.

[kwilczynski]

@rowleyaj looks good to me! But we need to wait for someone like @radeksimko to make the final call.

[grubernaut]

LGTM, however mind adding an acceptance test for this, or adding an additional attribute item to check onto an existing acceptance test?
Thanks!

[rowleyaj]

Thanks @grubernaut for taking the time to review my attempt here :)

I took a look at https://github.com/terraform-providers/terraform-provider-aws/blob/master/aws/resource_aws_instance_test.go and I think I worked out how to add an acceptance test for a static value and check the resource attributes (by changing the config and using resource.TestCheckResourceAttr, but how would I go about adding an acceptance test for this where I need to know the id of the volume beforehand to confirm it is set correctly?

Do I need to create a volume in a similar way to this: https://github.com/terraform-providers/terraform-provider-aws/blob/master/aws/resource_aws_instance_test.go#L60 and then use resource.TestCheckResourceAttr in a similar way to this: https://github.com/terraform-providers/terraform-provider-aws/blob/master/aws/resource_aws_instance_test.go#L78 but with vol.VolumeId passed in to check?

I can try and take a stab at this tomorrow. One important question, how do I run the acceptance tests, ideally just the one I write/edit so I don't have to spin up everything. I'm guessing that https://github.com/hashicorp/terraform/blob/master/.github/CONTRIBUTING.md#running-an-acceptance-test still applies even though the providers are now split out, and it will just require standard AWS credentials to with permission to create an instance + volume?

Sorry for all the questions.

[radeksimko]

I think I worked out how to add an acceptance test for a static value and check the resource attributes (by changing the config and using resource.TestCheckResourceAttr, but how would I go about adding an acceptance test for this where I need to know the id of the volume beforehand to confirm it is set correctly?

You have basically two options:

resource.TestCheckResourceAttrSet can check whether the field was set or not (simple empty/non-empty check). See example at https://github.com/terraform-providers/terraform-provider-aws/blob/master/aws/resource_aws_alb_test.go#L71-L74

resource.TestMatchResourceAttr provides checking via regexp, so you can check whether the field matches something like vol-[a-z0-9]+. See example at https://github.com/terraform-providers/terraform-provider-aws/blob/master/aws/resource_aws_alb_test.go#L135-L136

I don't personally have a preference - so pick the one you like.

Do I need to create a volume in a similar way to this

Unless I'm mistaken you shouldn't need to. You just need to pick an AMI which has a volume baked into it. See example at https://github.com/terraform-providers/terraform-provider-aws/blob/master/aws/resource_aws_instance_test.go#L196-L284

how do I run the acceptance tests, ideally just the one I write/edit

We don't have a provider-specific guide (yet) for running acceptance tests, but it indeed works pretty much the same way it did prior to provider split, e.g.

AWS_PROFILE=xyz make testacc TEST=./aws TESTARGS='-run= TestAccAWSInstance_blockDevices'

[Ninir]

Hi @rowleyaj

Do you feel you could have time to finish this? It seems that we are about to cross the finish line :)

let us know if you need any help here!

[rowleyaj]

Sorry @Ninir - I completely dropped the ball on this one. I've added acceptance tests now, for both my change and the ebs volume_id as I didn't see any for that either. Thanks for the pointers how to do this @radeksimko.

[Ninir]

Hey @rowleyaj

This is ready to be merged!

Thanks for the work! 👍 🚀

 make testacc TEST=./aws TESTARGS='-run=TestAccAWSInstance_'                              

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSInstance_ -timeout 120m
=== RUN   TestAccAWSInstance_importBasic
--- PASS: TestAccAWSInstance_importBasic (148.50s)
=== RUN   TestAccAWSInstance_importInDefaultVpc
--- PASS: TestAccAWSInstance_importInDefaultVpc (148.70s)
=== RUN   TestAccAWSInstance_importInEc2Classic
--- PASS: TestAccAWSInstance_importInEc2Classic (147.50s)
=== RUN   TestAccAWSInstance_basic
--- PASS: TestAccAWSInstance_basic (222.97s)
=== RUN   TestAccAWSInstance_userDataBase64
--- PASS: TestAccAWSInstance_userDataBase64 (175.75s)
=== RUN   TestAccAWSInstance_GP2IopsDevice
--- PASS: TestAccAWSInstance_GP2IopsDevice (113.60s)
=== RUN   TestAccAWSInstance_blockDevices
--- PASS: TestAccAWSInstance_blockDevices (114.41s)
=== RUN   TestAccAWSInstance_rootInstanceStore
--- PASS: TestAccAWSInstance_rootInstanceStore (121.84s)
=== RUN   TestAccAWSInstance_noAMIEphemeralDevices
--- PASS: TestAccAWSInstance_noAMIEphemeralDevices (90.53s)
=== RUN   TestAccAWSInstance_sourceDestCheck
--- PASS: TestAccAWSInstance_sourceDestCheck (280.35s)
=== RUN   TestAccAWSInstance_disableApiTermination
--- PASS: TestAccAWSInstance_disableApiTermination (242.34s)
=== RUN   TestAccAWSInstance_vpc
--- PASS: TestAccAWSInstance_vpc (144.06s)
=== RUN   TestAccAWSInstance_ipv6_supportAddressCount
--- PASS: TestAccAWSInstance_ipv6_supportAddressCount (287.54s)
=== RUN   TestAccAWSInstance_ipv6AddressCountAndSingleAddressCausesError
--- PASS: TestAccAWSInstance_ipv6AddressCountAndSingleAddressCausesError (37.80s)
=== RUN   TestAccAWSInstance_ipv6_supportAddressCountWithIpv4
--- PASS: TestAccAWSInstance_ipv6_supportAddressCountWithIpv4 (180.18s)
=== RUN   TestAccAWSInstance_multipleRegions
--- PASS: TestAccAWSInstance_multipleRegions (179.12s)
=== RUN   TestAccAWSInstance_NetworkInstanceSecurityGroups
--- PASS: TestAccAWSInstance_NetworkInstanceSecurityGroups (179.60s)
=== RUN   TestAccAWSInstance_NetworkInstanceVPCSecurityGroupIDs
--- PASS: TestAccAWSInstance_NetworkInstanceVPCSecurityGroupIDs (182.67s)
=== RUN   TestAccAWSInstance_tags
--- PASS: TestAccAWSInstance_tags (196.52s)
=== RUN   TestAccAWSInstance_volumeTags
--- PASS: TestAccAWSInstance_volumeTags (190.28s)
=== RUN   TestAccAWSInstance_volumeTagsComputed
--- PASS: TestAccAWSInstance_volumeTagsComputed (166.74s)
=== RUN   TestAccAWSInstance_instanceProfileChange
--- PASS: TestAccAWSInstance_instanceProfileChange (167.11s)
=== RUN   TestAccAWSInstance_withIamInstanceProfile
--- PASS: TestAccAWSInstance_withIamInstanceProfile (171.12s)
=== RUN   TestAccAWSInstance_privateIP
--- PASS: TestAccAWSInstance_privateIP (287.18s)
=== RUN   TestAccAWSInstance_associatePublicIPAndPrivateIP
--- PASS: TestAccAWSInstance_associatePublicIPAndPrivateIP (155.88s)
=== RUN   TestAccAWSInstance_keyPairCheck
--- PASS: TestAccAWSInstance_keyPairCheck (124.79s)
=== RUN   TestAccAWSInstance_rootBlockDeviceMismatch
--- PASS: TestAccAWSInstance_rootBlockDeviceMismatch (148.31s)
=== RUN   TestAccAWSInstance_forceNewAndTagsDrift
--- PASS: TestAccAWSInstance_forceNewAndTagsDrift (271.34s)
=== RUN   TestAccAWSInstance_changeInstanceType
--- PASS: TestAccAWSInstance_changeInstanceType (188.66s)
=== RUN   TestAccAWSInstance_primaryNetworkInterface
--- PASS: TestAccAWSInstance_primaryNetworkInterface (202.40s)
=== RUN   TestAccAWSInstance_primaryNetworkInterfaceSourceDestCheck
--- PASS: TestAccAWSInstance_primaryNetworkInterfaceSourceDestCheck (191.35s)
=== RUN   TestAccAWSInstance_addSecondaryInterface
--- PASS: TestAccAWSInstance_addSecondaryInterface (270.36s)
=== RUN   TestAccAWSInstance_addSecurityGroupNetworkInterface
--- PASS: TestAccAWSInstance_addSecurityGroupNetworkInterface (401.69s)
=== RUN   TestAccAWSInstance_associatePublic_defaultPrivate
--- PASS: TestAccAWSInstance_associatePublic_defaultPrivate (154.83s)
=== RUN   TestAccAWSInstance_associatePublic_defaultPublic
--- PASS: TestAccAWSInstance_associatePublic_defaultPublic (177.98s)
=== RUN   TestAccAWSInstance_associatePublic_explicitPublic
--- PASS: TestAccAWSInstance_associatePublic_explicitPublic (167.22s)
=== RUN   TestAccAWSInstance_associatePublic_explicitPrivate
--- PASS: TestAccAWSInstance_associatePublic_explicitPrivate (167.20s)
=== RUN   TestAccAWSInstance_associatePublic_overridePublic
--- PASS: TestAccAWSInstance_associatePublic_overridePublic (168.25s)
=== RUN   TestAccAWSInstance_associatePublic_overridePrivate
--- PASS: TestAccAWSInstance_associatePublic_overridePrivate (178.94s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	7145.660s

[bflad]

This has been released in terraform-provider-aws version 1.7.0. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

[ghost]

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!