resource/aws_alb_target_group: Add support for `target_type` #1589

stack72 · 2017-09-05T09:59:57Z

% make testacc TEST=./aws TESTARGS='-run=TestAccAWSALBTargetGroup_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSALBTargetGroup_ -timeout 120m
=== RUN   TestAccAWSALBTargetGroup_basic
--- PASS: TestAccAWSALBTargetGroup_basic (73.55s)
=== RUN   TestAccAWSALBTargetGroup_namePrefix
--- PASS: TestAccAWSALBTargetGroup_namePrefix (72.26s)
=== RUN   TestAccAWSALBTargetGroup_generatedName
--- PASS: TestAccAWSALBTargetGroup_generatedName (74.94s)
=== RUN   TestAccAWSALBTargetGroup_changeNameForceNew
--- PASS: TestAccAWSALBTargetGroup_changeNameForceNew (129.78s)
=== RUN   TestAccAWSALBTargetGroup_changeProtocolForceNew
--- PASS: TestAccAWSALBTargetGroup_changeProtocolForceNew (151.66s)
=== RUN   TestAccAWSALBTargetGroup_changePortForceNew
--- PASS: TestAccAWSALBTargetGroup_changePortForceNew (129.73s)
=== RUN   TestAccAWSALBTargetGroup_changeVpcForceNew
--- PASS: TestAccAWSALBTargetGroup_changeVpcForceNew (112.48s)
=== RUN   TestAccAWSALBTargetGroup_tags
--- PASS: TestAccAWSALBTargetGroup_tags (124.72s)
=== RUN   TestAccAWSALBTargetGroup_updateHealthCheck
--- PASS: TestAccAWSALBTargetGroup_updateHealthCheck (122.61s)
=== RUN   TestAccAWSALBTargetGroup_updateSticknessEnabled
--- PASS: TestAccAWSALBTargetGroup_updateSticknessEnabled (170.23s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	1161.982s

Ninir

Hi @stack72 ,

This LGTM! just left a few comments to address :)

Tests are passing though :)

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSALBTargetGroupAttachment_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSALBTargetGroupAttachment_basic -timeout 120m
=== RUN   TestAccAWSALBTargetGroup_basic
--- PASS: TestAccAWSALBTargetGroup_basic (57.32s)
=== RUN   TestAccAWSALBTargetGroup_namePrefix
--- PASS: TestAccAWSALBTargetGroup_namePrefix (61.87s)
=== RUN   TestAccAWSALBTargetGroup_generatedName
--- PASS: TestAccAWSALBTargetGroup_generatedName (53.07s)
=== RUN   TestAccAWSALBTargetGroup_changeNameForceNew
--- PASS: TestAccAWSALBTargetGroup_changeNameForceNew (90.71s)
=== RUN   TestAccAWSALBTargetGroup_changeProtocolForceNew
--- PASS: TestAccAWSALBTargetGroup_changeProtocolForceNew (93.98s)
=== RUN   TestAccAWSALBTargetGroup_changePortForceNew
--- PASS: TestAccAWSALBTargetGroup_changePortForceNew (89.92s)
=== RUN   TestAccAWSALBTargetGroup_changeVpcForceNew
--- PASS: TestAccAWSALBTargetGroup_changeVpcForceNew (84.06s)
=== RUN   TestAccAWSALBTargetGroup_tags
--- PASS: TestAccAWSALBTargetGroup_tags (88.30s)
=== RUN   TestAccAWSALBTargetGroup_updateHealthCheck
--- PASS: TestAccAWSALBTargetGroup_updateHealthCheck (87.04s)
=== RUN   TestAccAWSALBTargetGroup_updateSticknessEnabled
--- PASS: TestAccAWSALBTargetGroup_updateSticknessEnabled (114.76s)

Ninir · 2017-09-06T08:08:54Z

aws/resource_aws_alb_target_group.go

+				Optional:     true,
+				Default:      "instance",
+				ForceNew:     true,
+				ValidateFunc: validation.StringInSlice([]string{"ip", "instance"}, true),


