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

Add max_connetions and max_connections_per_instance to backend service #1353

Merged
merged 3 commits into from
Apr 19, 2018

Conversation

jschwing
Copy link
Contributor

Hi,

could you check this pull request?
It seems max_connections_per_instance is missing for the backend service (max_connections likewise).

This makes it impossible to use external tcp load balancers and internal load balancers that point to the same instance group.

@nat-henderson nat-henderson self-requested a review April 18, 2018 23:31
@nat-henderson
Copy link
Contributor

Looks good! We'll just need a test, and an update to the docs, and I'll be happy to merge it. :)

Copy link
Contributor

@nat-henderson nat-henderson left a comment

Choose a reason for hiding this comment

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

You can write a test in google/resource_compute_backend_service_test.go and the docs in website/docs/r/compute_backend_service.html.markdown.

@jschwing jschwing changed the title [WIP] Add max_connections_per_instance to backend service Add max_connetions and max_connections_per_instance to backend service Apr 19, 2018
@jschwing
Copy link
Contributor Author

Hi,

I added max_connections as well wrote two tests and adapter the documentation.

Please let me know if there is anything missing.

Copy link
Contributor

@nat-henderson nat-henderson left a comment

Choose a reason for hiding this comment

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

Those are some really excellent detailed tests. :) I'm going to run them in our CI then merge.

@jschwing
Copy link
Contributor Author

Thanks. Let me know if the tests work.
I didn't run them locally, because I don't know if/if all tests do a proper cleanup afterwards.

@nat-henderson
Copy link
Contributor

They do a proper cleanup, yep.

In any event:

Test ended in panic.

------- Stdout: -------
=== RUN   TestAccComputeBackendService_withMaxConnections
--- FAIL: TestAccComputeBackendService_withMaxConnections (0.00s)
    testing.go:513: Step 0 error: config is invalid: resource 'google_compute_instance_group_manager.foobar' config: unknown resource 'google_compute_http_health_check.default' referenced in variable google_compute_http_health_check.default.self_link

------- Stderr: -------
panic: runtime error: index out of range [recovered]
    panic: runtime error: index out of range

goroutine 6 [running]:
testing.tRunner.func1(0xc420445a40)
    /usr/local/go/src/testing/testing.go:711 +0x2d2
panic(0x18717e0, 0x27b4a10)
    /usr/local/go/src/runtime/panic.go:491 +0x283
github.com/terraform-providers/terraform-provider-google/google.TestAccComputeBackendService_withMaxConnections(0xc420445a40)
    /opt/teamcity-agent/work/b5c4b70a841cda2e/src/github.com/terraform-providers/terraform-provider-google/google/resource_compute_backend_service_test.go:543 +0x99d
testing.tRunner(0xc420445a40, 0x1bf5ac8)
    /usr/local/go/src/testing/testing.go:746 +0xd0
created by testing.(*T).Run
    /usr/local/go/src/testing/testing.go:789 +0x2de
Test ended in panic.

------- Stdout: -------
=== RUN   TestAccComputeBackendService_withMaxConnectionsPerInstance
--- FAIL: TestAccComputeBackendService_withMaxConnectionsPerInstance (0.00s)
    testing.go:513: Step 0 error: config is invalid: resource 'google_compute_instance_group_manager.foobar' config: unknown resource 'google_compute_http_health_check.default' referenced in variable google_compute_http_health_check.default.self_link

------- Stderr: -------
panic: runtime error: index out of range [recovered]
    panic: runtime error: index out of range

goroutine 6 [running]:
testing.tRunner.func1(0xc420445a40)
    /usr/local/go/src/testing/testing.go:711 +0x2d2
panic(0x18717e0, 0x27b4a10)
    /usr/local/go/src/runtime/panic.go:491 +0x283
github.com/terraform-providers/terraform-provider-google/google.TestAccComputeBackendService_withMaxConnectionsPerInstance(0xc420445a40)
    /opt/teamcity-agent/work/b5c4b70a841cda2e/src/github.com/terraform-providers/terraform-provider-google/google/resource_compute_backend_service_test.go:586 +0x99d
testing.tRunner(0xc420445a40, 0x1bf5ac0)
    /usr/local/go/src/testing/testing.go:746 +0xd0
created by testing.(*T).Run
    /usr/local/go/src/testing/testing.go:789 +0x2de

@jschwing
Copy link
Contributor Author

My bad. I changed the http_health check to a tcp health check and forgot to adapt its usages.
I will try to fix this and upload a new change.

@nat-henderson
Copy link
Contributor

Thanks! Should be safe to run those tests locally for development cycle speed - the cleanup code is supposed to be robust. :)

@jschwing
Copy link
Contributor Author

I tried to run the tests. But I couldn't fetch the image:
oauth2: cannot fetch token: Post https://accounts.google.com/o/oauth2/token: dial tcp 216.58.210.13:443: i/o timeout

I guess this is due to my current setup where my repository sits behind a proxy.
Anyways the initial panic is gone (at least). If there are any more issues I will try to setup another repository that is not behind a proxy tomorrow.

@nat-henderson
Copy link
Contributor

Rerunning the tests.

@jschwing
Copy link
Contributor Author

I got the tests to work. I will upload another commit that will fix the test errors ;)

@jschwing
Copy link
Contributor Author

TF_ACC=1 go test -run TestAccComputeBackendService_withMaxConnections github.com/terraform-providers/terraform-provider-google/google -v

=== RUN TestAccComputeBackendService_withMaxConnections
=== PAUSE TestAccComputeBackendService_withMaxConnections
=== RUN TestAccComputeBackendService_withMaxConnectionsPerInstance
=== PAUSE TestAccComputeBackendService_withMaxConnectionsPerInstance
=== CONT TestAccComputeBackendService_withMaxConnections
=== CONT TestAccComputeBackendService_withMaxConnectionsPerInstance
--- PASS: TestAccComputeBackendService_withMaxConnections (150.55s)
--- PASS: TestAccComputeBackendService_withMaxConnectionsPerInstance (152.09s)
PASS
ok github.com/terraform-providers/terraform-provider-google/google 152.102s

@nat-henderson
Copy link
Contributor

All the tests pass on my end too. Merging.

@nat-henderson nat-henderson merged commit 0ae2047 into hashicorp:master Apr 19, 2018
@jschwing
Copy link
Contributor Author

Thanks for reviewing and merging this so fast!

chrisst pushed a commit to chrisst/terraform-provider-google that referenced this pull request Nov 9, 2018
@ghost
Copy link

ghost commented Nov 18, 2018

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. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Nov 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants