From d76b09c1c396bb7c407c6bcd1f53f813b7a7c531 Mon Sep 17 00:00:00 2001 From: Arthur Shvarts Date: Thu, 28 Nov 2024 20:40:20 -0500 Subject: [PATCH 01/13] wip --- .gitignore | 1 + cmd/telefonistka/server.go | 7 ++++ internal/pkg/argocd/argocd.go | 21 ++++++----- internal/pkg/argocd/argocd_test.go | 57 ++++++++++++++++++++++++++++++ internal/pkg/mocks/mocks.go | 1 + 5 files changed, 79 insertions(+), 8 deletions(-) diff --git a/.gitignore b/.gitignore index 386d5d7d..78970be6 100644 --- a/.gitignore +++ b/.gitignore @@ -1,2 +1,3 @@ /telefonistka vendor/ +internal/pkg/mocks/argocd_settings.go diff --git a/cmd/telefonistka/server.go b/cmd/telefonistka/server.go index 076b28f8..e346b346 100644 --- a/cmd/telefonistka/server.go +++ b/cmd/telefonistka/server.go @@ -10,6 +10,7 @@ import ( "github.com/prometheus/client_golang/prometheus/promhttp" log "github.com/sirupsen/logrus" "github.com/spf13/cobra" + "github.com/wayfair-incubator/telefonistka/internal/pkg/argocd" "github.com/wayfair-incubator/telefonistka/internal/pkg/githubapi" ) @@ -57,6 +58,12 @@ func serve() { mainGhClientCache, _ := lru.New[string, githubapi.GhClientPair](128) prApproverGhClientCache, _ := lru.New[string, githubapi.GhClientPair](128) + // init argoclients + err := argocd.InitArgoClients() + if err != nil { + log.Fatalf("error initializing argo clients: %v", err) + } + mux := http.NewServeMux() mux.HandleFunc("/webhook", handleWebhook(githubWebhookSecret, mainGhClientCache, prApproverGhClientCache)) mux.Handle("/metrics", promhttp.Handler()) diff --git a/internal/pkg/argocd/argocd.go b/internal/pkg/argocd/argocd.go index 5038521b..eae8ebdb 100644 --- a/internal/pkg/argocd/argocd.go +++ b/internal/pkg/argocd/argocd.go @@ -33,6 +33,17 @@ import ( // ctxLines is the number of context lines used in application diffs. const ctxLines = 10 +var argoClients argoCdClients // replaced during tests + +func InitArgoClients() error { + var err error + argoClients, err = createArgoCdClients() + if err != nil { + return fmt.Errorf("error creating ArgoCD clients: %w", err) + } + return nil +} + type argoCdClients struct { app application.ApplicationServiceClient project projectpkg.ProjectServiceClient @@ -547,21 +558,15 @@ func generateDiffOfAComponent(ctx context.Context, commentDiff bool, componentPa 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) { 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) return false, true, nil, err } for componentPath, shouldIDiff := range componentsToDiff { - currentDiffResult := generateDiffOfAComponent(ctx, shouldIDiff, componentPath, prBranch, repo, ac, argoSettings, useSHALabelForArgoDicovery, createTempAppObjectFromNewApps) + currentDiffResult := generateDiffOfAComponent(ctx, shouldIDiff, componentPath, prBranch, repo, argoClients, argoSettings, useSHALabelForArgoDicovery, createTempAppObjectFromNewApps) if currentDiffResult.DiffError != nil { log.Errorf("Error generating diff for component %s: %v", componentPath, currentDiffResult.DiffError) hasComponentDiffErrors = true diff --git a/internal/pkg/argocd/argocd_test.go b/internal/pkg/argocd/argocd_test.go index 80e094d9..e9eb3dc9 100644 --- a/internal/pkg/argocd/argocd_test.go +++ b/internal/pkg/argocd/argocd_test.go @@ -3,11 +3,13 @@ package argocd import ( "bytes" "context" + "fmt" "log" "os" "strings" "testing" "text/template" + "time" argoappv1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" "github.com/golang/mock/gomock" @@ -311,3 +313,58 @@ func TestFindArgocdAppByPathAnnotationNotFound(t *testing.T) { log.Fatal("expected the application to be nil") } } + +func TestFetchArgoDiffConcurrently(t *testing.T) { + // MockApplicationServiceClient + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + + // Save and restore the original argoClients + saved := argoClients + defer func() { argoClients = saved }() + + // mock the argoClients + mockAppServiceClient := mocks.NewMockApplicationServiceClient(mockCtrl) + mockSettingsServiceClient := mocks.NewMockSettingsServiceClient(mockCtrl) + argoClients = argoCdClients{ + app: mockAppServiceClient, + setting: mockSettingsServiceClient, + } + + // slowReply simulates a slow reply from the server + slowReply := func(ctx context.Context, in any, opts ...any) { + time.Sleep(1 * time.Second) + } + + // makeComponents + 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(nil, nil) + mockAppServiceClient.EXPECT(). + List(gomock.Any(), gomock.Any(), gomock.Any()). + Return(&argoappv1.ApplicationList{}, nil). + AnyTimes(). + Do(slowReply) + // type argoCdClients struct { + // app application.ApplicationServiceClient + // project projectpkg.ProjectServiceClient + // setting settings.SettingsServiceClient + // appSet applicationsetpkg.ApplicationSetServiceClient + // } + + GenerateDiffOfChangedComponents( + context.TODO(), + makeComponents(5), + "test-pr-branch", + "test-repo", + true, + false, + ) + +} diff --git a/internal/pkg/mocks/mocks.go b/internal/pkg/mocks/mocks.go index 0fbed2e1..c0999b9b 100644 --- a/internal/pkg/mocks/mocks.go +++ b/internal/pkg/mocks/mocks.go @@ -3,3 +3,4 @@ package mocks // This package contains generated mocks //go:generate go run github.com/golang/mock/mockgen@v1.6.0 -destination=argocd_application.go -package=mocks github.com/argoproj/argo-cd/v2/pkg/apiclient/application ApplicationServiceClient +//go:generate go run github.com/golang/mock/mockgen@v1.6.0 -destination=argocd_settings.go -package=mocks github.com/argoproj/argo-cd/v2/pkg/apiclient/settings SettingsServiceClient From 762a1a83c4952f69ee01c15e79dbe725e8751068 Mon Sep 17 00:00:00 2001 From: Arthur Shvarts Date: Tue, 3 Dec 2024 11:02:37 -0500 Subject: [PATCH 02/13] Concurrent argocd diff --- .gitignore | 1 + internal/pkg/argocd/argocd.go | 16 ++++-- internal/pkg/argocd/argocd_test.go | 88 +++++++++++++++++++++++++++--- internal/pkg/mocks/mocks.go | 3 + 4 files changed, 95 insertions(+), 13 deletions(-) diff --git a/.gitignore b/.gitignore index 78970be6..eaa026f9 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +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 index eae8ebdb..5593f02f 100644 --- a/internal/pkg/argocd/argocd.go +++ b/internal/pkg/argocd/argocd.go @@ -472,7 +472,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 { @@ -483,7 +483,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) @@ -565,10 +565,17 @@ func GenerateDiffOfChangedComponents(ctx context.Context, componentsToDiff map[s return false, true, nil, err } + diffResult := make(chan DiffResult, len(componentsToDiff)) for componentPath, shouldIDiff := range componentsToDiff { - currentDiffResult := generateDiffOfAComponent(ctx, shouldIDiff, componentPath, prBranch, repo, argoClients, 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 } @@ -577,6 +584,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 index e9eb3dc9..490cfc71 100644 --- a/internal/pkg/argocd/argocd_test.go +++ b/internal/pkg/argocd/argocd_test.go @@ -11,8 +11,13 @@ import ( "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" @@ -315,6 +320,7 @@ func TestFindArgocdAppByPathAnnotationNotFound(t *testing.T) { } func TestFetchArgoDiffConcurrently(t *testing.T) { + t.Parallel() // MockApplicationServiceClient mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() @@ -326,17 +332,20 @@ func TestFetchArgoDiffConcurrently(t *testing.T) { // mock the argoClients mockAppServiceClient := mocks.NewMockApplicationServiceClient(mockCtrl) mockSettingsServiceClient := mocks.NewMockSettingsServiceClient(mockCtrl) + mockProjectServiceClient := mocks.NewMockProjectServiceClient(mockCtrl) + 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(1 * time.Second) + time.Sleep(time.Second) } - // makeComponents + // makeComponents for test makeComponents := func(num int) map[string]bool { components := make(map[string]bool, num) for i := 0; i < num; i++ { @@ -345,26 +354,89 @@ func TestFetchArgoDiffConcurrently(t *testing.T) { return components } - mockSettingsServiceClient.EXPECT().Get(gomock.Any(), gomock.Any()).Return(nil, nil) + 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{}, nil). + 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) - // type argoCdClients struct { + 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() // type argoCdClients struct { // app application.ApplicationServiceClient // project projectpkg.ProjectServiceClient // setting settings.SettingsServiceClient // appSet applicationsetpkg.ApplicationSetServiceClient // } - GenerateDiffOfChangedComponents( + 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(5), + makeComponents(numComponents), "test-pr-branch", "test-repo", true, false, ) + // 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/mocks/mocks.go b/internal/pkg/mocks/mocks.go index c0999b9b..42b1e1e6 100644 --- a/internal/pkg/mocks/mocks.go +++ b/internal/pkg/mocks/mocks.go @@ -3,4 +3,7 @@ package mocks // This package contains generated mocks //go:generate go run github.com/golang/mock/mockgen@v1.6.0 -destination=argocd_application.go -package=mocks github.com/argoproj/argo-cd/v2/pkg/apiclient/application ApplicationServiceClient + //go:generate go run github.com/golang/mock/mockgen@v1.6.0 -destination=argocd_settings.go -package=mocks github.com/argoproj/argo-cd/v2/pkg/apiclient/settings SettingsServiceClient + +//go:generate go run github.com/golang/mock/mockgen@v1.6.0 -destination=argocd_project.go -package=mocks github.com/argoproj/argo-cd/v2/pkg/apiclient/project ProjectServiceClient From 380eb67fa3e7918c56f3a2bec93d825af9157ef2 Mon Sep 17 00:00:00 2001 From: Arthur Shvarts Date: Wed, 4 Dec 2024 10:10:23 -0500 Subject: [PATCH 03/13] Remove dead code --- internal/pkg/argocd/argocd_test.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/internal/pkg/argocd/argocd_test.go b/internal/pkg/argocd/argocd_test.go index 490cfc71..798d3a79 100644 --- a/internal/pkg/argocd/argocd_test.go +++ b/internal/pkg/argocd/argocd_test.go @@ -413,12 +413,7 @@ func TestFetchArgoDiffConcurrently(t *testing.T) { mockProjectServiceClient.EXPECT(). GetDetailedProject(gomock.Any(), gomock.Any()). Return(&project.DetailedProjectsResponse{}, nil). - AnyTimes() // type argoCdClients struct { - // app application.ApplicationServiceClient - // project projectpkg.ProjectServiceClient - // setting settings.SettingsServiceClient - // appSet applicationsetpkg.ApplicationSetServiceClient - // } + AnyTimes() const numComponents = 5 // start timer From 594a039304abadab74c4333506751c01751c82dd Mon Sep 17 00:00:00 2001 From: Arthur Shvarts Date: Wed, 4 Dec 2024 11:28:16 -0500 Subject: [PATCH 04/13] Move InitArgoClients out of server.go --- cmd/telefonistka/server.go | 7 ------- internal/pkg/argocd/argocd.go | 19 ++++++++++++------- internal/pkg/argocd/argocd_test.go | 17 ++++++++++------- internal/pkg/githubapi/github.go | 1 + 4 files changed, 23 insertions(+), 21 deletions(-) diff --git a/cmd/telefonistka/server.go b/cmd/telefonistka/server.go index e346b346..076b28f8 100644 --- a/cmd/telefonistka/server.go +++ b/cmd/telefonistka/server.go @@ -10,7 +10,6 @@ import ( "github.com/prometheus/client_golang/prometheus/promhttp" log "github.com/sirupsen/logrus" "github.com/spf13/cobra" - "github.com/wayfair-incubator/telefonistka/internal/pkg/argocd" "github.com/wayfair-incubator/telefonistka/internal/pkg/githubapi" ) @@ -58,12 +57,6 @@ func serve() { mainGhClientCache, _ := lru.New[string, githubapi.GhClientPair](128) prApproverGhClientCache, _ := lru.New[string, githubapi.GhClientPair](128) - // init argoclients - err := argocd.InitArgoClients() - if err != nil { - log.Fatalf("error initializing argo clients: %v", err) - } - mux := http.NewServeMux() mux.HandleFunc("/webhook", handleWebhook(githubWebhookSecret, mainGhClientCache, prApproverGhClientCache)) mux.Handle("/metrics", promhttp.Handler()) diff --git a/internal/pkg/argocd/argocd.go b/internal/pkg/argocd/argocd.go index 5593f02f..4b1fea9e 100644 --- a/internal/pkg/argocd/argocd.go +++ b/internal/pkg/argocd/argocd.go @@ -33,15 +33,13 @@ import ( // ctxLines is the number of context lines used in application diffs. const ctxLines = 10 -var argoClients argoCdClients // replaced during tests - -func InitArgoClients() error { - var err error - argoClients, err = createArgoCdClients() +// replace in testing +var InitArgoClients = func() (argoCdClients, error) { + argoClients, err := createArgoCdClients() if err != nil { - return fmt.Errorf("error creating ArgoCD clients: %w", err) + return argoCdClients{}, fmt.Errorf("error creating ArgoCD clients: %w", err) } - return nil + return argoClients, nil } type argoCdClients struct { @@ -556,6 +554,13 @@ 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) { + + // init argoclients + argoClients, err := InitArgoClients() + if err != nil { + log.Fatalf("error initializing argo clients: %v", err) + } + hasComponentDiff = false hasComponentDiffErrors = false diff --git a/internal/pkg/argocd/argocd_test.go b/internal/pkg/argocd/argocd_test.go index 798d3a79..9d903a9e 100644 --- a/internal/pkg/argocd/argocd_test.go +++ b/internal/pkg/argocd/argocd_test.go @@ -326,18 +326,21 @@ func TestFetchArgoDiffConcurrently(t *testing.T) { defer mockCtrl.Finish() // Save and restore the original argoClients - saved := argoClients - defer func() { argoClients = saved }() + saved := InitArgoClients + defer func() { InitArgoClients = saved }() // mock the argoClients mockAppServiceClient := mocks.NewMockApplicationServiceClient(mockCtrl) mockSettingsServiceClient := mocks.NewMockSettingsServiceClient(mockCtrl) mockProjectServiceClient := mocks.NewMockProjectServiceClient(mockCtrl) - - argoClients = argoCdClients{ - app: mockAppServiceClient, - setting: mockSettingsServiceClient, - project: mockProjectServiceClient, + // fake InitArgoClients + InitArgoClients = func() (argoCdClients, error) { + argoClients := argoCdClients{ + app: mockAppServiceClient, + setting: mockSettingsServiceClient, + project: mockProjectServiceClient, + } + return argoClients, nil } // slowReply simulates a slow reply from the server diff --git a/internal/pkg/githubapi/github.go b/internal/pkg/githubapi/github.go index f8372997..5d59bf85 100644 --- a/internal/pkg/githubapi/github.go +++ b/internal/pkg/githubapi/github.go @@ -224,6 +224,7 @@ 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) if err != nil { return fmt.Errorf("getting diff information: %w", err) From b06757c92694ffbae44b038bf3ec435b2e682416 Mon Sep 17 00:00:00 2001 From: Arthur Shvarts Date: Wed, 4 Dec 2024 15:16:09 -0500 Subject: [PATCH 05/13] Update internal/pkg/githubapi/github.go Co-authored-by: Hannes Gustafsson --- internal/pkg/githubapi/github.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/internal/pkg/githubapi/github.go b/internal/pkg/githubapi/github.go index 5d59bf85..2bdd8c73 100644 --- a/internal/pkg/githubapi/github.go +++ b/internal/pkg/githubapi/github.go @@ -224,8 +224,12 @@ func handleChangedPREvent(ctx context.Context, mainGithubClientPair GhClientPair ghPrClientDetails.PrLogger.Debugf("ArgoCD diff disabled for %s\n", componentPath) } } + argoClients, err := 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) + 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) } From 13f3c188d92a7a789f493412ee365a247b23c75c Mon Sep 17 00:00:00 2001 From: Arthur Shvarts Date: Wed, 4 Dec 2024 15:16:23 -0500 Subject: [PATCH 06/13] Update internal/pkg/argocd/argocd_test.go Co-authored-by: Hannes Gustafsson --- internal/pkg/argocd/argocd_test.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/internal/pkg/argocd/argocd_test.go b/internal/pkg/argocd/argocd_test.go index 9d903a9e..b30bb301 100644 --- a/internal/pkg/argocd/argocd_test.go +++ b/internal/pkg/argocd/argocd_test.go @@ -325,9 +325,10 @@ func TestFetchArgoDiffConcurrently(t *testing.T) { mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() - // Save and restore the original argoClients - saved := InitArgoClients - defer func() { InitArgoClients = saved }() + argoClients, err := createArgoCdClients() + if err != nil { + t.Errorf("error creating ArgoCD clients: %v", err) + } // mock the argoClients mockAppServiceClient := mocks.NewMockApplicationServiceClient(mockCtrl) From d76712ece0d368133f44b22a0efde5631f6da033 Mon Sep 17 00:00:00 2001 From: Arthur Shvarts Date: Wed, 4 Dec 2024 15:16:31 -0500 Subject: [PATCH 07/13] Update internal/pkg/argocd/argocd.go Co-authored-by: Hannes Gustafsson --- internal/pkg/argocd/argocd.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/internal/pkg/argocd/argocd.go b/internal/pkg/argocd/argocd.go index 4b1fea9e..b62b7e47 100644 --- a/internal/pkg/argocd/argocd.go +++ b/internal/pkg/argocd/argocd.go @@ -554,9 +554,6 @@ 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) { - - // init argoclients - argoClients, err := InitArgoClients() if err != nil { log.Fatalf("error initializing argo clients: %v", err) } From fc2cf4da0b7eb9ebb17574af19c65a1f6b046ce1 Mon Sep 17 00:00:00 2001 From: Arthur Shvarts Date: Wed, 4 Dec 2024 15:17:26 -0500 Subject: [PATCH 08/13] Update internal/pkg/argocd/argocd_test.go Co-authored-by: Hannes Gustafsson --- internal/pkg/argocd/argocd_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/pkg/argocd/argocd_test.go b/internal/pkg/argocd/argocd_test.go index b30bb301..ca503b1b 100644 --- a/internal/pkg/argocd/argocd_test.go +++ b/internal/pkg/argocd/argocd_test.go @@ -431,6 +431,7 @@ func TestFetchArgoDiffConcurrently(t *testing.T) { "test-repo", true, false, + argoClients, ) // stop timer From 9ce65b543455f43199588a6bf011637fababee12 Mon Sep 17 00:00:00 2001 From: Arthur Shvarts Date: Wed, 4 Dec 2024 15:17:34 -0500 Subject: [PATCH 09/13] Update internal/pkg/argocd/argocd.go Co-authored-by: Hannes Gustafsson --- internal/pkg/argocd/argocd.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/pkg/argocd/argocd.go b/internal/pkg/argocd/argocd.go index b62b7e47..820f3b66 100644 --- a/internal/pkg/argocd/argocd.go +++ b/internal/pkg/argocd/argocd.go @@ -553,7 +553,7 @@ 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) { if err != nil { log.Fatalf("error initializing argo clients: %v", err) } From a29fdf5ebe546a706f2e18df62bd6b7dc6443a2f Mon Sep 17 00:00:00 2001 From: Arthur Shvarts Date: Wed, 4 Dec 2024 15:17:46 -0500 Subject: [PATCH 10/13] Update internal/pkg/argocd/argocd.go Co-authored-by: Hannes Gustafsson --- internal/pkg/argocd/argocd.go | 9 --------- 1 file changed, 9 deletions(-) diff --git a/internal/pkg/argocd/argocd.go b/internal/pkg/argocd/argocd.go index 820f3b66..bb48ca02 100644 --- a/internal/pkg/argocd/argocd.go +++ b/internal/pkg/argocd/argocd.go @@ -33,15 +33,6 @@ import ( // ctxLines is the number of context lines used in application diffs. const ctxLines = 10 -// replace in testing -var InitArgoClients = func() (argoCdClients, error) { - argoClients, err := createArgoCdClients() - if err != nil { - return argoCdClients{}, fmt.Errorf("error creating ArgoCD clients: %w", err) - } - return argoClients, nil -} - type argoCdClients struct { app application.ApplicationServiceClient project projectpkg.ProjectServiceClient From bb890c1a5bc039c0a129fa86acf682e82ce3bc98 Mon Sep 17 00:00:00 2001 From: Arthur Shvarts Date: Wed, 4 Dec 2024 15:17:58 -0500 Subject: [PATCH 11/13] Update internal/pkg/argocd/argocd.go Co-authored-by: Hannes Gustafsson --- internal/pkg/argocd/argocd.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/pkg/argocd/argocd.go b/internal/pkg/argocd/argocd.go index bb48ca02..5afaf031 100644 --- a/internal/pkg/argocd/argocd.go +++ b/internal/pkg/argocd/argocd.go @@ -558,7 +558,7 @@ func GenerateDiffOfChangedComponents(ctx context.Context, componentsToDiff map[s return false, true, nil, err } - diffResult := make(chan DiffResult, len(componentsToDiff)) + diffResult := make(chan DiffResult) for componentPath, shouldIDiff := range componentsToDiff { go func(componentPath string, shouldDiff bool) { diffResult <- generateDiffOfAComponent(ctx, shouldIDiff, componentPath, prBranch, repo, argoClients, argoSettings, useSHALabelForArgoDicovery, createTempAppObjectFromNewApps) From b1627466cc96ebe36d46231479fe1437ae9df20c Mon Sep 17 00:00:00 2001 From: Arthur Shvarts Date: Wed, 4 Dec 2024 15:31:25 -0500 Subject: [PATCH 12/13] Use dependency injection with argocd client --- internal/pkg/argocd/argocd.go | 4 ++-- internal/pkg/argocd/argocd_test.go | 18 +++++------------- internal/pkg/githubapi/github.go | 2 +- 3 files changed, 8 insertions(+), 16 deletions(-) diff --git a/internal/pkg/argocd/argocd.go b/internal/pkg/argocd/argocd.go index 5afaf031..8c0b161c 100644 --- 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) } diff --git a/internal/pkg/argocd/argocd_test.go b/internal/pkg/argocd/argocd_test.go index ca503b1b..a34766b6 100644 --- a/internal/pkg/argocd/argocd_test.go +++ b/internal/pkg/argocd/argocd_test.go @@ -325,25 +325,17 @@ func TestFetchArgoDiffConcurrently(t *testing.T) { mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() - argoClients, err := createArgoCdClients() - if err != nil { - t.Errorf("error creating ArgoCD clients: %v", err) - } - // mock the argoClients mockAppServiceClient := mocks.NewMockApplicationServiceClient(mockCtrl) mockSettingsServiceClient := mocks.NewMockSettingsServiceClient(mockCtrl) mockProjectServiceClient := mocks.NewMockProjectServiceClient(mockCtrl) // fake InitArgoClients - InitArgoClients = func() (argoCdClients, error) { - argoClients := argoCdClients{ - app: mockAppServiceClient, - setting: mockSettingsServiceClient, - project: mockProjectServiceClient, - } - return argoClients, nil - } + 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) diff --git a/internal/pkg/githubapi/github.go b/internal/pkg/githubapi/github.go index 2bdd8c73..5f128c98 100644 --- a/internal/pkg/githubapi/github.go +++ b/internal/pkg/githubapi/github.go @@ -224,7 +224,7 @@ func handleChangedPREvent(ctx context.Context, mainGithubClientPair GhClientPair ghPrClientDetails.PrLogger.Debugf("ArgoCD diff disabled for %s\n", componentPath) } } - argoClients, err := createArgoCdClients() + argoClients, err := argocd.CreateArgoCdClients() if err != nil { return fmt.Errorf("error creating ArgoCD clients: %w", err) } From 847c5fce3358ba011442b2e453d03b220a399f0c Mon Sep 17 00:00:00 2001 From: Arthur Shvarts Date: Wed, 4 Dec 2024 16:50:04 -0500 Subject: [PATCH 13/13] Remove dangling err check --- internal/pkg/argocd/argocd.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/internal/pkg/argocd/argocd.go b/internal/pkg/argocd/argocd.go index 8c0b161c..074ce615 100644 --- a/internal/pkg/argocd/argocd.go +++ b/internal/pkg/argocd/argocd.go @@ -545,16 +545,12 @@ 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, argoClients argoCdClients) (hasComponentDiff bool, hasComponentDiffErrors bool, diffResults []DiffResult, err error) { - if err != nil { - log.Fatalf("error initializing argo clients: %v", err) - } - hasComponentDiff = false hasComponentDiffErrors = false 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 }