Could you prefer the use of constants (TargetTypeEnumInstance & TargetTypeEnumIp) here?

I don't see what extra that would give us? It's for validation of what the user inputs - i'm interested in seeing what you think

By using SDK constants everywhere, you rely on the same exact value everywhere.
Relying on a user-typed string can brings issues if any typo is made. Thus, that's why we try to stick to them as much as possible, whenever they exist.

I will update the PR to remove the validation - I don't see any reason why we need to use constants when the values are ip or instance - if they were longer strings then I would understand

Paul,

Perhaps I was misleading in my comment, but the validation was fine.

Was just wondering if we could do that:

ValidateFunc: validation.StringInSlice([]string{elbv2.TargetTypeEnumInstance, elbv2.TargetTypeEnumIp}, true),

Sorry if it was not clear at all 😅

Ninir · 2017-09-06T08:17:47Z

website/docs/r/alb_target_group.html.markdown

@@ -39,6 +39,12 @@ The following arguments are supported:
 * `deregistration_delay` - (Optional) The amount time for Elastic Load Balancing to wait before changing the state of a deregistering target from draining to unused. The range is 0-3600 seconds. The default value is 300 seconds.
 * `stickiness` - (Optional) A Stickiness block. Stickiness blocks are documented below.
 * `health_check` - (Optional) A Health Check block. Health Check blocks are documented below.
+* `target_type` - (Optional) The type of target that you must specify when registering targets with this target group.
+The possible values are i`nstance` (targets are specified by instance ID) or `ip` (targets are specified by IP address).


Typo in instance

Ninir · 2017-09-06T08:18:17Z

website/docs/r/alb_target_group.html.markdown

+* `target_type` - (Optional) The type of target that you must specify when registering targets with this target group.
+The possible values are i`nstance` (targets are specified by instance ID) or `ip` (targets are specified by IP address).
+The default is `instance`. Note that you can't specify targets for a target group using both instance IDs and IP addresses.
+If the target type is `ip` , specify IP addresses from the subnets of the virtual private cloud (VPC) for the target group,


Additional space before the comma

stack72 · 2017-09-08T06:01:18Z

Thanks for the review @Ninir - made the docs changes - I don't agree with the constants change but i'm willing to listen to why

P.

stack72 · 2017-09-13T08:59:36Z

Hi @Ninir

I made the docs changes and just removed the validation check - I believe this is ready to go now

P.

Ninir · 2017-09-15T09:56:27Z

This looks very good to me!

Just a small question: shouldn't we add a validation for that? do you think adding it will restrict us as soon as there is a new potential target?

Will make the last review & run tests as soon as I get your answer on that :)

Thanks a lot! 👍

stack72 · 2017-09-23T11:50:06Z

@Ninir I think validation of that type will restrict us going forward - I had a huge conversation with @tombuildsstuff about validation the other day and the types we do feel wrong. Think of how long it has been since an AWS provider release

IF a new target was added, then it stops people using the provider

f3lang · 2017-09-28T10:31:51Z

@stack72 At first, great change :) I just did a test in my staging environment and stumbled upon an issue, that currently exists, when you try to attach ip targets.

When you attach an instance target, aws knows the availability zone. But when you attach an ip target to a target group with the target_type "ip", you need to tell AWS the Availability Zone of the target. If the target is not within a VPC, but on-premise, you need to set the Availability Zone of the target to "all".

The aws_alb_target_group_attachment doesn't have the availability_zone option right now, so the attachment process fails.

stack72 · 2017-10-04T12:49:56Z

Hi @f3lang

Thanks for the feedback here - you are indeed correct, I just updated the PR to include availability_zone in the alb_target_group_attachment

terraform-provider-aws [f-aws-alb-target-group-type●] % acctests aws TestAccAWSALBTargetGroupAttachment_
=== RUN   TestAccAWSALBTargetGroupAttachment_basic
--- PASS: TestAccAWSALBTargetGroupAttachment_basic (143.64s)
=== RUN   TestAccAWSALBTargetGroupAttachment_withoutPort
--- PASS: TestAccAWSALBTargetGroupAttachment_withoutPort (150.73s)
=== RUN   TestAccAWSALBTargetGroupAttachment_ipAddress
--- PASS: TestAccAWSALBTargetGroupAttachment_ipAddress (150.28s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	444.678s

Thanks for the nudge on this :)

P.

stack72 · 2017-10-04T12:50:21Z

@Ninir I don't believe we should add validation here so I believe this can be finalized

Paul

Fixes: hashicorp#1588 ``` % make testacc TEST=./aws TESTARGS='-run=TestAccAWSALBTargetGroup_' ==> Checking that code complies with gofmt requirements... TF_ACC=1 go test ./aws -v -run=TestAccAWSALBTargetGroup_ -timeout 120m === RUN TestAccAWSALBTargetGroup_basic --- PASS: TestAccAWSALBTargetGroup_basic (73.55s) === RUN TestAccAWSALBTargetGroup_namePrefix --- PASS: TestAccAWSALBTargetGroup_namePrefix (72.26s) === RUN TestAccAWSALBTargetGroup_generatedName --- PASS: TestAccAWSALBTargetGroup_generatedName (74.94s) === RUN TestAccAWSALBTargetGroup_changeNameForceNew --- PASS: TestAccAWSALBTargetGroup_changeNameForceNew (129.78s) === RUN TestAccAWSALBTargetGroup_changeProtocolForceNew --- PASS: TestAccAWSALBTargetGroup_changeProtocolForceNew (151.66s) === RUN TestAccAWSALBTargetGroup_changePortForceNew --- PASS: TestAccAWSALBTargetGroup_changePortForceNew (129.73s) === RUN TestAccAWSALBTargetGroup_changeVpcForceNew --- PASS: TestAccAWSALBTargetGroup_changeVpcForceNew (112.48s) === RUN TestAccAWSALBTargetGroup_tags --- PASS: TestAccAWSALBTargetGroup_tags (124.72s) === RUN TestAccAWSALBTargetGroup_updateHealthCheck --- PASS: TestAccAWSALBTargetGroup_updateHealthCheck (122.61s) === RUN TestAccAWSALBTargetGroup_updateSticknessEnabled --- PASS: TestAccAWSALBTargetGroup_updateSticknessEnabled (170.23s) PASS ok github.com/terraform-providers/terraform-provider-aws/aws 1161.982s ```

Ninir · 2017-10-11T08:06:20Z

@stack72 hey Paul,

Could you fix the Go issue (aws/resource_aws_alb_target_group_test.go:41:13: undefined: albTargetGroupSuffixFromARN)? Can't test it right now :/

Will run tests & merge as soon as it's all ok. Thanks!

alex1x · 2017-10-26T12:36:27Z

Would be great to have this finalized, this is an important missing feature.

Ninir

LGTM! 👍

