Closed fhitchen closed 4 years ago
Hi Tom,
I'm done, ready for the next review. I have included tests for the current and the last two versions of couchbase.
regards, Francis.
On Thu, Jul 23, 2020 at 7:03 AM Tom Proctor notifications@github.com wrote:
@tomhjp commented on this pull request.
In couchbase_test.go https://github.com/hashicorp/vault-plugin-database-couchbase/pull/1#discussion_r459398092 :
- return func() {}, os.Getenv("COUCHBASE_HOST"), 0
- }
- if containerInitialized == true {
- return cleanup, "localhost", 0
- }
- pool, err := dockertest.NewPool("")
- if err != nil {
- t.Fatalf("Failed to connect to docker: %s", err)
- }
- ro := &dockertest.RunOptions{
- Repository: "docker.io/fhitchen/vault-couchbase",
- Tag: "latest",
- ExposedPorts: []string{"8091", "8092", "8093", "8094", "11207", "11210", "18091", "18092", "18093", "18094"},
Yeah I think docker networking is a better idea, although stepping back, I wonder if maybe it's best left as-is after all.
As long as this is the only test suite in the repo that runs the couchbase container (a reasonable constraint), the fact that the tests are running in a dedicated VM means this actually shouldn't be problematic with hardcoded ports. Given the difficulty and complexity of engineering around those fixed ports, it seems pragmatic to live with the constraint for now, and then reassess if it does cause issues down the line.
WDYT? Apologies for the wild goose chase on this one - I've been learning on the go about CircleCI and couchbase, so didn't have the full picture before.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/hashicorp/vault-plugin-database-couchbase/pull/1#discussion_r459398092, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABML24H73ABNQAODMAHIFIDR5ARI5ANCNFSM4OA3HPJQ .
I think the flakiness is due to the hard coded timeout in the connection_producer Connection method. I should have replaced it with the Context based timeout. I pumped up the CPU load on my old Linux box and have managed to reproduce 2 failures as it waits for the cluster to be ready after connecting.
I should fork the hashicorp repo now to make this change?
Also, will someone update the main hashicorp documentation to add couchbase to the secret engines -> database documentation?
Yup, with the Connection method using a Context and the default timeout upped from 5 seconds to 20 seconds, the testing flakiness is resolved. I have run the test for several hours on one of the older database versions (they are much slower) and did not see a single failure.
I think the flakiness is due to the hard coded timeout in the connection_producer Connection method. I should have replaced it with the Context based timeout. I pumped up the CPU load on my old Linux box and have managed to reproduce 2 failures as it waits for the cluster to be ready after connecting.
Yup, with the Connection method using a Context and the default timeout upped from 5 seconds to 20 seconds, the testing flakiness is resolved. I have run the test for several hours on one of the older database versions (they are much slower) and did not see a single failure.
That's great! Thanks for looking into that.
I should fork the hashicorp repo now to make this change?
That would be great. Hopefully if you make a fresh fork and PR, the CI should work properly this time too as it has been set up prior to the PR creation.
Also, will someone update the main hashicorp documentation to add couchbase to the secret engines -> database documentation?
Yep, I've got a few follow up tasks to get it integrated and packaged with vault
releases, one of which is documentation. You'll see a bit of activity from me soon on these pieces.
Hi, I have updated the plugin to support versions prior to the latest 6.5.0 version of Couchbase and incorporated suggestions from their engineering team regarding the use of the gocb SDK.
To do is to add support for provisioning accounts that use the group roles supported by Couchbase. I don't think this will be difficult, but haven't got around to it yet. Note:This has been done. Also need to add a few utilities to create test accounts and buckets in the off the shelf Couchbase Docker image rather than rely on the modified personal copy.
I was unable to think of a way to add a more sophisticated static role password rotation test, I hope you don't mind.