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

Support for proxy protocol 2 attribute #4365

Merged
merged 2 commits into from
May 7, 2018
Merged

Support for proxy protocol 2 attribute #4365

merged 2 commits into from
May 7, 2018

Conversation

ddtmachado
Copy link

Just adding support for the attribute proxy_protocol_v2 on target groups.

I need this for our services to work and can't afford to do it manually every time we create a new environment.

I don't know if I got the test's right, feel free to change it or point the right way.

@ghost ghost added the size/M Managed by automation to categorize the size of a PR. label Apr 26, 2018
@ddtmachado ddtmachado mentioned this pull request Apr 26, 2018
Closed
bflad
bflad reviewed Apr 26, 2018
View reviewed changes
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Thanks for this @ddtmachado! 😄 Can you see the below and let me know if you have any questions?

aws/resource_aws_lb_target_group_test.go Outdated
@@ -156,6 +157,13 @@ func TestAccAWSLBTargetGroup_networkLB_TargetGroup(t *testing.T) {
ExpectNonEmptyPlan: true,
ExpectError: regexp.MustCompile("Health check interval cannot be updated"),
},
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please move this to its own acceptance test?

aws/resource_aws_lb_target_group.go
@@ -83,6 +83,12 @@ func resourceAwsLbTargetGroup() *schema.Resource {
ValidateFunc: validation.IntBetween(0, 3600),
},

"proxy_protocol_v2": {
Copy link
Contributor

@bflad bflad Apr 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Does this attribute support updates? If so, we should add an acceptance test step for that. If not, it should be marked as ForceNew: true,

This attribute is also missing a d.Set() call in the flattenAwsLbTargetGroupResource function so Terraform is not being updated about its status.

@bflad bflad added enhancement Requests to existing resources that expand the functionality or scope. service/elbv2 Issues and PRs that pertain to the elbv2 service. labels Apr 26, 2018
@bflad
Copy link
Contributor

bflad commented Apr 26, 2018

Oh! I forgot to mention -- this new attribute should also be documented in website/docs/r/lb_target_group.html.markdown

@bflad bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Apr 26, 2018
@ddtmachado
Copy link
Author

Thanks @bflad for the review!
I'll look at it and update the PR soon

@ghost ghost added the size/M Managed by automation to categorize the size of a PR. label May 7, 2018
@bflad bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label May 7, 2018
@bflad bflad added this to the v1.18.0 milestone May 7, 2018
bflad
bflad approved these changes May 7, 2018
View reviewed changes
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

This looks great, thanks! 🚀

21 tests passed (all tests)
=== RUN   TestAccAWSLBTargetGroup_namePrefix
--- PASS: TestAccAWSLBTargetGroup_namePrefix (65.71s)
=== RUN   TestAccAWSLBTargetGroup_stickinessWithTCPEnabledShouldError
--- PASS: TestAccAWSLBTargetGroup_stickinessWithTCPEnabledShouldError (4.07s)
=== RUN   TestAccAWSLBTargetGroupAttachment_basic
--- PASS: TestAccAWSLBTargetGroupAttachment_basic (249.69s)
=== RUN   TestAccAWSLBTargetGroup_basic
--- PASS: TestAccAWSLBTargetGroup_basic (255.90s)
=== RUN   TestAccAWSLBTargetGroupAttachmentBackwardsCompatibility
--- PASS: TestAccAWSLBTargetGroupAttachmentBackwardsCompatibility (273.69s)
=== RUN   TestAccAWSLBTargetGroupAttachment_withoutPort
--- PASS: TestAccAWSLBTargetGroupAttachment_withoutPort (302.98s)
=== RUN   TestAccAWSLBTargetGroup_defaults_application
--- PASS: TestAccAWSLBTargetGroup_defaults_application (620.91s)
=== RUN   TestAccAWSLBTargetGroupBackwardsCompatibility
--- PASS: TestAccAWSLBTargetGroupBackwardsCompatibility (657.66s)
=== RUN   TestAccAWSLBTargetGroup_changeProtocolForceNew
--- PASS: TestAccAWSLBTargetGroup_changeProtocolForceNew (666.31s)
=== RUN   TestAccAWSLBTargetGroup_defaults_network
--- PASS: TestAccAWSLBTargetGroup_defaults_network (707.42s)
=== RUN   TestAccAWSLBTargetGroup_networkLB_TargetGroup
--- PASS: TestAccAWSLBTargetGroup_networkLB_TargetGroup (709.80s)
=== RUN   TestAccAWSLBTargetGroup_changeVpcForceNew
--- PASS: TestAccAWSLBTargetGroup_changeVpcForceNew (720.41s)
=== RUN   TestAccAWSLBTargetGroup_TCP_HTTPHealthCheck
--- PASS: TestAccAWSLBTargetGroup_TCP_HTTPHealthCheck (743.54s)
=== RUN   TestAccAWSLBTargetGroup_stickinessWithTCPDisabled
--- PASS: TestAccAWSLBTargetGroup_stickinessWithTCPDisabled (743.91s)
=== RUN   TestAccAWSLBTargetGroup_networkLB_TargetGroupWithProxy
--- PASS: TestAccAWSLBTargetGroup_networkLB_TargetGroupWithProxy (774.78s)
=== RUN   TestAccAWSLBTargetGroup_updateSticknessEnabled
--- PASS: TestAccAWSLBTargetGroup_updateSticknessEnabled (799.35s)
=== RUN   TestAccAWSLBTargetGroup_tags
--- PASS: TestAccAWSLBTargetGroup_tags (856.68s)
=== RUN   TestAccAWSLBTargetGroup_changePortForceNew
--- PASS: TestAccAWSLBTargetGroup_changePortForceNew (1051.84s)
=== RUN   TestAccAWSLBTargetGroup_changeNameForceNew
--- PASS: TestAccAWSLBTargetGroup_changeNameForceNew (1200.71s)
=== RUN   TestAccAWSLBTargetGroup_updateHealthCheck
--- PASS: TestAccAWSLBTargetGroup_updateHealthCheck (2184.87s)
=== RUN   TestAccAWSLBTargetGroup_generatedName
--- PASS: TestAccAWSLBTargetGroup_generatedName (2219.23s)

@bflad bflad merged commit 21ace8f into hashicorp:master May 7, 2018
bflad added a commit that referenced this pull request May 7, 2018
@bflad
Update CHANGELOG for #4365
fac71b8
@bflad
Copy link
Contributor

bflad commented May 10, 2018

This has been released in version 1.18.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

@vandrijevik vandrijevik mentioned this pull request May 15, 2018
Merged
@ghost
Copy link

ghost commented Apr 6, 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 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bflad bflad bflad approved these changes

Assignees
No one assigned
Labels
enhancement Requests to existing resources that expand the functionality or scope. service/elbv2 Issues and PRs that pertain to the elbv2 service. size/M Managed by automation to categorize the size of a PR.
Projects
None yet
Milestone
v1.18.0
Development

Successfully merging this pull request may close these issues.
2 participants
@ddtmachado @bflad