@stack72 Took the liberty to update and fix the issue regarding the wrong method name usage.

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSALBTargetGroup_'                   
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSALBTargetGroup_ -timeout 120m
=== RUN   TestAccAWSALBTargetGroup_basic
--- PASS: TestAccAWSALBTargetGroup_basic (19.96s)
=== RUN   TestAccAWSALBTargetGroup_namePrefix
--- PASS: TestAccAWSALBTargetGroup_namePrefix (18.92s)
=== RUN   TestAccAWSALBTargetGroup_generatedName
--- PASS: TestAccAWSALBTargetGroup_generatedName (18.86s)
=== RUN   TestAccAWSALBTargetGroup_changeNameForceNew
--- PASS: TestAccAWSALBTargetGroup_changeNameForceNew (33.44s)
=== RUN   TestAccAWSALBTargetGroup_changeProtocolForceNew
--- PASS: TestAccAWSALBTargetGroup_changeProtocolForceNew (36.35s)
=== RUN   TestAccAWSALBTargetGroup_changePortForceNew
--- PASS: TestAccAWSALBTargetGroup_changePortForceNew (33.36s)
=== RUN   TestAccAWSALBTargetGroup_changeVpcForceNew
--- PASS: TestAccAWSALBTargetGroup_changeVpcForceNew (30.39s)
=== RUN   TestAccAWSALBTargetGroup_tags
--- PASS: TestAccAWSALBTargetGroup_tags (31.86s)
=== RUN   TestAccAWSALBTargetGroup_updateHealthCheck
--- PASS: TestAccAWSALBTargetGroup_updateHealthCheck (32.79s)
=== RUN   TestAccAWSALBTargetGroup_updateSticknessEnabled
--- PASS: TestAccAWSALBTargetGroup_updateSticknessEnabled (45.29s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	301.249s

Thanks for the work!

ghost · 2020-04-10T17:47:51Z

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!

Ninir suggested changes Sep 6, 2017

View reviewed changes

stack72 force-pushed the f-aws-alb-target-group-type branch from 04c6f81 to 2c07d41 Compare September 8, 2017 07:29

Ninir added the enhancement Requests to existing resources that expand the functionality or scope. label Sep 11, 2017

Ninir added the waiting-response Maintainers are waiting on response from community or contributor. label Sep 15, 2017

stack72 force-pushed the f-aws-alb-target-group-type branch from 2c07d41 to 893d6b5 Compare October 4, 2017 12:49

stack72 force-pushed the f-aws-alb-target-group-type branch from 893d6b5 to 7e2a397 Compare October 5, 2017 11:45

Fixed wrong method name usage

191b3ef

Ninir approved these changes Oct 26, 2017

View reviewed changes

Ninir merged commit 8d66014 into hashicorp:master Oct 26, 2017

ghost locked and limited conversation to collaborators Apr 10, 2020

breathingdust removed the waiting-response Maintainers are waiting on response from community or contributor. label Sep 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource/aws_alb_target_group: Add support for `target_type` #1589

resource/aws_alb_target_group: Add support for `target_type` #1589

stack72 commented Sep 5, 2017

Ninir left a comment •

edited

Loading

Ninir Sep 6, 2017

stack72 Sep 8, 2017

Ninir Sep 8, 2017

stack72 Sep 8, 2017

Ninir Sep 8, 2017 •

edited

Loading

Ninir Sep 6, 2017

Ninir Sep 6, 2017

stack72 commented Sep 8, 2017

stack72 commented Sep 13, 2017

Ninir commented Sep 15, 2017

stack72 commented Sep 23, 2017

f3lang commented Sep 28, 2017 •

edited

Loading

stack72 commented Oct 4, 2017

stack72 commented Oct 4, 2017

Ninir commented Oct 11, 2017

alex1x commented Oct 26, 2017

Ninir left a comment

ghost commented Apr 10, 2020

resource/aws_alb_target_group: Add support for target_type #1589

resource/aws_alb_target_group: Add support for target_type #1589

Conversation

stack72 commented Sep 5, 2017

Ninir left a comment • edited Loading

Choose a reason for hiding this comment

Ninir Sep 6, 2017

Choose a reason for hiding this comment

stack72 Sep 8, 2017

Choose a reason for hiding this comment

Ninir Sep 8, 2017

Choose a reason for hiding this comment

stack72 Sep 8, 2017

Choose a reason for hiding this comment

Ninir Sep 8, 2017 • edited Loading

Choose a reason for hiding this comment

Ninir Sep 6, 2017

Choose a reason for hiding this comment

Ninir Sep 6, 2017

Choose a reason for hiding this comment

stack72 commented Sep 8, 2017

stack72 commented Sep 13, 2017

Ninir commented Sep 15, 2017

stack72 commented Sep 23, 2017

f3lang commented Sep 28, 2017 • edited Loading

stack72 commented Oct 4, 2017

stack72 commented Oct 4, 2017

Ninir commented Oct 11, 2017

alex1x commented Oct 26, 2017

Ninir left a comment

Choose a reason for hiding this comment

ghost commented Apr 10, 2020

resource/aws_alb_target_group: Add support for `target_type` #1589

resource/aws_alb_target_group: Add support for `target_type` #1589

Ninir left a comment •

edited

Loading

Ninir Sep 8, 2017 •

edited

Loading

f3lang commented Sep 28, 2017 •

edited

Loading