[Sqldef Pluging] refactor(sqldef) refactor sqldef toolRegistry download binary (#36)

* refactor(sqldef) refactor sqldef toolRegistry download binary

Signed-off-by: kadai0308 <kadai0308@gmail.com>

* refactor(sqldef) remove binary path cache from stage_plan

Signed-off-by: kadai0308 <kadai0308@gmail.com>

* refactor(sqldef) fix deployment/stage_plan_test.go

Signed-off-by: kadai0308 <kadai0308@gmail.com>

* refactor(sqldef) remove useless comment and add TODO to sqldef registry_test

Signed-off-by: kadai0308 <kadai0308@gmail.com>

---------

Signed-off-by: kadai0308 <kadai0308@gmail.com>

Author: kadai0308

plugins/sqldef/deployment/stage_plan.go

@@ -2,7 +2,6 @@ package deployment
 
 import (
 	"context"
-	"fmt"
 	"github.com/pipe-cd/community-plugins/plugins/sqldef/config"
 	toolRegistryPkg "github.com/pipe-cd/community-plugins/plugins/sqldef/toolregistry"
 
@@ -16,9 +15,6 @@ func (p *Plugin) executePlanStage(ctx context.Context, dts []*sdk.DeployTarget[c
 	// Currently, we create them every time the stage is executed beucause we can't pass input.Client.toolRegistry to the plugin when starting the plugin.
 	toolRegistry := toolRegistryPkg.NewRegistry(input.Client.ToolRegistry())
 
-	// map for prevent repeatedly download sqldef
-	downloadedSqldefPaths := make(map[config.DBType]string)
-
 	for _, dt := range dts {
 		lp.Infof("Deploy Target [%s]: host=%s, port=%s, db=%s, schemaFile=%s",
 			dt.Name,
@@ -27,26 +23,10 @@ func (p *Plugin) executePlanStage(ctx context.Context, dts []*sdk.DeployTarget[c
 			dt.Config.DBName,
 		)
 
-		// check db_type from dt.config to choose which sqldef binary to download
-		// Now we only support mysql temporarily
-		var sqlDefPath string
-		var existed bool
-		if sqlDefPath, existed = downloadedSqldefPaths[config.DBTypeMySQL]; !existed {
-			var binPath string
-			var err error
-			switch dt.Config.DbType {
-			case config.DBTypeMySQL:
-				binPath, err = toolRegistry.Mysqldef(ctx, "")
-			default:
-				lp.Errorf(fmt.Sprintf("Unsupported database type: %s, currently only support: mysql", dt.Name))
-				return sdk.StageStatusFailure
-			}
-			if err != nil {
-				lp.Errorf("Failed while getting Sqldef tool (%v)", err)
-				return sdk.StageStatusFailure
-			}
-			downloadedSqldefPaths[dt.Config.DbType] = binPath
-			lp.Info(fmt.Sprintf("Sqldef binary downloadeded: %s", sqlDefPath))
+		sqlDefPath, err := toolRegistry.DownloadBinary(ctx, dt.Config.DbType, "")
+		if err != nil {
+			lp.Errorf("Failed while getting Sqldef tool (%v)", err)
+			return sdk.StageStatusFailure
 		}
 
 		appDir := input.Request.RunningDeploymentSource.ApplicationDirectory

plugins/sqldef/deployment/stage_plan_test.go

@@ -171,7 +171,7 @@ func TestPlugin_executePlanStage_EmptySchemaFileName(t *testing.T) {
 		"3306",
 		"testdb",
 		"testdata/schema.sql",
-		"",
+		mock.AnythingOfType("string"), // execPath from tool registry
 	).Return(nil)
 
 	mockSqldef.On("Execute", mock.Anything, true).Return(errors.New("execute failed."))

plugins/sqldef/toolregistry/registry.go

@@ -19,6 +19,9 @@ package toolregistry
 import (
 	"cmp"
 	"context"
+	"fmt"
+
+	"github.com/pipe-cd/community-plugins/plugins/sqldef/config"
 )
 
 const (
@@ -40,8 +43,13 @@ type Registry struct {
 	client client
 }
 
-// Mysqldef installs the mysqldef tool with the given version and return the path to the installed binary.
-// If the version is empty, the default version will be used.
-func (r *Registry) Mysqldef(ctx context.Context, version string) (string, error) {
-	return r.client.InstallTool(ctx, "mysqldef", cmp.Or(version, defaultSqldefVersion), MysqldefInstallScript)
+// DownloadBinary downloads the appropriate sqldef binary based on the database type.
+// Currently only supports MySQL, but can be extended for other database types.
+func (r *Registry) DownloadBinary(ctx context.Context, dbType config.DBType, version string) (string, error) {
+	switch dbType {
+	case config.DBTypeMySQL:
+		return r.client.InstallTool(ctx, "mysqldef", cmp.Or(version, defaultSqldefVersion), MysqldefInstallScript)
+	default:
+		return "", fmt.Errorf("unsupported database type: %s, currently only support: mysql", dbType)
+	}
 }

plugins/sqldef/toolregistry/registry_test.go

@@ -18,22 +18,86 @@ import (
 	"context"
 	"testing"
 
+	"github.com/pipe-cd/community-plugins/plugins/sqldef/config"
 	"github.com/pipe-cd/piped-plugin-sdk-go/toolregistry/toolregistrytest"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
-func TestRegistry_Sqldef(t *testing.T) {
+func TestRegistry_DownloadBinary(t *testing.T) {
 	t.Parallel()
 
-	c := toolregistrytest.NewTestToolRegistry(t)
+	tests := []struct {
+		name        string
+		dbType      config.DBType
+		version     string
+		expectError bool
+		errorMsg    string
+	}{
+		{
+			name:        "MySQL_with_specific_version",
+			dbType:      config.DBTypeMySQL,
+			version:     "2.0.4",
+			expectError: false,
+		},
+		{
+			name:        "MySQL_with_empty_version_should_use_default",
+			dbType:      config.DBTypeMySQL,
+			version:     "",
+			expectError: false,
+		},
+		{
+			name:        "PostgreSQL_unsupported",
+			dbType:      config.DBTypePostgres,
+			version:     "2.0.4",
+			expectError: true,
+			errorMsg:    "unsupported database type: psql, currently only support: mysql",
+		},
+		{
+			name:        "SQLite_unsupported",
+			dbType:      config.DBTypeSQLite,
+			version:     "2.0.4",
+			expectError: true,
+			errorMsg:    "unsupported database type: sqlite, currently only support: mysql",
+		},
+		{
+			name:        "MSSQL_unsupported",
+			dbType:      config.DBTypeMSSQL,
+			version:     "2.0.4",
+			expectError: true,
+			errorMsg:    "unsupported database type: mssql, currently only support: mysql",
+		},
+	}
 
-	r := NewRegistry(c)
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			c := toolregistrytest.NewTestToolRegistry(t)
+			r := NewRegistry(c)
 
-	path, err := r.Mysqldef(context.Background(), "2.0.4")
+			path, err := r.DownloadBinary(context.Background(), tt.dbType, tt.version)
 
-	assert.NoError(t, err)
-	require.NotEmpty(t, path)
+			if tt.expectError {
+				assert.Error(t, err)
+				assert.Equal(t, tt.errorMsg, err.Error())
+				assert.Empty(t, path)
+			} else {
+				assert.NoError(t, err)
+				require.NotEmpty(t, path)
 
-	assert.Contains(t, path, "mysqldef-2.0.4", "Expected file title not found in path")
+				// For MySQL, verify the path contains the expected binary name
+				if tt.dbType == config.DBTypeMySQL {
+					assert.Contains(t, path, "mysqldef", "Expected mysqldef binary name in path")
+
+					// If version was provided, check it's in the path
+					if tt.version != "" {
+						assert.Contains(t, path, tt.version, "Expected version in path")
+					} else {
+						// If no version provided, should use default
+						assert.Contains(t, path, defaultSqldefVersion, "Expected default version in path")
+					}
+				}
+				// TODO: Add more dbType cases when supported
+			}
+		})
+	}
 }