Sd 854 parallelize fetching of argocd diff in telefonistka (#43)

* wip * Concurrent argocd diff * Move InitArgoClients out of server.go * Update internal/pkg/githubapi/github.go Co-authored-by: Hannes Gustafsson <[email protected]> * Update internal/pkg/argocd/argocd_test.go Co-authored-by: Hannes Gustafsson <[email protected]> * Update internal/pkg/argocd/argocd.go Co-authored-by: Hannes Gustafsson <[email protected]> * Update internal/pkg/argocd/argocd_test.go Co-authored-by: Hannes Gustafsson <[email protected]> * Update internal/pkg/argocd/argocd.go Co-authored-by: Hannes Gustafsson <[email protected]> * Update internal/pkg/argocd/argocd.go Co-authored-by: Hannes Gustafsson <[email protected]> * Update internal/pkg/argocd/argocd.go Co-authored-by: Hannes Gustafsson <[email protected]> * Use dependency injection with argocd client * Remove dangling err check --------- Co-authored-by: Hannes Gustafsson <[email protected]>
commercetools · Dec 5, 2024 · a77da6a · a77da6a
1 parent 8ded097
commit a77da6a
Show file tree

Hide file tree

Showing 5 changed files with 149 additions and 17 deletions.
diff --git a/.gitignore b/.gitignore
@@ -1,2 +1,4 @@
 /telefonistka
 vendor/
+internal/pkg/mocks/argocd_settings.go
+internal/pkg/mocks/argocd_project.go
diff --git a/internal/pkg/argocd/argocd.go b/internal/pkg/argocd/argocd.go
@@ -172,7 +172,7 @@ func getEnv(key, fallback string) string {
 	return fallback
 }
 
-func createArgoCdClients() (ac argoCdClients, err error) {
+func CreateArgoCdClients() (ac argoCdClients, err error) {
 	plaintext, _ := strconv.ParseBool(getEnv("ARGOCD_PLAINTEXT", "false"))
 	insecure, _ := strconv.ParseBool(getEnv("ARGOCD_INSECURE", "false"))
 
@@ -320,7 +320,7 @@ func findArgocdApp(ctx context.Context, componentPath string, repo string, appCl
 func SetArgoCDAppRevision(ctx context.Context, componentPath string, revision string, repo string, useSHALabelForArgoDicovery bool) error {
 	var foundApp *argoappv1.Application
 	var err error
-	ac, err := createArgoCdClients()
+	ac, err := CreateArgoCdClients()
 	if err != nil {
 		return fmt.Errorf("Error creating ArgoCD clients: %w", err)
 	}
@@ -461,7 +461,7 @@ func generateDiffOfAComponent(ctx context.Context, commentDiff bool, componentPa
 				componentDiffResult.AppWasTemporarilyCreated = true
 			}
 		} else {
-			componentDiffResult.DiffError = fmt.Errorf("No ArgoCD application found for component path %s(repo %s)", componentPath, repo)
+			componentDiffResult.DiffError = fmt.Errorf("no ArgoCD application found for component path %s(repo %s)", componentPath, repo)
 			return
 		}
 	} else {
@@ -472,7 +472,7 @@ func generateDiffOfAComponent(ctx context.Context, commentDiff bool, componentPa
 			Name:    &app.Name, // we expect only one app with this label and repo selectors
 			Refresh: &refreshType,
 		}
-		app, err := ac.app.Get(ctx, &appNameQuery)
+		app, err = ac.app.Get(ctx, &appNameQuery)
 		if err != nil {
 			componentDiffResult.DiffError = err
 			log.Errorf("Error getting app(HardRefresh) %v: %v", appNameQuery.Name, err)
@@ -544,26 +544,27 @@ func generateDiffOfAComponent(ctx context.Context, commentDiff bool, componentPa
 }
 
 // GenerateDiffOfChangedComponents generates diff of changed components
-func GenerateDiffOfChangedComponents(ctx context.Context, componentsToDiff map[string]bool, prBranch string, repo string, useSHALabelForArgoDicovery bool, createTempAppObjectFromNewApps bool) (hasComponentDiff bool, hasComponentDiffErrors bool, diffResults []DiffResult, err error) {
+func GenerateDiffOfChangedComponents(ctx context.Context, componentsToDiff map[string]bool, prBranch string, repo string, useSHALabelForArgoDicovery bool, createTempAppObjectFromNewApps bool, argoClients argoCdClients) (hasComponentDiff bool, hasComponentDiffErrors bool, diffResults []DiffResult, err error) {
 	hasComponentDiff = false
 	hasComponentDiffErrors = false
-	// env var should be centralized
-	ac, err := createArgoCdClients()
-	if err != nil {
-		log.Errorf("Error creating ArgoCD clients: %v", err)
-		return false, true, nil, err
-	}
 
-	argoSettings, err := ac.setting.Get(ctx, &settings.SettingsQuery{})
+	argoSettings, err := argoClients.setting.Get(ctx, &settings.SettingsQuery{})
 	if err != nil {
-		log.Errorf("Error getting ArgoCD settings: %v", err)
+		log.Errorf("error getting ArgoCD settings: %v", err)
 		return false, true, nil, err
 	}
 
+	diffResult := make(chan DiffResult)
 	for componentPath, shouldIDiff := range componentsToDiff {
-		currentDiffResult := generateDiffOfAComponent(ctx, shouldIDiff, componentPath, prBranch, repo, ac, argoSettings, useSHALabelForArgoDicovery, createTempAppObjectFromNewApps)
+		go func(componentPath string, shouldDiff bool) {
+			diffResult <- generateDiffOfAComponent(ctx, shouldIDiff, componentPath, prBranch, repo, argoClients, argoSettings, useSHALabelForArgoDicovery, createTempAppObjectFromNewApps)
+		}(componentPath, shouldIDiff)
+	}
+
+	for range componentsToDiff {
+		currentDiffResult := <-diffResult
 		if currentDiffResult.DiffError != nil {
-			log.Errorf("Error generating diff for component %s: %v", componentPath, currentDiffResult.DiffError)
+			log.Errorf("Error generating diff for component %s: %v", currentDiffResult.ComponentPath, currentDiffResult.DiffError)
 			hasComponentDiffErrors = true
 			err = currentDiffResult.DiffError
 		}
@@ -572,6 +573,5 @@ func GenerateDiffOfChangedComponents(ctx context.Context, componentsToDiff map[s
 		}
 		diffResults = append(diffResults, currentDiffResult)
 	}
-
 	return hasComponentDiff, hasComponentDiffErrors, diffResults, err
 }
diff --git a/internal/pkg/argocd/argocd_test.go b/internal/pkg/argocd/argocd_test.go
@@ -3,14 +3,21 @@ package argocd
 import (
 	"bytes"
 	"context"
+	"fmt"
 	"log"
 	"os"
 	"strings"
 	"testing"
 	"text/template"
+	"time"
 
+	"github.com/argoproj/argo-cd/v2/pkg/apiclient/application"
+	"github.com/argoproj/argo-cd/v2/pkg/apiclient/project"
+	"github.com/argoproj/argo-cd/v2/pkg/apiclient/settings"
 	argoappv1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
+	reposerverApiClient "github.com/argoproj/argo-cd/v2/reposerver/apiclient"
 	"github.com/golang/mock/gomock"
+	"github.com/stretchr/testify/assert"
 	"github.com/wayfair-incubator/telefonistka/internal/pkg/mocks"
 	"github.com/wayfair-incubator/telefonistka/internal/pkg/testutils"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -311,3 +318,117 @@ func TestFindArgocdAppByPathAnnotationNotFound(t *testing.T) {
 		log.Fatal("expected the application to be nil")
 	}
 }
+
+func TestFetchArgoDiffConcurrently(t *testing.T) {
+	t.Parallel()
+	// MockApplicationServiceClient
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+
+	// mock the argoClients
+	mockAppServiceClient := mocks.NewMockApplicationServiceClient(mockCtrl)
+	mockSettingsServiceClient := mocks.NewMockSettingsServiceClient(mockCtrl)
+	mockProjectServiceClient := mocks.NewMockProjectServiceClient(mockCtrl)
+	// fake InitArgoClients
+
+	argoClients := argoCdClients{
+		app:     mockAppServiceClient,
+		setting: mockSettingsServiceClient,
+		project: mockProjectServiceClient,
+	}
+	// slowReply simulates a slow reply from the server
+	slowReply := func(ctx context.Context, in any, opts ...any) {
+		time.Sleep(time.Second)
+	}
+
+	// makeComponents for test
+	makeComponents := func(num int) map[string]bool {
+		components := make(map[string]bool, num)
+		for i := 0; i < num; i++ {
+			components[fmt.Sprintf("component/to/diff/%d", i)] = true
+		}
+		return components
+	}
+
+	mockSettingsServiceClient.EXPECT().
+		Get(gomock.Any(), gomock.Any()).
+		Return(&settings.Settings{
+			URL: "https://test-argocd.test.test",
+		}, nil)
+	// mock the List method
+	mockAppServiceClient.EXPECT().
+		List(gomock.Any(), gomock.Any(), gomock.Any()).
+		Return(&argoappv1.ApplicationList{
+			Items: []argoappv1.Application{
+				{
+					TypeMeta:   metav1.TypeMeta{},
+					ObjectMeta: metav1.ObjectMeta{},
+					Spec:       argoappv1.ApplicationSpec{},
+					Status:     argoappv1.ApplicationStatus{},
+					Operation:  &argoappv1.Operation{},
+				},
+			},
+		}, nil).
+		AnyTimes().
+		Do(slowReply) // simulate slow reply
+
+	// mock the Get method
+	mockAppServiceClient.EXPECT().
+		Get(gomock.Any(), gomock.Any()).
+		Return(&argoappv1.Application{
+			TypeMeta: metav1.TypeMeta{},
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "test-app",
+			},
+			Spec: argoappv1.ApplicationSpec{
+				Source: &argoappv1.ApplicationSource{
+					TargetRevision: "test-revision",
+				},
+				SyncPolicy: &argoappv1.SyncPolicy{
+					Automated: &argoappv1.SyncPolicyAutomated{},
+				},
+			},
+			Status:    argoappv1.ApplicationStatus{},
+			Operation: &argoappv1.Operation{},
+		}, nil).
+		AnyTimes()
+
+	// mock managedResource
+	mockAppServiceClient.EXPECT().
+		ManagedResources(gomock.Any(), gomock.Any()).
+		Return(&application.ManagedResourcesResponse{}, nil).
+		AnyTimes()
+
+	// mock the GetManifests method
+	mockAppServiceClient.EXPECT().
+		GetManifests(gomock.Any(), gomock.Any()).
+		Return(&reposerverApiClient.ManifestResponse{}, nil).
+		AnyTimes()
+
+	// mock the GetDetailedProject method
+	mockProjectServiceClient.EXPECT().
+		GetDetailedProject(gomock.Any(), gomock.Any()).
+		Return(&project.DetailedProjectsResponse{}, nil).
+		AnyTimes()
+
+	const numComponents = 5
+	// start timer
+	start := time.Now()
+
+	// TODO: Test all the return values, for now we will just ignore the linter.
+	_, _, diffResults, _ := GenerateDiffOfChangedComponents( //nolint:dogsled
+		context.TODO(),
+		makeComponents(numComponents),
+		"test-pr-branch",
+		"test-repo",
+		true,
+		false,
+		argoClients,
+	)
+
+	// stop timer
+	elapsed := time.Since(start)
+	assert.Equal(t, numComponents, len(diffResults))
+	// assert that the entire run takes less than numComponents * 1 second
+	assert.Less(t, elapsed, time.Duration(numComponents)*time.Second)
+}
diff --git a/internal/pkg/githubapi/github.go b/internal/pkg/githubapi/github.go
@@ -224,7 +224,12 @@ func handleChangedPREvent(ctx context.Context, mainGithubClientPair GhClientPair
 				ghPrClientDetails.PrLogger.Debugf("ArgoCD diff disabled for %s\n", componentPath)
 			}
 		}
-		hasComponentDiff, hasComponentDiffErrors, diffOfChangedComponents, err := argocd.GenerateDiffOfChangedComponents(ctx, componentsToDiff, ghPrClientDetails.Ref, ghPrClientDetails.RepoURL, config.Argocd.UseSHALabelForAppDiscovery, config.Argocd.CreateTempAppObjectFroNewApps)
+		argoClients, err := argocd.CreateArgoCdClients()
+		if err != nil {
+			return fmt.Errorf("error creating ArgoCD clients: %w", err)
+		}
+
+		hasComponentDiff, hasComponentDiffErrors, diffOfChangedComponents, err := argocd.GenerateDiffOfChangedComponents(ctx, componentsToDiff, ghPrClientDetails.Ref, ghPrClientDetails.RepoURL, config.Argocd.UseSHALabelForAppDiscovery, config.Argocd.CreateTempAppObjectFroNewApps, argoClients)
 		if err != nil {
 			return fmt.Errorf("getting diff information: %w", err)
 		}

diff --git a/internal/pkg/mocks/mocks.go b/internal/pkg/mocks/mocks.go
@@ -3,3 +3,7 @@ package mocks
 // This package contains generated mocks
 
 //go:generate go run github.com/golang/mock/[email protected] -destination=argocd_application.go -package=mocks github.com/argoproj/argo-cd/v2/pkg/apiclient/application ApplicationServiceClient
+
+//go:generate go run github.com/golang/mock/[email protected] -destination=argocd_settings.go -package=mocks github.com/argoproj/argo-cd/v2/pkg/apiclient/settings SettingsServiceClient
+
+//go:generate go run github.com/golang/mock/[email protected] -destination=argocd_project.go -package=mocks github.com/argoproj/argo-cd/v2/pkg/apiclient/project ProjectServiceClient