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

added placement group as an option for autoscaling groups #3704

Merged
merged 7 commits into from
Dec 4, 2015

Conversation

dayer4b
Copy link
Contributor

@dayer4b dayer4b commented Oct 30, 2015

in reference to issue #3703

Elem: &schema.Schema{Type: schema.TypeString},
Set: schema.HashString,
},

Copy link
Contributor

Choose a reason for hiding this comment

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

I just think the addition to the schema should be really simple:

TypeString
Optional

This goes as per the fact the docs says that the input is a string and can be empty

@stack72
Copy link
Contributor

stack72 commented Oct 30, 2015

@dayer4b you have only catered for the Create method. You need to make sure that placement_group is also taken care of in the Update method as well

You can look at #3705 to see the way I have implemented it

@dayer4b
Copy link
Contributor Author

dayer4b commented Nov 2, 2015

@stack72 thanks for all the help! I've caught up with your suggestions and updated my branch.

@stack72
Copy link
Contributor

stack72 commented Nov 4, 2015

@dayer4b, this looks much better

I just ran your tests and got the following:

=== RUN   TestAccAWSAutoScalingGroup_basic
--- FAIL: TestAccAWSAutoScalingGroup_basic (7.20s)
    testing.go:137: Step 0 error: Error applying: 1 error(s) occurred:

        * aws_autoscaling_group.bar: Error creating Autoscaling Group: ValidationError: Placement group test is invalid: The Placement Group 'test' is unknown.
            status code: 400, request id: 7403f6e8-828b-11e5-93c6-b9e2a4ee28a3

In order to fix these (verified), you need to make the test config as follows:

const testAccAWSAutoScalingGroupConfig = 
resource "aws_launch_configuration" "foobar" {
  image_id = "ami-21f78e11"
  instance_type = "t1.micro"
}

resource "aws_placement_group" "test" {
  name = "test"
  strategy = "cluster"
}

resource "aws_autoscaling_group" "bar" {
  availability_zones = ["us-west-2a"]
  name = "foobar3-terraform-test"
  max_size = 5
  min_size = 2
  health_check_grace_period = 300
  health_check_type = "ELB"
  desired_capacity = 4
  force_delete = true
  termination_policies = ["OldestInstance","ClosestToNextInstanceHour"]
  placement_group = "${aws_placement_group.test.id}"

  launch_configuration = "${aws_launch_configuration.foobar.name}"

  tag {
    key = "Foo"
    value = "foo-bar"
    propagate_at_launch = true
  }
}

Notice that the test now sets up resource "aws_placement_group" "test

Paul

@catsby catsby added the waiting-response An issue/pull request is waiting for a response from the community label Nov 5, 2015
@dayer4b
Copy link
Contributor Author

dayer4b commented Nov 5, 2015

oh, of course. that makes a lot of sense. sorry. I'll get on that

@dayer4b
Copy link
Contributor Author

dayer4b commented Nov 9, 2015

Just an update - I added the aws_placement_group resource (to the test) last week. Sorry that I didn't make that clear.

@stack72
Copy link
Contributor

stack72 commented Nov 9, 2015

thanks @dayer4b - I will give your tests a shot again :)

@stack72
Copy link
Contributor

stack72 commented Nov 11, 2015

@dayer4b ok, I re-pulled your PR and the basic test actually doesn't work.

Currently, your test config looks as follows:

resource "aws_launch_configuration" "foobar" {
  image_id = "ami-21f78e11"
  instance_type = "t1.micro"
}

resource "aws_autoscaling_group" "bar" {
  availability_zones = ["us-west-2a"]
  name = "foobar3-terraform-test"
  max_size = 5
  min_size = 2
  health_check_grace_period = 300
  health_check_type = "ELB"
  desired_capacity = 4
  force_delete = true
  termination_policies = ["OldestInstance","ClosestToNextInstanceHour"]
  placement_group = "test"

  launch_configuration = "${aws_launch_configuration.foobar.name}"

  tag {
    key = "Foo"
    value = "foo-bar"
    propagate_at_launch = true
  }
}

This results in a test error as follows:

=== RUN   TestAccAWSAutoScalingGroup_basic
--- FAIL: TestAccAWSAutoScalingGroup_basic (6.14s)
    testing.go:137: Step 0 error: Error applying: 1 error(s) occurred:

        * aws_autoscaling_group.bar: Error creating Autoscaling Group: ValidationError: Placement group test is invalid: The Placement Group 'test' is unknown.
            status code: 400, request id: 60ebc73c-8894-11e5-ae11-fd4e0662e4f6

Your config needs to look as follows:

resource "aws_launch_configuration" "foobar" {
  image_id = "ami-21f78e11"
  instance_type = "t1.micro"
}

resource "aws_placement_group" "test" {
  name = "test"
  strategy = "cluster"
}

resource "aws_autoscaling_group" "bar" {
  availability_zones = ["us-west-2a"]
  name = "foobar3-terraform-test"
  max_size = 5
  min_size = 2
  health_check_grace_period = 300
  health_check_type = "ELB"
  desired_capacity = 4
  force_delete = true
  termination_policies = ["OldestInstance","ClosestToNextInstanceHour"]
  placement_group = "${aws_placement_group.test.id}"

  launch_configuration = "${aws_launch_configuration.foobar.name}"

  tag {
    key = "Foo"
    value = "foo-bar"
    propagate_at_launch = true
  }
}

You only added the placement_group resource to the documentation

@dayer4b
Copy link
Contributor Author

dayer4b commented Nov 13, 2015

wow, i had made the change and left it unstaged. sorry. i just pushed.

@dayer4b
Copy link
Contributor Author

dayer4b commented Nov 23, 2015

are there any remaining issues with this? I can amend my branch if need be

@catsby catsby removed the waiting-response An issue/pull request is waiting for a response from the community label Nov 23, 2015
@stack72
Copy link
Contributor

stack72 commented Nov 26, 2015

@dayer4b ok, so I've just re-tested this and have some final recommendations (I promise!)

func TestAccAWSAutoScalingGroup_withPlacementGroup(t *testing.T) {
    var group autoscaling.Group

    resource.Test(t, resource.TestCase{
        PreCheck:     func() { testAccPreCheck(t) },
        Providers:    testAccProviders,
        CheckDestroy: testAccCheckAWSAutoScalingGroupDestroy,
        Steps: []resource.TestStep{
            resource.TestStep{
                Config: testAccAWSAutoScalingGroupConfig_withPlacementGroup,
                Check: resource.ComposeTestCheckFunc(
                    testAccCheckAWSAutoScalingGroupExists("aws_autoscaling_group.bar", &group),
                    resource.TestCheckResourceAttr(
                        "aws_autoscaling_group.bar", "placement_group", "test"),
                ),
            },
        },
    })
}

then at the bottom of the test class you can add the config:


const testAccAWSAutoScalingGroupConfig_withPlacementGroup = `
resource "aws_launch_configuration" "foobar" {
  image_id = "ami-21f78e11"
  instance_type = "c3.large"
}

resource "aws_placement_group" "test" {
  name = "test"
  strategy = "cluster"
}

resource "aws_autoscaling_group" "bar" {
  availability_zones = ["us-west-2a"]
  name = "foobar3-terraform-test"
  max_size = 1
  min_size = 1
  health_check_grace_period = 300
  health_check_type = "ELB"
  desired_capacity = 1
  force_delete = true
  termination_policies = ["OldestInstance","ClosestToNextInstanceHour"]
  placement_group = "${aws_placement_group.test.name}"

  launch_configuration = "${aws_launch_configuration.foobar.name}"

  tag {
    key = "Foo"
    value = "foo-bar"
    propagate_at_launch = true
  }
}
`

This then makes the test pass!

@phinze
Copy link
Contributor

phinze commented Dec 4, 2015

Will merge this and make @stack72's recommended tweaks on master. 👍

Thanks for your work here, @dayer4b!

phinze added a commit that referenced this pull request Dec 4, 2015
added placement group as an option for autoscaling groups
@phinze phinze merged commit 709d1f3 into hashicorp:master Dec 4, 2015
phinze added a commit that referenced this pull request Dec 4, 2015
@dayer4b dayer4b deleted the add-placement-group branch December 18, 2015 15:02
@ghost
Copy link

ghost commented Apr 29, 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 29, 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.

5 participants