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

add partition_count argument to aws_placement_group #7649

Conversation

ys56
Copy link

@ys56 ys56 commented Feb 22, 2019

Fix #7754

AWS added partition placement group feature which uses partition_count argument, so I added partiton_count argument to set the number.

Changes proposed in this pull request:

  • Added partition_count argument to aws_placement_group

Output from acceptance testing:

$  make testacc TEST=./aws TESTARGS='-run=TestAccAWSPlacementGroup'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -parallel 20 -run=TestAccAWSPlacementGroup -timeout 120m
=== RUN   TestAccAWSPlacementGroup_basic
=== PAUSE TestAccAWSPlacementGroup_basic
=== RUN   TestAccAWSPlacementGroup_partition
=== PAUSE TestAccAWSPlacementGroup_partition
=== CONT  TestAccAWSPlacementGroup_basic
=== CONT  TestAccAWSPlacementGroup_partition
--- PASS: TestAccAWSPlacementGroup_partition (7.83s)
--- PASS: TestAccAWSPlacementGroup_basic (8.20s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	8.218s

And bellows are sample partition placement group which acctest has created.

$ aws ec2 describe-placement-groups
{
    "PlacementGroups": [
        {
            "GroupName": "tf-acc-test-7259013876622124670",
            "State": "available",
            "Strategy": "partition",
            "PartitionCount": 7
        }
    ]
}

@ghost ghost added size/S Managed by automation to categorize the size of a PR. documentation Introduces or discusses updates to documentation. service/ec2 Issues and PRs that pertain to the ec2 service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Feb 22, 2019
@ys56 ys56 changed the title WIP add partition_count argument to aws_placement_group [WIP] add partition_count argument to aws_placement_group Feb 22, 2019
@ys56 ys56 changed the title [WIP] add partition_count argument to aws_placement_group add partition_count argument to aws_placement_group Feb 26, 2019
@bflad bflad added the enhancement Requests to existing resources that expand the functionality or scope. label Mar 4, 2019
@bflad
Copy link
Contributor

bflad commented Mar 4, 2019

Hi @ys56 👋 Would you prefer to fix feedback in this pull request or have it adjusted in #7777?

@bflad bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Mar 4, 2019
@ys56
Copy link
Author

ys56 commented Mar 4, 2019

Hi @bflad ,
thanks for your reviewing.
Well, I’d like to fix in this pull request unless it breaks #7777 .

@ghost ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Mar 4, 2019
@ys56
Copy link
Author

ys56 commented Mar 28, 2019

Hi @bflad
If it is easier for you to review in #7777 , I don't mind and please proceed that way because our team would like to import our partition placement group to our tf files.

Thanks,
Yu

@aeschright aeschright requested a review from a team June 26, 2019 00:47
@ys56
Copy link
Author

ys56 commented Sep 16, 2019

Hi @ziggythehamster
Thanks for reviewing my commit !
I have reflected your review.

@Monpoke
Copy link

Monpoke commented Jan 10, 2020

Hello @ziggythehamster !

Could you please review this commit?
It's been a while this MR is opened and I think it would be really useful for a lot of people using AWS.

Thanks.

@AlexanderKaraberov
Copy link

AlexanderKaraberov commented Feb 3, 2020

Hello,
Just wanted to ask whether it's possible that this PR will land before the next terraform-provider-aws release? Partition placement groups is an important feature for almost all cases to deploy a distributed database system. For instance in our case we need this to deploy a CouchDB cluster. Currently when we create a placement group with a partition strategy, AWS API fallbacks to a default partition_count of 2 which is not always useful.

Thanks!

@ziggythehamster
Copy link
Contributor

It looks good to me - I'm already running a fork containing this PR because I desperately needed this feature, but that is of course not great. Hopefully someone at Hashicorp can work this through. You do have some conflicts though.

AWS added partition placement group feature which uses partition_count argument.
So I added partiton_count argument to set the number.
@ys56 ys56 force-pushed the feature/add_partition_strategy_to_resource_aws_placement_group branch from 816791f to 9c09fb8 Compare February 6, 2020 21:47
@ghost ghost added size/M Managed by automation to categorize the size of a PR. and removed size/S Managed by automation to categorize the size of a PR. labels Feb 6, 2020
@ys56
Copy link
Author

ys56 commented Feb 6, 2020

Hi @ziggythehamster , @Monpoke , @AlexanderKaraberov ,
Thanks for reviewing and mentioning my pull request!

Well, I have rebased latest master and resolve some conflicts.
And below is new test results.

[ec2-user@ip-172-31-7-27 terraform-provider-aws]$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSPlacementGroup'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSPlacementGroup -timeout 120m
=== RUN   TestAccAWSPlacementGroup_basic
=== PAUSE TestAccAWSPlacementGroup_basic
=== RUN   TestAccAWSPlacementGroup_tags
=== PAUSE TestAccAWSPlacementGroup_tags
=== RUN   TestAccAWSPlacementGroup_disappears
=== PAUSE TestAccAWSPlacementGroup_disappears
=== RUN   TestAccAWSPlacementGroup_partition
=== PAUSE TestAccAWSPlacementGroup_partition
=== CONT  TestAccAWSPlacementGroup_basic
=== CONT  TestAccAWSPlacementGroup_partition
=== CONT  TestAccAWSPlacementGroup_disappears
=== CONT  TestAccAWSPlacementGroup_tags
--- PASS: TestAccAWSPlacementGroup_disappears (11.74s)
--- PASS: TestAccAWSPlacementGroup_partition (14.75s)
--- PASS: TestAccAWSPlacementGroup_basic (15.04s)
--- PASS: TestAccAWSPlacementGroup_tags (33.64s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	33.681s

I'd like @bflad or other Hashicorp members to re-review my new commits.

regards,

@ziggythehamster
Copy link
Contributor

I also would like them to review this (and #7777). It's not fun maintaining a fork! :)

Copy link

@chrisschaaf chrisschaaf left a comment

Choose a reason for hiding this comment

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

Approving based on our experience - have had this working in a forked repo for ~9 months. Would love to get this integrated here!

@ziggythehamster
Copy link
Contributor

I'm using the fork in #7777 for some time now (and updated for latest provider release, see: https://github.com/art19/terraform-provider-aws/tree/art19-v2.53.0) without issue.

@ziggythehamster
Copy link
Contributor

There is a newer, merging, almost identical PR in #15360. I'm not sure how to poke Hashicorp to review and merge either of these though :(

@ewbankkit
Copy link
Contributor

@ys56 Thanks for the contribution 🎉 👏.
Superseded by #15360.

@ewbankkit ewbankkit closed this Oct 13, 2021
@github-actions
Copy link

I'm going to lock this pull request 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 related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Introduces or discusses updates to documentation. enhancement Requests to existing resources that expand the functionality or scope. service/ec2 Issues and PRs that pertain to the ec2 service. size/M Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support EC2 Partition Placement Groups
7 participants