Cleanup unnecessary methods and files for k8s plugin

pkg/app/pipedv1/plugin/kubernetes/deployment/apply.go

@@ -17,10 +17,8 @@ package deployment
 import (
 	"context"
 	"errors"
-	"time"
 
 	"github.com/pipe-cd/pipecd/pkg/app/pipedv1/plugin/kubernetes/provider"
-	"github.com/pipe-cd/pipecd/pkg/plugin/logpersister"
 	"github.com/pipe-cd/pipecd/pkg/plugin/sdk"
 )
 
@@ -35,8 +33,7 @@ type applier interface {
 	ForceReplaceManifest(ctx context.Context, manifest provider.Manifest) error
 }
 
-// TODO: rewrite applyManifests with sdk.StageLogPersister
-func applyManifests(ctx context.Context, applier applier, manifests []provider.Manifest, namespace string, lp logpersister.StageLogPersister) error {
+func applyManifests(ctx context.Context, applier applier, manifests []provider.Manifest, namespace string, lp sdk.StageLogPersister) error {
 	if namespace == "" {
 		lp.Infof("Start applying %d manifests", len(manifests))
 	} else {
@@ -91,17 +88,3 @@ func applyManifests(ctx context.Context, applier applier, manifests []provider.M
 	lp.Successf("Successfully applied %d manifests", len(manifests))
 	return nil
 }
-
-// TODO: remove applyManifestsSDK and originalLogPersister after rewriting applyManifests
-func applyManifestsSDK(ctx context.Context, applier applier, manifests []provider.Manifest, namespace string, lp sdk.StageLogPersister) error {
-	originalLogPersister := &originalLogPersister{lp}
-	return applyManifests(ctx, applier, manifests, namespace, originalLogPersister)
-}
-
-type originalLogPersister struct {
-	sdk.StageLogPersister
-}
-
-func (lp *originalLogPersister) Complete(timeout time.Duration) error {
-	return nil
-}

pkg/app/pipedv1/plugin/kubernetes/deployment/apply_test.go

@@ -19,7 +19,6 @@ import (
 	"errors"
 	"fmt"
 	"testing"
-	"time"
 
 	"github.com/pipe-cd/pipecd/pkg/app/pipedv1/plugin/kubernetes/provider"
 )
@@ -58,11 +57,6 @@ func (m *mockStageLogPersister) Errorf(format string, a ...interface{}) {
 	m.logs = append(m.logs, fmt.Sprintf(format, a...))
 }
 
-func (m *mockStageLogPersister) Complete(timeout time.Duration) error {
-	m.completed = true
-	return nil
-}
-
 type mockApplier struct {
 	applyErr        error
 	forceReplaceErr error
@@ -86,7 +80,7 @@ func (m *mockApplier) CreateManifest(ctx context.Context, manifest provider.Mani
 	return m.createErr
 }
 
-func TestApplyManifests(t *testing.T) {
+func Test_applyManifests(t *testing.T) {
 	tests := []struct {
 		name      string
 		manifests []provider.Manifest
@@ -280,8 +274,10 @@ data:
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
 			lp := new(mockStageLogPersister)
-			err := applyManifests(context.Background(), tt.applier, tt.manifests, tt.namespace, lp)
+			err := applyManifests(t.Context(), tt.applier, tt.manifests, tt.namespace, lp)
 			if (err != nil) != tt.wantErr {
 				t.Errorf("applyManifests() error = %v, wantErr %v", err, tt.wantErr)
 			}

pkg/app/pipedv1/plugin/kubernetes/deployment/determine.go

@@ -23,7 +23,6 @@
 
 	"github.com/pipe-cd/pipecd/pkg/app/pipedv1/plugin/kubernetes/config"
 	"github.com/pipe-cd/pipecd/pkg/app/pipedv1/plugin/kubernetes/provider"
-	"github.com/pipe-cd/pipecd/pkg/model"
 	"github.com/pipe-cd/pipecd/pkg/plugin/diff"
 	"github.com/pipe-cd/pipecd/pkg/plugin/sdk"
 )
@@ -48,41 +47,22 @@
 
 // determineVersions decides artifact versions of an application.
 // It finds all container images that are being specified in the workload manifests then returns their names and tags.
-func determineVersions(manifests []provider.Manifest) []*model.ArtifactVersion {
+func determineVersions(manifests []provider.Manifest) []sdk.ArtifactVersion {
 	imageMap := map[string]struct{}{}
 	for _, m := range manifests {
 		for _, c := range provider.FindContainerImages(m) {
 			imageMap[c] = struct{}{}
 		}
 	}
 
-	versions := make([]*model.ArtifactVersion, 0, len(imageMap))
+	versions := make([]sdk.ArtifactVersion, 0, len(imageMap))
 	for i := range imageMap {
 		image := parseContainerImage(i)
-		versions = append(versions, &model.ArtifactVersion{
-			Kind:    model.ArtifactVersion_CONTAINER_IMAGE,
-			Version: image.tag,
-			Name:    image.name,
-			Url:     i,
-		})
-	}
-
-	return versions
-}
-
-// determineVersionsSDK decides artifact versions of an application.
-// It finds all container images that are being specified in the workload manifests then returns their names and tags.
-// TODO: rewrite this function to determineVersions after the current determineVersions is removed.
-func determineVersionsSDK(manifests []provider.Manifest) []sdk.ArtifactVersion {
-	values := determineVersions(manifests)
-
-	versions := make([]sdk.ArtifactVersion, 0, len(values))
-	for _, v := range values {
 		versions = append(versions, sdk.ArtifactVersion{
 			Kind:    sdk.ArtifactKindContainerImage,
-			Version: v.Version,
-			Name:    v.Name,
-			URL:     v.Url,
+			Version: image.tag,
+			Name:    image.name,
+			URL:     i,
 		})
 	}
 
@@ -176,16 +156,17 @@
 	return node.StringX(), node.StringY(), true
 }
 
+// determineStrategy decides the sync strategy and summary message based on the given manifests.
 // First up, checks to see if the workload's `spec.template` has been changed,
 // and then checks if the configmap/secret's data.
-func determineStrategy(olds, news []provider.Manifest, workloadRefs []config.K8sResourceReference, logger *zap.Logger) (strategy model.SyncStrategy, summary string) {
+func determineStrategy(olds, news []provider.Manifest, workloadRefs []config.K8sResourceReference, logger *zap.Logger) (strategy sdk.SyncStrategy, summary string) {
 	oldWorkloads := findWorkloadManifests(olds, workloadRefs)
 	if len(oldWorkloads) == 0 {
-		return model.SyncStrategy_QUICK_SYNC, "Quick sync by applying all manifests because it was unable to find the currently running workloads"
+		return sdk.SyncStrategyQuickSync, "Quick sync by applying all manifests because it was unable to find the currently running workloads"
 	}
 	newWorkloads := findWorkloadManifests(news, workloadRefs)
 	if len(newWorkloads) == 0 {
-		return model.SyncStrategy_QUICK_SYNC, "Quick sync by applying all manifests because it was unable to find workloads in the new manifests"
+		return sdk.SyncStrategyQuickSync, "Quick sync by applying all manifests because it was unable to find workloads in the new manifests"
 	}
 
 	workloads := provider.FindSameManifests(oldWorkloads, newWorkloads)
@@ -196,17 +177,17 @@
 		// do progressive deployment with the specified pipeline.
 		diffResult, err := provider.Diff(w.Old, w.New, logger)
 		if err != nil {
-			return model.SyncStrategy_PIPELINE, fmt.Sprintf("Sync progressively due to an error while calculating the diff (%v)", err)
+			return sdk.SyncStrategyPipelineSync, fmt.Sprintf("Sync progressively due to an error while calculating the diff (%v)", err)
 		}
 		diffNodes := diffResult.Nodes()
 		diffs[w.New.Key()] = diffNodes
 
 		templateDiffs := diffNodes.FindByPrefix("spec.template")
 		if len(templateDiffs) > 0 {
 			if msg, changed := checkImageChange(templateDiffs); changed {
-				return model.SyncStrategy_PIPELINE, msg
+				return sdk.SyncStrategyPipelineSync, msg
 			}
-			return model.SyncStrategy_PIPELINE, fmt.Sprintf("Sync progressively because pod template of workload %s was changed", w.New.Name())
+			return sdk.SyncStrategyPipelineSync, fmt.Sprintf("Sync progressively because pod template of workload %s was changed", w.New.Name())
 		}
 	}
 
@@ -215,22 +196,22 @@
 	oldConfigs := provider.FindConfigsAndSecrets(olds)
 	newConfigs := provider.FindConfigsAndSecrets(news)
 	if len(oldConfigs) > len(newConfigs) {
-		return model.SyncStrategy_PIPELINE, fmt.Sprintf("Sync progressively because %d configmap/secret deleted", len(oldConfigs)-len(newConfigs))
+		return sdk.SyncStrategyPipelineSync, fmt.Sprintf("Sync progressively because %d configmap/secret deleted", len(oldConfigs)-len(newConfigs))
 	}
 	if len(oldConfigs) < len(newConfigs) {
-		return model.SyncStrategy_PIPELINE, fmt.Sprintf("Sync progressively because new %d configmap/secret added", len(newConfigs)-len(oldConfigs))
+		return sdk.SyncStrategyPipelineSync, fmt.Sprintf("Sync progressively because new %d configmap/secret added", len(newConfigs)-len(oldConfigs))
 	}
 	for k, oc := range oldConfigs {
 		nc, ok := newConfigs[k]
 		if !ok {
-			return model.SyncStrategy_PIPELINE, fmt.Sprintf("Sync progressively because %s %s was deleted", oc.Kind(), oc.Name())
+			return sdk.SyncStrategyPipelineSync, fmt.Sprintf("Sync progressively because %s %s was deleted", oc.Kind(), oc.Name())
 		}
 		result, err := provider.Diff(oc, nc, logger)
 		if err != nil {
-			return model.SyncStrategy_PIPELINE, fmt.Sprintf("Sync progressively due to an error while calculating the diff (%v)", err)
+			return sdk.SyncStrategyPipelineSync, fmt.Sprintf("Sync progressively due to an error while calculating the diff (%v)", err)
 		}
 		if result.HasDiff() {
-			return model.SyncStrategy_PIPELINE, fmt.Sprintf("Sync progressively because %s %s was updated", oc.Kind(), oc.Name())
+			return sdk.SyncStrategyPipelineSync, fmt.Sprintf("Sync progressively because %s %s was updated", oc.Kind(), oc.Name())
 		}
 	}
 
@@ -244,24 +225,8 @@
 	}
 	if len(scales) > 0 {
 		slices.Sort(scales)
-		return model.SyncStrategy_QUICK_SYNC, fmt.Sprintf("Quick sync to scale %s", strings.Join(scales, ", "))
-	}
-
-	return model.SyncStrategy_QUICK_SYNC, "Quick sync by applying all manifests"
-}
-
-// determineStrategySDK decides the sync strategy and summary message based on the given manifests.
-// TODO: rewrite this function to determineStrategy after the current determineStrategy is removed.
-func determineStrategySDK(olds, news []provider.Manifest, workloadRefs []config.K8sResourceReference, logger *zap.Logger) (strategy sdk.SyncStrategy, summary string) {
-	mStrategy, summary := determineStrategy(olds, news, workloadRefs, logger)
-
-	var s sdk.SyncStrategy
-	switch mStrategy {
-	case model.SyncStrategy_QUICK_SYNC:
-		s = sdk.SyncStrategyQuickSync
-	case model.SyncStrategy_PIPELINE:
-		s = sdk.SyncStrategyQuickSync
+		return sdk.SyncStrategyQuickSync, fmt.Sprintf("Quick sync to scale %s", strings.Join(scales, ", "))
 	}
 
-	return s, summary
+	return sdk.SyncStrategyQuickSync, "Quick sync by applying all manifests"
 }