From 3524afdf72b7efffbfeb6d4c514a55f66531ccb3 Mon Sep 17 00:00:00 2001
From: Ivan De Marino <ivan.de.marino@gmail.com>
Date: Wed, 23 Oct 2024 00:04:26 +0100
Subject: [PATCH] Preventing each acceptance test from spawning another
 provider

---
 .github/workflows/build-test.yml   |  2 --
 internal/client/client.go          | 12 ++++++------
 internal/client/pool.go            |  5 +++--
 internal/provider/provider.go      |  5 +++--
 internal/provider/provider_test.go | 26 ++++++++++++++------------
 5 files changed, 26 insertions(+), 24 deletions(-)

diff --git a/.github/workflows/build-test.yml b/.github/workflows/build-test.yml
index 513d307..63a4682 100644
--- a/.github/workflows/build-test.yml
+++ b/.github/workflows/build-test.yml
@@ -85,8 +85,6 @@ jobs:
         image: zookeeper:${{ matrix.zookeeper }}
         ports:
           - 2181:2181
-        env:
-          ZOO_MAX_CLIENT_CNXNS: 200
 
     steps:
 
diff --git a/internal/client/client.go b/internal/client/client.go
index 56ed0b0..1bb84e5 100644
--- a/internal/client/client.go
+++ b/internal/client/client.go
@@ -129,12 +129,6 @@ func NewClientFromEnv() (*Client, error) {
 	return NewClient(zkServers, zkSessionInt, zkUsername, zkPassword)
 }
 
-// Close the Client underlying connection.
-func (c *Client) Close() {
-	fmt.Println("[DEBUG] Closing underlying ZooKeeper connection")
-	c.zkConn.Close()
-}
-
 // Create a ZNode at the given path.
 //
 // Note that any necessary ZNode parents will be created if absent.
@@ -268,6 +262,12 @@ func (c *Client) Update(path string, data []byte, acl []zk.ACL) (*ZNode, error)
 	return c.Read(path)
 }
 
+// Close the Client underlying connection.
+func (c *Client) Close() {
+	fmt.Println("[DEBUG] Closing underlying ZooKeeper connection")
+	c.zkConn.Close()
+}
+
 // Delete the given ZNode.
 //
 // Note that will also delete any child ZNode, recursively.
diff --git a/internal/client/pool.go b/internal/client/pool.go
index 913b014..46767a1 100644
--- a/internal/client/pool.go
+++ b/internal/client/pool.go
@@ -18,8 +18,9 @@ func NewPool() *Pool {
 	}
 }
 
-// GetClient retrieves (or creates) a Client.
-func (p *Pool) GetClient(servers string, sessionTimeoutSec int, username string, password string) (*Client, error) {
+// GetOrCreateClient retrieves (or creates) a Client.
+// A new client is created for each unique set of construction parameters.
+func (p *Pool) GetOrCreateClient(servers string, sessionTimeoutSec int, username string, password string) (*Client, error) {
 	p.mu.Lock()
 	defer p.mu.Unlock()
 
diff --git a/internal/provider/provider.go b/internal/provider/provider.go
index 8e957b5..5c67873 100644
--- a/internal/provider/provider.go
+++ b/internal/provider/provider.go
@@ -9,7 +9,8 @@ import (
 )
 
 func New() (*schema.Provider, error) {
-	// Hold a pool of Client, so a Client connecting to the same ZooKeeper can be shared between resources.
+	// Hold a Clients Pool, so connection to the same ZooKeeper can be shared between resources.
+	// The client is safe for concurrent usage.
 	clientPool := client.NewPool()
 
 	return &schema.Provider{
@@ -65,7 +66,7 @@ func New() (*schema.Provider, error) {
 			if servers != "" {
 				// NOTE: Client Pool above is in a closure here
 				// because we don't have a way to add fields to the Provider.
-				c, err := clientPool.GetClient(servers, sessionTimeout, username, password)
+				c, err := clientPool.GetOrCreateClient(servers, sessionTimeout, username, password)
 
 				if err != nil {
 					// Report inability to connect internal Client
diff --git a/internal/provider/provider_test.go b/internal/provider/provider_test.go
index 4dd6546..821968c 100644
--- a/internal/provider/provider_test.go
+++ b/internal/provider/provider_test.go
@@ -15,22 +15,26 @@ import (
 func TestProvider(t *testing.T) {
 	assert := testifyAssert.New(t)
 
-	provider, err := provider.New()
+	p, err := provider.New()
 	assert.NoError(err)
 
-	assert.NoError(provider.InternalValidate())
+	assert.NoError(p.InternalValidate())
 }
 
-// providerFactoriesMap associates to each Provider factory instance, a name.
-//
-// WARN: This is important as this will be the name the provider will be expected
-// to have when executing the acceptance tests.
-// Fail to match the provider expected name will mean that the underlying binary
-// terraform, used during acceptance tests, will error complaining it can't find
-// the provider and `terraform init` should be executed.
+//nolint:unparam
 func providerFactoriesMap() map[string]func() (*schema.Provider, error) {
+	// Instantiate the provider in advance...
+	p, err := provider.New()
+	if err != nil {
+		panic(fmt.Errorf("failed to instantiate provider: %w", err))
+	}
+
+	// ... then return it within the factory method.
+	// This avoids the tests creating a new provider (and new connections) every single time.
 	return map[string]func() (*schema.Provider, error){
-		"zookeeper": provider.New,
+		"zookeeper": func() (*schema.Provider, error) {
+			return p, nil
+		},
 	}
 }
 
@@ -41,8 +45,6 @@ func checkPreconditions(t *testing.T) {
 	}
 }
 
-// getTestZKClient can be used during test to procure a client.Client.
-
 // confirmAllZNodeDestroyed should be used with the field `CheckDestroy` of resource.TestCase.
 func confirmAllZNodeDestroyed(s *terraform.State) error {
 	fmt.Println("[DEBUG] Confirming all ZNodes have been removed")