Allow Data Source for AWS instances to get instances that are in a state other than running #4950

dieterdemeyer · 2018-06-22T10:13:49Z

Changes proposed in this pull request:

Also fetch AWS instances that have an instance state other than running. The only state not taken into account is terminated.

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSAvailabilityZones'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run=TestAccAWSAvailabilityZones -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSAvailabilityZones_basic
--- FAIL: TestAccAWSAvailabilityZones_basic (0.00s)
	provider_test.go:62: AWS_ACCESS_KEY_ID must be set for acceptance tests
=== RUN   TestAccAWSAvailabilityZones_stateFilter
--- FAIL: TestAccAWSAvailabilityZones_stateFilter (0.00s)
	provider_test.go:62: AWS_ACCESS_KEY_ID must be set for acceptance tests
FAIL
FAIL	github.com/terraform-providers/terraform-provider-aws/aws	0.045s
make: *** [testacc] Error 1
...

bflad

Hi @dieterdemeyer 👋 Thanks for submitting this!

While you have the right intentions here, as written this could be a breaking change in behavior for some Terraform configurations. Some may be relying on the existing behavior of only running. Please see the below for ideas of how to implement this without breaking backwards compatibility.

Also I'm noting in your acceptance test output a few things:

make testacc TESTARGS='-run=TestAccAWSAvailabilityZones'

The pull request template only provides an example of which acceptance tests to run. Following typical Go conventions, the test names can be found in aws/data_source_aws_instances_test.go. So in this case: make testacc TESTARGS='-run=TestAccAWSInstancesDataSource' will run the appropriate acceptance tests for this data source.

provider_test.go:62: AWS_ACCESS_KEY_ID must be set for acceptance tests

You'll need to appropriately set either the AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY environment variables or the AWS_PROFILE environment variables. The testing suite will spin up real infrastructure on AWS and requires credentials to do so.

Please reach out with any questions or if you do not have time to implement the feedback. Thanks!

bflad · 2018-06-22T12:46:15Z

aws/data_source_aws_instances.go

