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

d/aws_instance: Set placement_group if available #2400

Merged
merged 3 commits into from
Dec 6, 2017
Merged

d/aws_instance: Set placement_group if available #2400

merged 3 commits into from
Dec 6, 2017

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Nov 22, 2017

This is complementary PR to #2398 since the read functions are different between the resource and data source. The data source docs already note that placement_group may be set.

make testacc TEST=./aws TESTARGS='-run=TestAccAWSInstanceDataSource_PlacementGroup'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSInstanceDataSource_PlacementGroup -timeout 120m
=== RUN   TestAccAWSInstanceDataSource_PlacementGroup
--- PASS: TestAccAWSInstanceDataSource_PlacementGroup (84.20s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	84.240s

@Ninir Ninir added the enhancement Requests to existing resources that expand the functionality or scope. label Nov 22, 2017
Ninir
Ninir reviewed Nov 22, 2017
View reviewed changes
Copy link
Contributor

@Ninir Ninir left a comment

Choose a reason for hiding this comment

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

Hi @bflad

Thanks a lot for this work :) 🚀

Just left a nitpick here before merging! :)

aws/data_source_aws_instance_test.go Outdated
}

resource "aws_placement_group" "foo" {
name = "testAccInstanceDataSourceConfig_PlacementGroup"
Copy link
Contributor

Choose a reason for hiding this comment

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

We will need to add some name randomization here, so that we avoid conflicts when running tests

@bflad
d/aws_instance: Add name randomization and remove dedicated tenancy i…
1ae69d2 
…n TestAccAWSInstanceDataSource_PlacementGroup
@bflad
Copy link
Contributor Author

bflad commented Nov 22, 2017

Comment addressed! Passes testing for me still:

make testacc TEST=./aws TESTARGS='-run=TestAccAWSInstanceDataSource_PlacementGroup'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSInstanceDataSource_PlacementGroup -timeout 120m
=== RUN   TestAccAWSInstanceDataSource_PlacementGroup
--- PASS: TestAccAWSInstanceDataSource_PlacementGroup (88.35s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	88.392s

@stack72
Copy link
Contributor

stack72 commented Nov 23, 2017

@bflad / @Ninir I believe the documentation also needs to be updated as well - currently, for data_aws_instance, we list the outputs AFAICT

Thoughts?

@Ninir
Copy link
Contributor

Ninir commented Nov 24, 2017

HI @stack72

You're indeed right. @bflad could you also add the entry in the related documentation?

Thanks!

@bflad
Copy link
Contributor Author

bflad commented Nov 24, 2017

Are you referring to a location different than https://www.terraform.io/docs/providers/aws/d/instance.html#placement_group ?

@bflad
Copy link
Contributor Author

bflad commented Nov 28, 2017

@stack72 / @Ninir simple question above. Let me know if/where anything else needs adjustment. 😄

radeksimko
radeksimko approved these changes Dec 6, 2017
View reviewed changes
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

I believe this is ready to be merged - the field is already documented as mentioned.

Feel free to correct me if I'm wrong @stack72 / @Ninir

Thanks @bflad 🎉

@radeksimko radeksimko merged commit ff812a4 into hashicorp:master Dec 6, 2017
mdlavin pushed a commit to mdlavin/terraform-provider-aws that referenced this pull request Dec 9, 2017
@bflad @mdlavin
d/aws_instance: Set placement_group if available (hashicorp#2400)
285ad6c 
* d/aws_instance: Set placement_group if available

* d/aws_instance: Fix test indentations

* d/aws_instance: Add name randomization and remove dedicated tenancy in TestAccAWSInstanceDataSource_PlacementGroup
@ghost
Copy link

ghost commented Apr 10, 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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Apr 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Ninir Ninir Ninir left review comments

@radeksimko radeksimko radeksimko approved these changes

Assignees
No one assigned
Labels
enhancement Requests to existing resources that expand the functionality or scope.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bflad @stack72 @Ninir @radeksimko