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

provider/aws: VPC ID, Port, Protocol and Name change on aws_alb_target_group will ForceNew resource #8989

Merged
merged 1 commit into from
Sep 22, 2016

Conversation

stack72
Copy link
Contributor

@stack72 stack72 commented Sep 22, 2016

Fixes #8741

The modify-target-group doesn't allow changes to name, port, protocol or
vpc_id - therefore, they should all be ForceNew: true

% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSALBTargetGroup_'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/09/22 16:04:29 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v
-run=TestAccAWSALBTargetGroup_ -timeout 120m
=== RUN   TestAccAWSALBTargetGroup_basic
--- PASS: TestAccAWSALBTargetGroup_basic (50.66s)
=== RUN   TestAccAWSALBTargetGroup_changeNameForceNew
--- PASS: TestAccAWSALBTargetGroup_changeNameForceNew (84.48s)
=== RUN   TestAccAWSALBTargetGroup_changeProtocolForceNew
--- PASS: TestAccAWSALBTargetGroup_changeProtocolForceNew (95.89s)
=== RUN   TestAccAWSALBTargetGroup_changePortForceNew
--- PASS: TestAccAWSALBTargetGroup_changePortForceNew (85.77s)
=== RUN   TestAccAWSALBTargetGroup_changeVpcForceNew
--- PASS: TestAccAWSALBTargetGroup_changeVpcForceNew (85.00s)
=== RUN   TestAccAWSALBTargetGroup_tags
--- PASS: TestAccAWSALBTargetGroup_tags (88.11s)
=== RUN   TestAccAWSALBTargetGroup_updateHealthCheck
--- PASS: TestAccAWSALBTargetGroup_updateHealthCheck (82.15s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    572.083s

func testAccChecALBTargetGroupRecreated(t *testing.T,
before, after *elbv2.TargetGroup) resource.TestCheckFunc {
return func(s *terraform.State) error {
if before.TargetGroupArn == after.TargetGroupArn {
Copy link
Contributor

@catsby catsby Sep 22, 2016

Choose a reason for hiding this comment

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

This checks the pointers, not the values, so this will always pass given how we're construction the two group structs in testAccCheckAWSALBTargetGroupExists. If you compare the actual values, you find that only TestAccAWSALBTargetGroup_changeNameForceNew passes here because that's the only one that changes the name, thus changes the ARN (being a combo of region, account, and name). All the other tests fail when checking the values here, because the name hasn't changed, so the ARN hasn't changed, even though the resource was actually destroyed and recreated.

I think it's sufficient to check that the new values were expecting were set. In this situation we don't actually care if it has a new ARN, or that the VPC changed in a ForceNew operation, we just care that if someone changed the VPC, that the change actually happens.

* `port` - (Required) The port on which targets receive traffic, unless overridden when registering a specific target.
* `protocol` - (Required) The protocol to use for routing traffic to the targets.
* `vpc_id` - (Required) The identifier of the VPC in which to create the target group.
* `name` - (Required) The name of the target group. Change forces new resource.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not strictly opposed to this inclusion, but I'm not sure we make note of the ForceNew behavior in other resources, do we?

@stack72 stack72 force-pushed the b-aws-alb-protocol-change-forcenew branch from 1058bf1 to 8edb43e Compare September 22, 2016 16:06
@stack72
Copy link
Contributor Author

stack72 commented Sep 22, 2016

@catsby you are correct - I have updated all as requested :)

aws_alb_target_group will ForceNew resource

Fixes #8741

The modify-target-group doesn't allow changes to name, port, protocol or
vpc_id - therefore, they should all be ForceNew: true

```
% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSALBTargetGroup_'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/09/22 16:04:29 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v
-run=TestAccAWSALBTargetGroup_ -timeout 120m
=== RUN   TestAccAWSALBTargetGroup_basic
--- PASS: TestAccAWSALBTargetGroup_basic (50.66s)
=== RUN   TestAccAWSALBTargetGroup_changeNameForceNew
--- PASS: TestAccAWSALBTargetGroup_changeNameForceNew (84.48s)
=== RUN   TestAccAWSALBTargetGroup_changeProtocolForceNew
--- PASS: TestAccAWSALBTargetGroup_changeProtocolForceNew (95.89s)
=== RUN   TestAccAWSALBTargetGroup_changePortForceNew
--- PASS: TestAccAWSALBTargetGroup_changePortForceNew (85.77s)
=== RUN   TestAccAWSALBTargetGroup_changeVpcForceNew
--- PASS: TestAccAWSALBTargetGroup_changeVpcForceNew (85.00s)
=== RUN   TestAccAWSALBTargetGroup_tags
--- PASS: TestAccAWSALBTargetGroup_tags (88.11s)
=== RUN   TestAccAWSALBTargetGroup_updateHealthCheck
--- PASS: TestAccAWSALBTargetGroup_updateHealthCheck (82.15s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    572.083s
```
@stack72 stack72 force-pushed the b-aws-alb-protocol-change-forcenew branch from 8edb43e to 9fbbc34 Compare September 22, 2016 16:12
@stack72
Copy link
Contributor Author

stack72 commented Sep 22, 2016

New acceptance test run:

% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSALBTargetGroup_'                                                                       ✹
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/09/22 17:12:38 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSALBTargetGroup_ -timeout 120m
=== RUN   TestAccAWSALBTargetGroup_basic
--- PASS: TestAccAWSALBTargetGroup_basic (51.46s)
=== RUN   TestAccAWSALBTargetGroup_changeNameForceNew
--- PASS: TestAccAWSALBTargetGroup_changeNameForceNew (85.77s)
=== RUN   TestAccAWSALBTargetGroup_changeProtocolForceNew
--- PASS: TestAccAWSALBTargetGroup_changeProtocolForceNew (90.85s)
=== RUN   TestAccAWSALBTargetGroup_changePortForceNew
--- PASS: TestAccAWSALBTargetGroup_changePortForceNew (85.44s)
=== RUN   TestAccAWSALBTargetGroup_changeVpcForceNew
--- PASS: TestAccAWSALBTargetGroup_changeVpcForceNew (77.74s)
=== RUN   TestAccAWSALBTargetGroup_tags
--- PASS: TestAccAWSALBTargetGroup_tags (81.86s)
=== RUN   TestAccAWSALBTargetGroup_updateHealthCheck
--- PASS: TestAccAWSALBTargetGroup_updateHealthCheck (81.79s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    554.929s

@stack72 stack72 merged commit ecabebf into master Sep 22, 2016
@stack72 stack72 deleted the b-aws-alb-protocol-change-forcenew branch September 22, 2016 19:57
@ghost
Copy link

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

aws_alb_target_group name/protocol change should force new resource
2 participants