@@ -50,8 +50,14 @@ func dataSourceAwsInstancesRead(d *schema.ResourceData, meta interface{}) error
 	params := &ec2.DescribeInstancesInput{
 		Filters: []*ec2.Filter{
 			&ec2.Filter{
-				Name:   aws.String("instance-state-name"),
-				Values: []*string{aws.String("running")},


The enhancement here needs to allow the instance-state to either be configurable, while leaving the existing default behavior to not break backwards compatibility. This can be accomplished two ways:

Less preferably as a string attribute, but provided as an example, e.g.

"instance_state_name": { Type: schema.TypeString, Optional: true, Default: ec2.InstanceStateNameRunning, ValidateFunc: validation.StringInSlice([]string{ ec2.InstanceStateNamePending, ec2.InstanceStateNameRunning, ec2.InstanceStateNameShuttingDown, ec2.InstanceStateNameStopped, ec2.InstanceStateNameStopping, ec2.InstanceStateNameTerminated, }, false), },

Where it is referenced in the code with:

params := &ec2.DescribeInstancesInput{ Filters: []*ec2.Filter{ &ec2.Filter{ Name: aws.String("instance-state-name"), Values: []*string{aws.String(d.Get("instance_state_name").(string))}, }, }, }

Or, more preferably, as a set so you can configure one or more instance states:

"instance_state_names": { Type: schema.TypeSet, Optional: true, Elem: schema.Schema{ Type: schema.TypeString, ValidateFunc: validation.StringInSlice([]string{ ec2.InstanceStateNamePending, ec2.InstanceStateNameRunning, ec2.InstanceStateNameShuttingDown, ec2.InstanceStateNameStopped, ec2.InstanceStateNameStopping, ec2.InstanceStateNameTerminated, }, false), }, },

Where it is referenced in the code with:

instanceStateNames := []*string{aws.String(ec2.InstanceStateNameRunning)} if v, ok := d.GetOk("instance_state_names"); ok && len(v.(*schema.Set).List()) > 0 { instanceStateNames = expandStringSet(v.(*schema.Set)) } params := &ec2.DescribeInstancesInput{ Filters: []*ec2.Filter{ &ec2.Filter{ Name: aws.String("instance-state-name"), Values: instanceStateNames, }, }, }

Either case should implement a new acceptance test that creates two instances and immediately calls the data source that accepts the pending (and probably running as well) instance states.

dieterdemeyer · 2018-06-24T18:05:51Z

@bflad,

Thanks for your feedback.
I will implement the required changes and get back to you.

dieterdemeyer · 2018-07-02T17:08:13Z

@bflad,

I made the required changes locally and now I'm trying to write a test case.
Is it allowed to manually setup some AWS instances and run the test case against them ?
If not, I'm not sure how I could create a test case that creates 2 instances where 1 of the instances is in a pending or stopped state that is required for successfully testing this new feature given that I need to write Terraform code to initiate the EC2 instances...
See also #190

bflad · 2018-07-02T18:21:27Z

@dieterdemeyer testing against pending and running should always catch aws_instance resources after creation. I wouldn't worry about stopped instances. Otherwise, you'll need to implement a PreConfig for the TestStep that manually calls EC2 API calls to stop the instances (in a retry loop) after creation since we do not currently implement the ability to manage aws_instance as stopped within Terraform itself.

No worries if you cannot figure out the testing. I would push up what you have so far.

dieterdemeyer · 2018-07-03T08:36:41Z

@bflad I modified the required code and tested this locally with one of our AWS accounts. This seems to work.
But my test case doesn't seem to pass. Could you have a look at what I'm missing here ?

My output is as follows:

make testacc TESTARGS='-run=TestAccAWSInstancesDataSource'

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run=TestAccAWSInstancesDataSource -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSInstancesDataSource_basic
--- PASS: TestAccAWSInstancesDataSource_basic (149.74s)
=== RUN   TestAccAWSInstancesDataSource_tags
--- PASS: TestAccAWSInstancesDataSource_tags (137.58s)
=== RUN   TestAccAWSInstancesDataSource_instance_state_names
--- FAIL: TestAccAWSInstancesDataSource_instance_state_names (5.29s)
	testing.go:518: Step 0 error: Error refreshing: 1 error(s) occurred:

		* data.aws_instances.test: 1 error(s) occurred:

		* data.aws_instances.test: data.aws_instances.test: Your query returned no results. Please change your search criteria and try again.
FAIL
FAIL	github.com/terraform-providers/terraform-provider-aws/aws	292.668s
make: *** [testacc] Error 1

bflad · 2018-07-03T14:54:09Z

aws/data_source_aws_instances_test.go

+
+data "aws_instances" "test" {
+  instance_tags {
+    Name = "*"


To properly setup the resource and data source ordering, you can reference the aws_instance resources created above 😄

Name = "${aws_instance.test.0.tags["Name"]}"

This should be enough to delay the data source to catch the two new instances 👍 Let me know if this still is causing an issue and we can dive in deeper. Thanks for your work here!

bflad · 2018-07-03T14:55:16Z

website/docs/d/instances.html.markdown

@@ -45,6 +47,8 @@ resource "aws_eip" "test" {
 * `instance_tags` - (Optional) A mapping of tags, each pair of which must
 exactly match a pair on desired instances.

+* `instance_state_names` - (Optional) A list of instance states that should be applicable to the desired instances. The permitted values are: `pending, running, shutting-down, stopped, stopping, terminated`


We should list the default behavior of returning only running here 👍

dieterdemeyer · 2018-07-03T18:10:31Z

@bflad Thanks for your assistance. I've updated the code and ran the tests. It seems to work with your suggestion and you can find the output below:

make testacc TESTARGS='-run=TestAccAWSInstancesDataSource'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run=TestAccAWSInstancesDataSource -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSInstancesDataSource_basic
--- PASS: TestAccAWSInstancesDataSource_basic (128.95s)
=== RUN   TestAccAWSInstancesDataSource_tags
--- PASS: TestAccAWSInstancesDataSource_tags (130.73s)
=== RUN   TestAccAWSInstancesDataSource_instance_state_names
--- PASS: TestAccAWSInstancesDataSource_instance_state_names (128.10s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	387.825s

bflad

Looks great, thanks @dieterdemeyer! 🚀

make testacc TEST=./aws TESTARGS='-run=TestAccAWSInstancesDataSource'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSInstancesDataSource -timeout 120m
=== RUN   TestAccAWSInstancesDataSource_basic
--- PASS: TestAccAWSInstancesDataSource_basic (100.72s)
=== RUN   TestAccAWSInstancesDataSource_tags
--- PASS: TestAccAWSInstancesDataSource_tags (109.24s)
=== RUN   TestAccAWSInstancesDataSource_instance_state_names
--- PASS: TestAccAWSInstancesDataSource_instance_state_names (99.12s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	309.122s

dieterdemeyer · 2018-07-04T06:13:23Z

@bflad Thanks for getting this merged !
Any idea when a new release will be made that includes this feature ?
Going by the ChangeLog.md, this provider is released weekly. Is that correct ?

bflad · 2018-07-04T12:32:41Z

This has been released in version 1.26.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

ghost · 2020-04-04T17:51:13Z

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!

Also retrieve AWS instances that are in a state other than running

2db7e9a

ghost added the size/XS Managed by automation to categorize the size of a PR. label Jun 22, 2018

dieterdemeyer changed the title ~~Also retrieve AWS instances that are in a state other than running~~ Allow Data Source for AWS instances to get instances that are in a state other than running Jun 22, 2018

bflad added enhancement Requests to existing resources that expand the functionality or scope. breaking-change Introduces a breaking change in current functionality; usually deferred to the next major release. service/ec2 Issues and PRs that pertain to the ec2 service. labels Jun 22, 2018

bflad suggested changes Jun 22, 2018

View reviewed changes

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Jun 22, 2018

Added argument for defining instance state names + added test case

a2e194b

ghost added size/M Managed by automation to categorize the size of a PR. and removed size/XS Managed by automation to categorize the size of a PR. labels Jul 3, 2018

bflad reviewed Jul 3, 2018

View reviewed changes

bflad removed the breaking-change Introduces a breaking change in current functionality; usually deferred to the next major release. label Jul 3, 2018

Fix test case + update documentation

3579071

ghost added the size/M Managed by automation to categorize the size of a PR. label Jul 3, 2018

bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label Jul 4, 2018

bflad approved these changes Jul 4, 2018

View reviewed changes

bflad added this to the v1.26.0 milestone Jul 4, 2018

bflad merged commit 9ea5e04 into hashicorp:master Jul 4, 2018

bflad added a commit that referenced this pull request Jul 4, 2018

Update CHANGELOG for #4950

1590956

ghost locked and limited conversation to collaborators Apr 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Data Source for AWS instances to get instances that are in a state other than running #4950

Allow Data Source for AWS instances to get instances that are in a state other than running #4950

dieterdemeyer commented Jun 22, 2018

bflad left a comment

bflad Jun 22, 2018

dieterdemeyer commented Jun 24, 2018

dieterdemeyer commented Jul 2, 2018

bflad commented Jul 2, 2018 •

edited

Loading

dieterdemeyer commented Jul 3, 2018

bflad Jul 3, 2018

bflad Jul 3, 2018

dieterdemeyer commented Jul 3, 2018

bflad left a comment

dieterdemeyer commented Jul 4, 2018 •

edited

Loading

bflad commented Jul 4, 2018

ghost commented Apr 4, 2020

Allow Data Source for AWS instances to get instances that are in a state other than running #4950

Allow Data Source for AWS instances to get instances that are in a state other than running #4950

Conversation

dieterdemeyer commented Jun 22, 2018

bflad left a comment

Choose a reason for hiding this comment

bflad Jun 22, 2018

Choose a reason for hiding this comment

dieterdemeyer commented Jun 24, 2018

dieterdemeyer commented Jul 2, 2018

bflad commented Jul 2, 2018 • edited Loading

dieterdemeyer commented Jul 3, 2018

bflad Jul 3, 2018

Choose a reason for hiding this comment

bflad Jul 3, 2018

Choose a reason for hiding this comment

dieterdemeyer commented Jul 3, 2018

bflad left a comment

Choose a reason for hiding this comment

dieterdemeyer commented Jul 4, 2018 • edited Loading

bflad commented Jul 4, 2018

ghost commented Apr 4, 2020

bflad commented Jul 2, 2018 •

edited

Loading

dieterdemeyer commented Jul 4, 2018 •

edited

Loading