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

[WIP] Fix 3511 support all elbv2 attributes #3578

Closed
wants to merge 2 commits into from
Closed

[WIP] Fix 3511 support all elbv2 attributes #3578

wants to merge 2 commits into from

Conversation

appilon
Copy link
Contributor

@appilon appilon commented Mar 1, 2018

First PR, implement missing load balancer attributes for elbv2, small refactor to better group lbtype specific attributes.

  • Acceptance coverage
  • Documentation update

- add enable_cross_zone_load_balancing
- add enable_http2
- refactor updateFunc to group lbType specific attributes
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Mar 1, 2018
@radeksimko radeksimko added enhancement Requests to existing resources that expand the functionality or scope. service/elbv2 Issues and PRs that pertain to the elbv2 service. labels Mar 1, 2018
@bflad
Copy link
Contributor

bflad commented Mar 1, 2018

FYI cross-zone load balancing is being implemented in #3537

@appilon
Copy link
Contributor Author

appilon commented Mar 1, 2018

Dang, well it was a good exercise, I'll change it to just add support for http2 (which is on by default and I don't know that is is any advantage to turning it off.. but at least it will close off covering all the attributes).

@appilon
Copy link
Contributor Author

appilon commented Mar 1, 2018

I will wait for the mentioned PR to be merged and rebase from there, there is a small piece of refactoring I still think is a good idea

@mpilar
Copy link

mpilar commented Mar 2, 2018

@appilon I have a commit ready with your refactor and adding http2 with tests. Once my PR is merged I will rebase the commit.

Thinking about the refactor, it might be a good idea now but if AWS adds more LB types and rules apply to certain combinations it might get unruly, but it can just be refactored again when it gets to that.

@appilon
Copy link
Contributor Author

appilon commented Mar 2, 2018

@mpilar nice! yeah I agree things could get out of hand with more permutations. When your future PR gets merged I'll close this one. I was working off #3511 for reference for your next PR so you can trigger that issue being closed.

mpilar pushed a commit to mpilar/terraform-provider-aws that referenced this pull request Mar 2, 2018
- refactored to use a switch statement a la hashicorp#3578
- added enable_http2 as a flag with tests
- made the boolean flag tests have 3 steps: on, off, on. to test creation, removal and enabling of a running LB
- increased timeout and delay for aws_internet_gateway detachment refresh
- increased wait time to wait for network interfaces to be destroyed after deleting an NLB (this, combined with the above, caused the EIP hanging resource I saw yesterday)
- made all test fixtures use lb_test as the name for the aws_lb (to prevent the typo issue I had)
@appilon appilon closed this Mar 6, 2018
@ghost
Copy link

ghost commented Apr 7, 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 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Requests to existing resources that expand the functionality or scope. service/elbv2 Issues and PRs that pertain to the elbv2 service. size/L Managed by automation to categorize the size of a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants