From cce0e2ea69283b0a88b6054ea12f8ced31929a99 Mon Sep 17 00:00:00 2001 From: pashakostohrys Date: Wed, 16 Oct 2024 18:50:10 +0300 Subject: [PATCH 1/8] feat: acr controller crashes when here no history --- acr_controller/service/acr_service.go | 7 ++-- acr_controller/service/acr_service_test.go | 40 ++++++++++++++++++++++ changelog/CHANGELOG.md | 4 --- util/git/client.go | 5 +++ 4 files changed, 50 insertions(+), 6 deletions(-) delete mode 100644 changelog/CHANGELOG.md diff --git a/acr_controller/service/acr_service.go b/acr_controller/service/acr_service.go index 0eeac9255fb85..211d6492f189f 100644 --- a/acr_controller/service/acr_service.go +++ b/acr_controller/service/acr_service.go @@ -102,7 +102,7 @@ func (c *acrService) ChangeRevision(ctx context.Context, a *application.Applicat func (c *acrService) calculateRevision(ctx context.Context, a *application.Application) (*string, error) { currentRevision, previousRevision := c.getRevisions(ctx, a) - log.Infof("Calculate revision for application %s, current revision %s, previous revision %s", a.Name, currentRevision, previousRevision) + log.Infof("Calculate revision for application '%s', current revision '%s', previous revision '%s'", a.Name, currentRevision, previousRevision) changeRevisionResult, err := c.applicationServiceClient.GetChangeRevision(ctx, &appclient.ChangeRevisionRequest{ AppName: pointer.String(a.GetName()), Namespace: pointer.String(a.GetNamespace()), @@ -173,7 +173,10 @@ func (c *acrService) patchOperationSyncResultWithChangeRevision(ctx context.Cont func (c *acrService) getRevisions(ctx context.Context, a *application.Application) (string, string) { if a.Status.History == nil || len(a.Status.History) == 0 { - // it is first sync operation, and we dont need detect change revision in such case + // it is first sync operation, and we have only current revision + if a.Operation != nil && a.Operation.Sync != nil { + return a.Operation.Sync.Revision, "" + } return "", "" } diff --git a/acr_controller/service/acr_service_test.go b/acr_controller/service/acr_service_test.go index 660e6fbe51672..cac465f51871a 100644 --- a/acr_controller/service/acr_service_test.go +++ b/acr_controller/service/acr_service_test.go @@ -36,6 +36,39 @@ spec: server: https://cluster-api.example.com ` +const fakeAppWithOperation = ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + annotations: + argocd.argoproj.io/manifest-generate-paths: . + finalizers: + - resources-finalizer.argocd.argoproj.io + labels: + app.kubernetes.io/instance: guestbook + name: guestbook + namespace: codefresh +operation: + initiatedBy: + automated: true + retry: + limit: 5 + sync: + prune: true + revision: c732f4d2ef24c7eeb900e9211ff98f90bb646505 + syncOptions: + - CreateNamespace=true +spec: + destination: + namespace: guestbook + server: https://kubernetes.default.svc + project: default + source: + path: apps/guestbook + repoURL: https://github.com/pasha-codefresh/precisely-gitsource.git + targetRevision: HEAD +` + const syncedAppWithHistory = ` apiVersion: argoproj.io/v1alpha1 kind: Application @@ -135,6 +168,13 @@ func Test_getRevisions(r *testing.T) { assert.Equal(t, "", previous) }) + r.Run("history list is empty, but operation happens right now", func(t *testing.T) { + acrService := newTestACRService(&mocks.ApplicationClient{}) + current, previous := acrService.getRevisions(context.TODO(), createTestApp(fakeAppWithOperation)) + assert.Equal(t, "c732f4d2ef24c7eeb900e9211ff98f90bb646505", current) + assert.Equal(t, "", previous) + }) + r.Run("application is synced", func(t *testing.T) { acrService := newTestACRService(&mocks.ApplicationClient{}) app := createTestApp(syncedAppWithHistory) diff --git a/changelog/CHANGELOG.md b/changelog/CHANGELOG.md deleted file mode 100644 index ec837707a338b..0000000000000 --- a/changelog/CHANGELOG.md +++ /dev/null @@ -1,4 +0,0 @@ -### Features -- feat: argocd-repo-server: support for arrays in promotion versionSource -- feat: event-reporter: getting version based on synced revision -- feat: argocd-repo-server: suppress 'version not found' error message when version configuration is not provided \ No newline at end of file diff --git a/util/git/client.go b/util/git/client.go index 44a30993f3c75..4122886d894fd 100644 --- a/util/git/client.go +++ b/util/git/client.go @@ -907,6 +907,11 @@ func (m *nativeGitClient) Diff(targetRevision string) ([]string, error) { } func (m *nativeGitClient) ListRevisions(revision string, targetRevision string) ([]string, error) { + // it happens when app just created and there is no revision yet + if revision == "" { + return []string{targetRevision}, nil + } + if !IsCommitSHA(revision) || !IsCommitSHA(targetRevision) { return nil, fmt.Errorf("invalid revision provided, must be SHA") } From 032b524d03f5bb04cbe0c73e290a095621dcd065 Mon Sep 17 00:00:00 2001 From: pashakostohrys Date: Wed, 16 Oct 2024 20:25:34 +0300 Subject: [PATCH 2/8] feat: acr controller crashes when here no history --- acr_controller/service/acr_service.go | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/acr_controller/service/acr_service.go b/acr_controller/service/acr_service.go index 211d6492f189f..1bf941b854cd3 100644 --- a/acr_controller/service/acr_service.go +++ b/acr_controller/service/acr_service.go @@ -171,13 +171,17 @@ func (c *acrService) patchOperationSyncResultWithChangeRevision(ctx context.Cont return err } +func getCurrentRevisionFromOperation(a *application.Application) string { + if a.Operation != nil && a.Operation.Sync != nil { + return a.Operation.Sync.Revision + } + return "" +} + func (c *acrService) getRevisions(ctx context.Context, a *application.Application) (string, string) { if a.Status.History == nil || len(a.Status.History) == 0 { // it is first sync operation, and we have only current revision - if a.Operation != nil && a.Operation.Sync != nil { - return a.Operation.Sync.Revision, "" - } - return "", "" + return getCurrentRevisionFromOperation(a), "" } // in case if sync is already done, we need to use revision from sync result and previous revision from history @@ -187,7 +191,7 @@ func (c *acrService) getRevisions(ctx context.Context, a *application.Applicatio } // in case if sync is in progress, we need to use revision from operation and revision from latest history record - currentRevision := a.Operation.Sync.Revision + currentRevision := getCurrentRevisionFromOperation(a) previousRevision := a.Status.History[len(a.Status.History)-1].Revision return currentRevision, previousRevision } From fbf2674fb882d795e1029ecd1c62e09a66bcdb17 Mon Sep 17 00:00:00 2001 From: pashakostohrys Date: Wed, 16 Oct 2024 20:47:52 +0300 Subject: [PATCH 3/8] feat: acr controller crashes when here no history --- .golangci.yaml | 1 - 1 file changed, 1 deletion(-) diff --git a/.golangci.yaml b/.golangci.yaml index 7d6b684a83683..1ba95417ed49e 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -9,7 +9,6 @@ linters: - errcheck - errorlint - gocritic - - gofumpt - goimports - gosimple - govet From d4698b5d021852b93bb7e99de4ef9e3e17121df1 Mon Sep 17 00:00:00 2001 From: pashakostohrys Date: Wed, 16 Oct 2024 21:06:28 +0300 Subject: [PATCH 4/8] feat: acr controller crashes when here no history --- .golangci.yaml | 1 - 1 file changed, 1 deletion(-) diff --git a/.golangci.yaml b/.golangci.yaml index 1ba95417ed49e..d8b642f8bfac2 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -9,7 +9,6 @@ linters: - errcheck - errorlint - gocritic - - goimports - gosimple - govet - ineffassign From 9ed7adafbb5edb497136be494d461d7b97f4c3a4 Mon Sep 17 00:00:00 2001 From: pashakostohrys Date: Wed, 16 Oct 2024 21:18:16 +0300 Subject: [PATCH 5/8] feat: acr controller crashes when here no history --- .golangci.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.golangci.yaml b/.golangci.yaml index d8b642f8bfac2..7d6b684a83683 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -9,6 +9,8 @@ linters: - errcheck - errorlint - gocritic + - gofumpt + - goimports - gosimple - govet - ineffassign From 97d82afb34da136d3112dfc0bac0eeb8b09cf89d Mon Sep 17 00:00:00 2001 From: pashakostohrys Date: Wed, 16 Oct 2024 22:03:51 +0300 Subject: [PATCH 6/8] handle single history item --- acr_controller/service/acr_service.go | 4 ++ acr_controller/service/acr_service_test.go | 69 ++++++++++++++++++++++ 2 files changed, 73 insertions(+) diff --git a/acr_controller/service/acr_service.go b/acr_controller/service/acr_service.go index 1bf941b854cd3..e35dfc5766a65 100644 --- a/acr_controller/service/acr_service.go +++ b/acr_controller/service/acr_service.go @@ -187,6 +187,10 @@ func (c *acrService) getRevisions(ctx context.Context, a *application.Applicatio // in case if sync is already done, we need to use revision from sync result and previous revision from history if a.Status.Sync.Status == "Synced" && a.Status.OperationState != nil && a.Status.OperationState.SyncResult != nil { currentRevision := a.Status.OperationState.SyncResult.Revision + // in case if we have only one history record, we need to return empty previous revision, because it is first sync result + if len(a.Status.History) == 1 { + return currentRevision, "" + } return currentRevision, a.Status.History[len(a.Status.History)-2].Revision } diff --git a/acr_controller/service/acr_service_test.go b/acr_controller/service/acr_service_test.go index cac465f51871a..a2554d793693d 100644 --- a/acr_controller/service/acr_service_test.go +++ b/acr_controller/service/acr_service_test.go @@ -69,6 +69,68 @@ spec: targetRevision: HEAD ` +const syncedAppWithSingleHistory = ` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + annotations: + argocd.argoproj.io/manifest-generate-paths: . + finalizers: + - resources-finalizer.argocd.argoproj.io + labels: + app.kubernetes.io/instance: guestbook + name: guestbook + namespace: codefresh +operation: + initiatedBy: + automated: true + retry: + limit: 5 + sync: + prune: true + revision: c732f4d2ef24c7eeb900e9211ff98f90bb646505 + syncOptions: + - CreateNamespace=true +spec: + destination: + namespace: guestbook + server: https://kubernetes.default.svc + project: default + source: + path: apps/guestbook + repoURL: https://github.com/pasha-codefresh/precisely-gitsource.git + targetRevision: HEAD +status: + history: + - deployStartedAt: "2024-06-20T19:35:36Z" + deployedAt: "2024-06-20T19:35:44Z" + id: 3 + initiatedBy: {} + revision: 792822850fd2f6db63597533e16dfa27e6757dc5 + source: + path: apps/guestbook + repoURL: https://github.com/pasha-codefresh/precisely-gitsource.git + targetRevision: HEAD + operationState: + operation: + sync: + prune: true + revision: c732f4d2ef24c7eeb900e9211ff98f90bb646506 + syncOptions: + - CreateNamespace=true + phase: Running + startedAt: "2024-06-20T19:47:34Z" + syncResult: + revision: c732f4d2ef24c7eeb900e9211ff98f90bb646505 + source: + path: apps/guestbook + repoURL: https://github.com/pasha-codefresh/precisely-gitsource.git + targetRevision: HEAD + sync: + revision: 00d423763fbf56d2ea452de7b26a0ab20590f521 + status: Synced +` + const syncedAppWithHistory = ` apiVersion: argoproj.io/v1alpha1 kind: Application @@ -175,6 +237,13 @@ func Test_getRevisions(r *testing.T) { assert.Equal(t, "", previous) }) + r.Run("history list contains only one element, also sync result is here", func(t *testing.T) { + acrService := newTestACRService(&mocks.ApplicationClient{}) + current, previous := acrService.getRevisions(context.TODO(), createTestApp(syncedAppWithSingleHistory)) + assert.Equal(t, "c732f4d2ef24c7eeb900e9211ff98f90bb646505", current) + assert.Equal(t, "", previous) + }) + r.Run("application is synced", func(t *testing.T) { acrService := newTestACRService(&mocks.ApplicationClient{}) app := createTestApp(syncedAppWithHistory) From 2e35fd8440ebf33a7b4796549a202bfb2b776598 Mon Sep 17 00:00:00 2001 From: pashakostohrys Date: Wed, 16 Oct 2024 22:20:41 +0300 Subject: [PATCH 7/8] handle single history item --- acr_controller/service/acr_service_test.go | 2 +- util/git/client.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/acr_controller/service/acr_service_test.go b/acr_controller/service/acr_service_test.go index a2554d793693d..877006b979601 100644 --- a/acr_controller/service/acr_service_test.go +++ b/acr_controller/service/acr_service_test.go @@ -243,7 +243,7 @@ func Test_getRevisions(r *testing.T) { assert.Equal(t, "c732f4d2ef24c7eeb900e9211ff98f90bb646505", current) assert.Equal(t, "", previous) }) - + r.Run("application is synced", func(t *testing.T) { acrService := newTestACRService(&mocks.ApplicationClient{}) app := createTestApp(syncedAppWithHistory) diff --git a/util/git/client.go b/util/git/client.go index 4122886d894fd..317628579c5a2 100644 --- a/util/git/client.go +++ b/util/git/client.go @@ -911,7 +911,7 @@ func (m *nativeGitClient) ListRevisions(revision string, targetRevision string) if revision == "" { return []string{targetRevision}, nil } - + if !IsCommitSHA(revision) || !IsCommitSHA(targetRevision) { return nil, fmt.Errorf("invalid revision provided, must be SHA") } From cf48a0e4ba1dd88f04295127301d076dea666979 Mon Sep 17 00:00:00 2001 From: pashakostohrys Date: Wed, 16 Oct 2024 22:26:37 +0300 Subject: [PATCH 8/8] handle single history item --- acr_controller/service/acr_service_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acr_controller/service/acr_service_test.go b/acr_controller/service/acr_service_test.go index 877006b979601..a2554d793693d 100644 --- a/acr_controller/service/acr_service_test.go +++ b/acr_controller/service/acr_service_test.go @@ -243,7 +243,7 @@ func Test_getRevisions(r *testing.T) { assert.Equal(t, "c732f4d2ef24c7eeb900e9211ff98f90bb646505", current) assert.Equal(t, "", previous) }) - + r.Run("application is synced", func(t *testing.T) { acrService := newTestACRService(&mocks.ApplicationClient{}) app := createTestApp(syncedAppWithHistory)