(feat) start moving things to a factory to simplify to test

cmd/cleanup.go

@@ -20,7 +20,7 @@ import (
 	"log/slog"
 	"os"
 
-	"github.com/hashicorp/consul/api"
+	"github.com/ncode/tagit/pkg/consul"
 	"github.com/ncode/tagit/pkg/tagit"
 	"github.com/spf13/cobra"
 )
@@ -34,21 +34,20 @@ var cleanupCmd = &cobra.Command{
 			Level: slog.LevelInfo,
 		}))
 
-		config := api.DefaultConfig()
-		config.Address = cmd.InheritedFlags().Lookup("consul-addr").Value.String()
-		config.Token = cmd.InheritedFlags().Lookup("token").Value.String()
+		consulAddr := cmd.InheritedFlags().Lookup("consul-addr").Value.String()
+		token := cmd.InheritedFlags().Lookup("token").Value.String()
 
-		consulClient, err := api.NewClient(config)
+		consulClient, err := consul.CreateClient(consulAddr, token)
 		if err != nil {
 			logger.Error("Failed to create Consul client", "error", err)
-			return fmt.Errorf("failed to create Consul client: %w", err)
+			return err
 		}
 
 		serviceID := cmd.InheritedFlags().Lookup("service-id").Value.String()
 		tagPrefix := cmd.InheritedFlags().Lookup("tag-prefix").Value.String()
 
 		t := tagit.New(
-			tagit.NewConsulAPIWrapper(consulClient),
+			consulClient,
 			&tagit.CmdExecutor{},
 			serviceID,
 			"", // script is not needed for cleanup

cmd/cleanup_test.go

@@ -2,8 +2,11 @@ package cmd
 
 import (
 	"bytes"
+	"fmt"
 	"testing"
 
+	"github.com/hashicorp/consul/api"
+	"github.com/ncode/tagit/pkg/consul"
 	"github.com/spf13/cobra"
 	"github.com/stretchr/testify/assert"
 )
@@ -244,19 +247,36 @@ func TestCleanupCmdFlagRetrieval(t *testing.T) {
 
 func TestCleanupCmdSuccessFlow(t *testing.T) {
 	// Test the successful flow of cleanup command
+	// Since the actual cleanupCmd creates a real Consul client internally,
+	// we test with a mock command that simulates the successful path
 	cmd := &cobra.Command{Use: "tagit"}
 	cmd.PersistentFlags().StringP("consul-addr", "c", "127.0.0.1:8500", "consul address")
 	cmd.PersistentFlags().StringP("service-id", "s", "", "consul service id")
 	cmd.PersistentFlags().StringP("script", "x", "", "path to script used to generate tags")
 	cmd.PersistentFlags().StringP("tag-prefix", "p", "tagged", "prefix to be added to tags")
 	cmd.PersistentFlags().StringP("token", "t", "", "consul token")
 
+	var logOutput bytes.Buffer
 	testCleanupCmd := &cobra.Command{
 		Use:   "cleanup",
 		Short: "cleanup removes all services with the tag prefix from a given consul service",
 		RunE: func(cmd *cobra.Command, args []string) error {
-			// Simulate successful cleanup without actual consul connection
-			// This tests the success path that returns nil
+			// Verify all required flags are accessible
+			serviceID := cmd.InheritedFlags().Lookup("service-id").Value.String()
+			tagPrefix := cmd.InheritedFlags().Lookup("tag-prefix").Value.String()
+			consulAddr := cmd.InheritedFlags().Lookup("consul-addr").Value.String()
+			token := cmd.InheritedFlags().Lookup("token").Value.String()
+
+			// Simulate the logging that would happen
+			fmt.Fprintf(&logOutput, "Starting tag cleanup, serviceID=%s, tagPrefix=%s, consulAddr=%s\n",
+				serviceID, tagPrefix, consulAddr)
+
+			if token != "" {
+				fmt.Fprintf(&logOutput, "Using token authentication\n")
+			}
+
+			// Simulate successful cleanup
+			fmt.Fprintf(&logOutput, "Tag cleanup completed successfully\n")
 			return nil
 		},
 	}
@@ -268,8 +288,215 @@ func TestCleanupCmdSuccessFlow(t *testing.T) {
 		"--script=/tmp/test.sh",
 		"--consul-addr=localhost:8500",
 		"--tag-prefix=test",
+		"--token=secret-token",
 	})
 
 	err := cmd.Execute()
 	assert.NoError(t, err)
+
+	// Verify the command would have executed with the right parameters
+	output := logOutput.String()
+	assert.Contains(t, output, "serviceID=test-service")
+	assert.Contains(t, output, "tagPrefix=test")
+	assert.Contains(t, output, "consulAddr=localhost:8500")
+	assert.Contains(t, output, "Using token authentication")
+	assert.Contains(t, output, "Tag cleanup completed successfully")
+}
+
+// MockConsulClient for testing
+type MockConsulClient struct {
+	MockAgent consul.Agent
+}
+
+func (m *MockConsulClient) Agent() consul.Agent {
+	return m.MockAgent
+}
+
+// MockAgent implements the Agent interface
+type MockAgent struct {
+	ServiceFunc         func(serviceID string, q *api.QueryOptions) (*api.AgentService, *api.QueryMeta, error)
+	ServiceRegisterFunc func(reg *api.AgentServiceRegistration) error
+}
+
+func (m *MockAgent) Service(serviceID string, q *api.QueryOptions) (*api.AgentService, *api.QueryMeta, error) {
+	if m.ServiceFunc != nil {
+		return m.ServiceFunc(serviceID, q)
+	}
+	return &api.AgentService{
+		ID:      "test-service",
+		Service: "test",
+		Tags:    []string{"tagged:old", "other-tag"},
+	}, nil, nil
+}
+
+func (m *MockAgent) ServiceRegister(reg *api.AgentServiceRegistration) error {
+	if m.ServiceRegisterFunc != nil {
+		return m.ServiceRegisterFunc(reg)
+	}
+	return nil
+}
+
+func TestCleanupCmdWithMockFactory(t *testing.T) {
+	// Save and restore the original factory
+	originalFactory := consul.Factory
+	defer func() {
+		consul.Factory = originalFactory
+	}()
+
+	t.Run("Successful cleanup with mock", func(t *testing.T) {
+		// Create a mock agent that simulates a service with tags
+		mockAgent := &MockAgent{
+			ServiceFunc: func(serviceID string, q *api.QueryOptions) (*api.AgentService, *api.QueryMeta, error) {
+				return &api.AgentService{
+					ID:      serviceID,
+					Service: "test",
+					Tags:    []string{"tagged-value1", "tagged-value2", "other-tag"},
+				}, nil, nil
+			},
+			ServiceRegisterFunc: func(reg *api.AgentServiceRegistration) error {
+				// Verify that the tags were cleaned up
+				assert.Equal(t, "test-service", reg.ID)
+				assert.NotContains(t, reg.Tags, "tagged-value1")
+				assert.NotContains(t, reg.Tags, "tagged-value2")
+				assert.Contains(t, reg.Tags, "other-tag")
+				return nil
+			},
+		}
+
+		// Create mock client with the mock agent
+		mockClient := &MockConsulClient{
+			MockAgent: mockAgent,
+		}
+
+		// Set up the mock factory
+		mockFactory := &consul.MockFactory{
+			MockClient: mockClient,
+		}
+		consul.SetFactory(mockFactory)
+
+		// Create a new command instance for this test
+		cmd := &cobra.Command{
+			Use:  "cleanup",
+			RunE: cleanupCmd.RunE,
+		}
+		// Set up parent command for flags inheritance
+		parent := &cobra.Command{}
+		parent.PersistentFlags().String("consul-addr", "127.0.0.1:8500", "")
+		parent.PersistentFlags().String("token", "", "")
+		parent.PersistentFlags().String("service-id", "test-service", "")
+		parent.PersistentFlags().String("tag-prefix", "tagged", "")
+		parent.AddCommand(cmd)
+
+		// Run the actual cleanup command
+		err := cmd.RunE(cmd, []string{})
+		assert.NoError(t, err)
+	})
+
+	t.Run("Cleanup with connection error", func(t *testing.T) {
+		// Set up a factory that returns an error
+		mockFactory := &consul.MockFactory{
+			MockError: fmt.Errorf("connection failed"),
+		}
+		consul.SetFactory(mockFactory)
+
+		// Create a new command instance for this test
+		cmd := &cobra.Command{
+			Use:  "cleanup",
+			RunE: cleanupCmd.RunE,
+		}
+		// Set up parent command for flags inheritance
+		parent := &cobra.Command{}
+		parent.PersistentFlags().String("consul-addr", "127.0.0.1:8500", "")
+		parent.PersistentFlags().String("token", "", "")
+		parent.PersistentFlags().String("service-id", "test-service", "")
+		parent.PersistentFlags().String("tag-prefix", "tagged", "")
+		parent.AddCommand(cmd)
+
+		// Run the cleanup command - should fail
+		err := cmd.RunE(cmd, []string{})
+		assert.Error(t, err)
+	})
+
+	t.Run("Cleanup with service not found", func(t *testing.T) {
+		// Create a mock agent that returns nil service
+		mockAgent := &MockAgent{
+			ServiceFunc: func(serviceID string, q *api.QueryOptions) (*api.AgentService, *api.QueryMeta, error) {
+				return nil, nil, nil
+			},
+		}
+
+		// Create mock client with the mock agent
+		mockClient := &MockConsulClient{
+			MockAgent: mockAgent,
+		}
+
+		// Set up the mock factory
+		mockFactory := &consul.MockFactory{
+			MockClient: mockClient,
+		}
+		consul.SetFactory(mockFactory)
+
+		// Create a new command instance for this test
+		cmd := &cobra.Command{
+			Use:  "cleanup",
+			RunE: cleanupCmd.RunE,
+		}
+		// Set up parent command for flags inheritance
+		parent := &cobra.Command{}
+		parent.PersistentFlags().String("consul-addr", "127.0.0.1:8500", "")
+		parent.PersistentFlags().String("token", "", "")
+		parent.PersistentFlags().String("service-id", "test-service", "")
+		parent.PersistentFlags().String("tag-prefix", "tagged", "")
+		parent.AddCommand(cmd)
+
+		// Run the cleanup command - should fail
+		err := cmd.RunE(cmd, []string{})
+		assert.Error(t, err)
+		assert.Contains(t, err.Error(), "service test-service not found")
+	})
+
+	t.Run("Cleanup with service register error", func(t *testing.T) {
+		// Create a mock agent that simulates a service with tags but fails on register
+		mockAgent := &MockAgent{
+			ServiceFunc: func(serviceID string, q *api.QueryOptions) (*api.AgentService, *api.QueryMeta, error) {
+				return &api.AgentService{
+					ID:      serviceID,
+					Service: "test",
+					Tags:    []string{"tagged-value1", "other-tag"},
+				}, nil, nil
+			},
+			ServiceRegisterFunc: func(reg *api.AgentServiceRegistration) error {
+				return fmt.Errorf("failed to register service")
+			},
+		}
+
+		// Create mock client with the mock agent
+		mockClient := &MockConsulClient{
+			MockAgent: mockAgent,
+		}
+
+		// Set up the mock factory
+		mockFactory := &consul.MockFactory{
+			MockClient: mockClient,
+		}
+		consul.SetFactory(mockFactory)
+
+		// Create a new command instance for this test
+		cmd := &cobra.Command{
+			Use:  "cleanup",
+			RunE: cleanupCmd.RunE,
+		}
+		// Set up parent command for flags inheritance
+		parent := &cobra.Command{}
+		parent.PersistentFlags().String("consul-addr", "127.0.0.1:8500", "")
+		parent.PersistentFlags().String("token", "", "")
+		parent.PersistentFlags().String("service-id", "test-service", "")
+		parent.PersistentFlags().String("tag-prefix", "tagged", "")
+		parent.AddCommand(cmd)
+
+		// Run the cleanup command - should fail
+		err := cmd.RunE(cmd, []string{})
+		assert.Error(t, err)
+		assert.Contains(t, err.Error(), "failed to clean up tags")
+	})
 }

cmd/run.go

@@ -24,7 +24,7 @@ import (
 	"syscall"
 	"time"
 
-	"github.com/hashicorp/consul/api"
+	"github.com/ncode/tagit/pkg/consul"
 	"github.com/ncode/tagit/pkg/tagit"
 	"github.com/spf13/cobra"
 )
@@ -59,22 +59,21 @@ example: tagit run -s my-super-service -x '/tmp/tag-role.sh'
 			return fmt.Errorf("invalid interval %q: %w", interval, err)
 		}
 
-		config := api.DefaultConfig()
-		config.Address, err = cmd.InheritedFlags().GetString("consul-addr")
+		consulAddr, err := cmd.InheritedFlags().GetString("consul-addr")
 		if err != nil {
 			logger.Error("Failed to get consul-addr flag", "error", err)
 			return err
 		}
-		config.Token, err = cmd.InheritedFlags().GetString("token")
+		token, err := cmd.InheritedFlags().GetString("token")
 		if err != nil {
 			logger.Error("Failed to get token flag", "error", err)
 			return err
 		}
 
-		consulClient, err := api.NewClient(config)
+		consulClient, err := consul.CreateClient(consulAddr, token)
 		if err != nil {
 			logger.Error("Failed to create Consul client", "error", err)
-			return fmt.Errorf("failed to create Consul client: %w", err)
+			return err
 		}
 
 		serviceID, err := cmd.InheritedFlags().GetString("service-id")
@@ -94,7 +93,7 @@ example: tagit run -s my-super-service -x '/tmp/tag-role.sh'
 		}
 
 		t := tagit.New(
-			tagit.NewConsulAPIWrapper(consulClient),
+			consulClient,
 			&tagit.CmdExecutor{},
 			serviceID,
 			script,