From f18bd46a82d816ec7de3d32ca97802e644d47958 Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Tue, 21 May 2024 11:34:32 +0200 Subject: [PATCH 01/15] Use telefonistka in-pr metadata mechanism to pass changed component path to ArgoCD diff functionality --- go.mod | 14 +++++++------- go.sum | 14 ++++++++++++++ internal/pkg/githubapi/github.go | 21 +++++++++++++++++---- 3 files changed, 38 insertions(+), 11 deletions(-) diff --git a/go.mod b/go.mod index 2c5e8bba..45292bc0 100644 --- a/go.mod +++ b/go.mod @@ -20,7 +20,7 @@ require ( github.com/shurcooL/githubv4 v0.0.0-20240120211514-18a1ae0e79dc github.com/sirupsen/logrus v1.9.3 github.com/spf13/cobra v1.8.0 - golang.org/x/exp v0.0.0-20240409090435-93d18d7e34b8 + golang.org/x/exp v0.0.0-20240506185415-9bf2ced13842 golang.org/x/oauth2 v0.19.0 gopkg.in/yaml.v2 v2.4.0 k8s.io/apimachinery v0.26.11 @@ -148,15 +148,15 @@ require ( go.opentelemetry.io/otel/metric v1.25.0 // indirect go.opentelemetry.io/otel/trace v1.25.0 // indirect go.starlark.net v0.0.0-20240408152805-3f0a3703c02a // indirect - golang.org/x/crypto v0.22.0 // indirect + golang.org/x/crypto v0.23.0 // indirect golang.org/x/mod v0.17.0 // indirect - golang.org/x/net v0.24.0 // indirect + golang.org/x/net v0.25.0 // indirect golang.org/x/sync v0.7.0 // indirect - golang.org/x/sys v0.19.0 // indirect - golang.org/x/term v0.19.0 // indirect - golang.org/x/text v0.14.0 // indirect + golang.org/x/sys v0.20.0 // indirect + golang.org/x/term v0.20.0 // indirect + golang.org/x/text v0.15.0 // indirect golang.org/x/time v0.5.0 // indirect - golang.org/x/tools v0.20.0 // indirect + golang.org/x/tools v0.21.0 // indirect golang.org/x/xerrors v0.0.0-20220907171357-04be3eba64a2 // indirect google.golang.org/genproto v0.0.0-20240401170217-c3f982113cda // indirect google.golang.org/genproto/googleapis/api v0.0.0-20240401170217-c3f982113cda // indirect diff --git a/go.sum b/go.sum index 85c0e88e..8a8aac21 100644 --- a/go.sum +++ b/go.sum @@ -1315,6 +1315,8 @@ golang.org/x/crypto v0.10.0/go.mod h1:o4eNf7Ede1fv+hwOwZsTHl9EsPFO6q6ZvYR8vYfY45 golang.org/x/crypto v0.19.0/go.mod h1:Iy9bg/ha4yyC70EfRS8jz+B6ybOBKMaSxLj6P6oBDfU= golang.org/x/crypto v0.22.0 h1:g1v0xeRhjcugydODzvb3mEM9SQ0HGp9s/nh3COQ/C30= golang.org/x/crypto v0.22.0/go.mod h1:vr6Su+7cTlO45qkww3VDJlzDn0ctJvRgYbC2NvXHt+M= +golang.org/x/crypto v0.23.0 h1:dIJU/v2J8Mdglj/8rJ6UUOM3Zc9zLZxVZwwxMooUSAI= +golang.org/x/crypto v0.23.0/go.mod h1:CKFgDieR+mRhux2Lsu27y0fO304Db0wZe70UKqHu0v8= golang.org/x/exp v0.0.0-20180321215751-8460e604b9de/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA= golang.org/x/exp v0.0.0-20180807140117-3d87b88a115f/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA= golang.org/x/exp v0.0.0-20190121172915-509febef88a4/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA= @@ -1332,6 +1334,8 @@ golang.org/x/exp v0.0.0-20200224162631-6cc2880d07d6/go.mod h1:3jZMyOhIsHpP37uCMk golang.org/x/exp v0.0.0-20220827204233-334a2380cb91/go.mod h1:cyybsKvd6eL0RnXn6p/Grxp8F5bW7iYuBgsNCOHpMYE= golang.org/x/exp v0.0.0-20240409090435-93d18d7e34b8 h1:ESSUROHIBHg7USnszlcdmjBEwdMj9VUvU+OPk4yl2mc= golang.org/x/exp v0.0.0-20240409090435-93d18d7e34b8/go.mod h1:/lliqkxwWAhPjf5oSOIJup2XcqJaw8RGS6k3TGEc7GI= +golang.org/x/exp v0.0.0-20240506185415-9bf2ced13842 h1:vr/HnozRka3pE4EsMEg1lgkXJkTFJCVUX+S/ZT6wYzM= +golang.org/x/exp v0.0.0-20240506185415-9bf2ced13842/go.mod h1:XtvwrStGgqGPLc4cjQfWqZHG1YFdYs6swckp8vpsjnc= golang.org/x/image v0.0.0-20180708004352-c73c2afc3b81/go.mod h1:ux5Hcp/YLpHSI86hEcLt0YII63i6oz57MZXIpbrjZUs= golang.org/x/image v0.0.0-20190227222117-0694c2d4d067/go.mod h1:kZ7UVZpmo3dzQBMxlp+ypCbDeSB+sBbTgSJuh5dn5js= golang.org/x/image v0.0.0-20190802002840-cff245a6509b/go.mod h1:FeLwcggjj3mMvU+oOTbSwawSJRM1uh48EjtB4UJZlP0= @@ -1445,6 +1449,8 @@ golang.org/x/net v0.10.0/go.mod h1:0qNGK6F8kojg2nk9dLZ2mShWaEBan6FAoqfSigmmuDg= golang.org/x/net v0.11.0/go.mod h1:2L/ixqYpgIVXmeoSA/4Lu7BzTG4KIyPIryS4IsOd1oQ= golang.org/x/net v0.24.0 h1:1PcaxkF854Fu3+lvBIx5SYn9wRlBzzcnHZSiaFFAb0w= golang.org/x/net v0.24.0/go.mod h1:2Q7sJY5mzlzWjKtYUEXSlBWCdyaioyXzRB2RtU8KVE8= +golang.org/x/net v0.25.0 h1:d/OCCoBEUq33pjydKrGQhw7IlUPI2Oylr+8qLx49kac= +golang.org/x/net v0.25.0/go.mod h1:JkAGAh7GEvH74S6FOH42FLoXpXbE/aqXSrIQjXgsiwM= golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U= golang.org/x/oauth2 v0.0.0-20190226205417-e64efc72b421/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw= golang.org/x/oauth2 v0.0.0-20190604053449-0f29369cfe45/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw= @@ -1592,6 +1598,8 @@ golang.org/x/sys v0.9.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.17.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= golang.org/x/sys v0.19.0 h1:q5f1RH2jigJ1MoAWp2KTp3gm5zAGFUTarQZ5U386+4o= golang.org/x/sys v0.19.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= +golang.org/x/sys v0.20.0 h1:Od9JTbYCk261bKm4M/mw7AklTlFYIa0bIp9BgSm1S8Y= +golang.org/x/sys v0.20.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= golang.org/x/term v0.0.0-20210927222741-03fcf44c2211/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8= golang.org/x/term v0.1.0/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8= @@ -1605,6 +1613,8 @@ golang.org/x/term v0.9.0/go.mod h1:M6DEAAIenWoTxdKrOltXcmDY3rSplQUkrvaDU5FcQyo= golang.org/x/term v0.17.0/go.mod h1:lLRBjIVuehSbZlaOtGMbcMncT+aqLLLmKrsjNrUguwk= golang.org/x/term v0.19.0 h1:+ThwsDv+tYfnJFhF4L8jITxu1tdTWRTZpdsWgEgjL6Q= golang.org/x/term v0.19.0/go.mod h1:2CuTdWZ7KHSQwUzKva0cbMg6q2DMI3Mmxp+gKJbskEk= +golang.org/x/term v0.20.0 h1:VnkxpohqXaOBYJtBmEppKUG6mXpi+4O6purfc2+sMhw= +golang.org/x/term v0.20.0/go.mod h1:8UkIAJTvZgivsXaD6/pH6U9ecQzZ45awqEOzuCvwpFY= golang.org/x/text v0.0.0-20160726164857-2910a502d2bf/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.0.0-20170915032832-14c0d48ead0c/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= @@ -1625,6 +1635,8 @@ golang.org/x/text v0.9.0/go.mod h1:e1OnstbJyHTd6l/uOt8jFFHp6TRDWZR/bV3emEE/zU8= golang.org/x/text v0.10.0/go.mod h1:TvPlkZtksWOMsz7fbANvkp4WM8x/WCo/om8BMLbz+aE= golang.org/x/text v0.14.0 h1:ScX5w1eTa3QqT8oi6+ziP7dTV1S2+ALU0bI+0zXKWiQ= golang.org/x/text v0.14.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU= +golang.org/x/text v0.15.0 h1:h1V/4gjBv8v9cjcR6+AR5+/cIYK5N/WAgiv4xlsEtAk= +golang.org/x/text v0.15.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU= golang.org/x/time v0.0.0-20181108054448-85acf8d2951c/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ= golang.org/x/time v0.0.0-20190308202827-9d24e82272b4/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ= golang.org/x/time v0.0.0-20191024005414-555d28b269f0/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ= @@ -1704,6 +1716,8 @@ golang.org/x/tools v0.4.0/go.mod h1:UE5sM2OK9E/d67R0ANs2xJizIymRP5gJU295PvKXxjQ= golang.org/x/tools v0.6.0/go.mod h1:Xwgl3UAJ/d3gWutnCtw505GrjyAbvKui8lOU390QaIU= golang.org/x/tools v0.20.0 h1:hz/CVckiOxybQvFw6h7b/q80NTr9IUQb4s1IIzW7KNY= golang.org/x/tools v0.20.0/go.mod h1:WvitBU7JJf6A4jOdg4S1tviW9bhUxkgeCui/0JHctQg= +golang.org/x/tools v0.21.0 h1:qc0xYgIbsSDt9EyWz05J5wfa7LOVW0YTLOXrqdLAWIw= +golang.org/x/tools v0.21.0/go.mod h1:aiJjzUbINMkxbQROHiO6hDPo2LHcIPhhQsa9DLh0yGk= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= diff --git a/internal/pkg/githubapi/github.go b/internal/pkg/githubapi/github.go index 8d91a628..4da292cc 100644 --- a/internal/pkg/githubapi/github.go +++ b/internal/pkg/githubapi/github.go @@ -19,6 +19,7 @@ import ( "github.com/wayfair-incubator/telefonistka/internal/pkg/argocd" cfg "github.com/wayfair-incubator/telefonistka/internal/pkg/configuration" prom "github.com/wayfair-incubator/telefonistka/internal/pkg/prometheus" + "golang.org/x/exp/maps" ) type promotionInstanceMetaData struct { @@ -46,6 +47,7 @@ type GhPrClientDetails struct { type prMetadata struct { OriginalPrAuthor string `json:"originalPrAuthor"` OriginalPrNumber int `json:"originalPrNumber"` + PromotedPaths []string `json:"promotedPaths"` PreviousPromotionMetadata map[int]promotionInstanceMetaData `json:"previousPromotionPaths"` } @@ -104,11 +106,20 @@ func HandlePREvent(eventPayload *github.PullRequestEvent, ghPrClientDetails GhPr ghPrClientDetails.PrLogger.Errorf("Failed to minimize stale comments: err=%s\n", err) } if config.CommentArgocdDiffonPR { - componentPathList, err := generateListOfChangedComponentPaths(ghPrClientDetails, config) - if err != nil { - prHandleError = err - ghPrClientDetails.PrLogger.Errorf("Failed to get list of changed components: err=%s\n", err) + var componentPathList []string + + // Promotion PR have the list of paths to promote in the PR metadata + // For non promotion PR, we will generate the list of changed components based the PR changed files and the telefonistka configuration(sourcePath) + if DoesPrHasLabel(*eventPayload, "promotion") { + componentPathList = ghPrClientDetails.PrMetadata.PromotedPaths + } else { + componentPathList, err = generateListOfChangedComponentPaths(ghPrClientDetails, config) + if err != nil { + prHandleError = err + ghPrClientDetails.PrLogger.Errorf("Failed to get list of changed components: err=%s\n", err) + } } + hasComponentDiff, hasComponentDiffErrors, diffOfChangedComponents, err := argocd.GenerateDiffOfChangedComponents(ctx, componentPathList, ghPrClientDetails.Ref, ghPrClientDetails.RepoURL) if err != nil { prHandleError = err @@ -812,6 +823,8 @@ func generatePromotionPrBody(ghPrClientDetails GhPrClientDetails, components str // newPrMetadata.PreviousPromotionMetadata[ghPrClientDetails.PrNumber].TargetPaths = targetPaths // newPrMetadata.PreviousPromotionMetadata[ghPrClientDetails.PrNumber].SourcePath = sourcePath + newPrMetadata.PromotedPaths = maps.Keys(promotion.ComputedSyncPaths) + newPrBody = fmt.Sprintf("Promotion path(%s):\n\n", components) keys := make([]int, 0) From 2a5adc2de465b770bfaf04bbba2431a01c1fc362 Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Tue, 21 May 2024 11:44:12 +0200 Subject: [PATCH 02/15] Add some validation to avoid auto-merging PRs in cases telefonistka is unsure of the component that where changed --- internal/pkg/githubapi/github.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/pkg/githubapi/github.go b/internal/pkg/githubapi/github.go index 4da292cc..fc2c24cc 100644 --- a/internal/pkg/githubapi/github.go +++ b/internal/pkg/githubapi/github.go @@ -135,7 +135,9 @@ func HandlePREvent(eventPayload *github.PullRequestEvent, ghPrClientDetails GhPr } else { ghPrClientDetails.PrLogger.Debugf("PR %v labeled\n%+v", *eventPayload.PullRequest.Number, prLables) } - if DoesPrHasLabel(*eventPayload, "promotion") && config.AutoMergeNoDiffPRs { + // If the PR is a promotion PR and the diff is empty, we can auto-merge it + // "len(componentPathList) > 0" validates we are not auto-merging a PR that we failed to understand which apps it affects + if DoesPrHasLabel(*eventPayload, "promotion") && config.AutoMergeNoDiffPRs && len(componentPathList) > 0 { ghPrClientDetails.PrLogger.Infof("Auto-merging (no diff) PR %d", *eventPayload.PullRequest.Number) err := MergePr(ghPrClientDetails, eventPayload.PullRequest.Number) if err != nil { From 2b762300aa75687bbdb259f4c79491022f4a63bc Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Tue, 28 May 2024 13:56:51 +0200 Subject: [PATCH 03/15] Support two way to fetch matching ArgoCD apps from a Telefonistka component the telefonistka dedicated sha1 label and the ArgoCD native `manifest-generate-paths` annotation --- go.mod | 5 +- go.sum | 4 + internal/pkg/argocd/argocd.go | 88 ++++++++++-- internal/pkg/argocd/argocd_test.go | 196 +++++++++++++++++++++++++++ internal/pkg/configuration/config.go | 15 +- internal/pkg/githubapi/github.go | 2 +- internal/pkg/mocks/.gitignore | 1 + internal/pkg/mocks/mocks.go | 7 + 8 files changed, 295 insertions(+), 23 deletions(-) create mode 100644 internal/pkg/argocd/argocd_test.go create mode 100644 internal/pkg/mocks/.gitignore create mode 100644 internal/pkg/mocks/mocks.go diff --git a/go.mod b/go.mod index 45292bc0..d1872aa9 100644 --- a/go.mod +++ b/go.mod @@ -6,8 +6,8 @@ toolchain go1.22.1 require ( github.com/alexliesenfeld/health v0.8.0 - github.com/argoproj/argo-cd/v2 v2.11.0-rc1 - github.com/argoproj/gitops-engine v0.7.1-0.20240411122334-1ade3a199867 + github.com/argoproj/argo-cd/v2 v2.11.2 + github.com/argoproj/gitops-engine v0.7.1-0.20240416142647-fbecbb86e412 github.com/bradleyfalzon/ghinstallation/v2 v2.10.0 github.com/go-test/deep v1.1.0 github.com/google/go-github/v52 v52.0.0 @@ -77,6 +77,7 @@ require ( github.com/gogo/protobuf v1.3.2 // indirect github.com/golang-jwt/jwt/v4 v4.5.0 // indirect github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect + github.com/golang/mock v1.6.0 // indirect github.com/golang/protobuf v1.5.4 // indirect github.com/google/btree v1.1.2 // indirect github.com/google/gnostic v0.7.0 // indirect diff --git a/go.sum b/go.sum index 8a8aac21..79b5a6dd 100644 --- a/go.sum +++ b/go.sum @@ -651,8 +651,12 @@ github.com/apache/arrow/go/v11 v11.0.0/go.mod h1:Eg5OsL5H+e299f7u5ssuXsuHQVEGC4x github.com/apache/thrift v0.16.0/go.mod h1:PHK3hniurgQaNMZYaCLEqXKsYK8upmhPbmdP2FXSqgU= github.com/argoproj/argo-cd/v2 v2.11.0-rc1 h1:VCjpw0bwPcULNJ/FG8BnIyeesyOpT+1MUWuXPskbsWQ= github.com/argoproj/argo-cd/v2 v2.11.0-rc1/go.mod h1:/KySYrOzPQupCh7E1pzg6011p4AaRLszbUtkaWyATTU= +github.com/argoproj/argo-cd/v2 v2.11.2 h1:NygNrTFIMWUe1b48ddUuH+q2vRTHB+dFk3NcErx6GcM= +github.com/argoproj/argo-cd/v2 v2.11.2/go.mod h1:nUOZqAT9f3GewdG/dzYgrpwqOSMj5ukoWw4yAV2/WXA= github.com/argoproj/gitops-engine v0.7.1-0.20240411122334-1ade3a199867 h1:zMATM3uzAQHBLJ142MEGrZ4+3+xsXT36hzB1Dj2jptE= github.com/argoproj/gitops-engine v0.7.1-0.20240411122334-1ade3a199867/go.mod h1:gWE8uROi7hIkWGNAVM+8FWkMfo0vZ03SLx/aFw/DBzg= +github.com/argoproj/gitops-engine v0.7.1-0.20240416142647-fbecbb86e412 h1:je2wJpWtaoS55mA5MBPCeDnKMeF42pkxO9Oa5KbWrdg= +github.com/argoproj/gitops-engine v0.7.1-0.20240416142647-fbecbb86e412/go.mod h1:gWE8uROi7hIkWGNAVM+8FWkMfo0vZ03SLx/aFw/DBzg= github.com/argoproj/pkg v0.13.7-0.20230626144333-d56162821bd1 h1:qsHwwOJ21K2Ao0xPju1sNuqphyMnMYkyB3ZLoLtxWpo= github.com/argoproj/pkg v0.13.7-0.20230626144333-d56162821bd1/go.mod h1:CZHlkyAD1/+FbEn6cB2DQTj48IoLGvEYsWEvtzP3238= github.com/armon/go-socks5 v0.0.0-20160902184237-e75332964ef5 h1:0CwZNZbxp69SHPdPJAN/hZIm0C4OItdklCFmMRWYpio= diff --git a/internal/pkg/argocd/argocd.go b/internal/pkg/argocd/argocd.go index b45c9207..0abe8925 100644 --- a/internal/pkg/argocd/argocd.go +++ b/internal/pkg/argocd/argocd.go @@ -7,7 +7,9 @@ import ( "encoding/json" "fmt" "os" + "path/filepath" "strconv" + "strings" cmdutil "github.com/argoproj/argo-cd/v2/cmd/util" "github.com/argoproj/argo-cd/v2/pkg/apiclient" @@ -151,17 +153,14 @@ func createArgoCdClient() (apiclient.Client, error) { return clientset, nil } -func generateDiffOfAComponent(ctx context.Context, componentPath string, prBranch string, repo string, appIf application.ApplicationServiceClient, projIf projectpkg.ProjectServiceClient, argoSettings *settings.Settings) (componentDiffResult DiffResult) { - componentDiffResult.ComponentPath = componentPath - +// This function is used to find an ArgoCD application by the SHA1 label of the component path its supposed to avoid perfromance issues with the "manifest-generate-paths" annotation method with requires pulling all ArgoCD applications(!) on every PR event. +// The SHA1 label is assumed to be populated by the ApplicationSet controller(or apps of apps or similar). +func findArgocdAppBySHA1Label(ctx context.Context, componentPath string, repo string, appIf application.ApplicationServiceClient) (app *argoappv1.Application, err error) { // Calculate sha1 of component path to use in a label selector cPathBa := []byte(componentPath) hasher := sha1.New() //nolint:gosec // G505: Blocklisted import crypto/sha1: weak cryptographic primitive (gosec), this is not a cryptographic use case hasher.Write(cPathBa) componentPathSha1 := hex.EncodeToString(hasher.Sum(nil)) - - // Find ArgoCD application by the path SHA1 label selector and repo name - // That label is assumed to be pupulated by the ApplicationSet controller(or apps of apps or similar). labelSelector := fmt.Sprintf("telefonistka.io/component-path-sha1=%s", componentPathSha1) appLabelQuery := application.ApplicationQuery{ Selector: &labelSelector, @@ -169,24 +168,87 @@ func generateDiffOfAComponent(ctx context.Context, componentPath string, prBranc } foundApps, err := appIf.List(ctx, &appLabelQuery) if err != nil { - componentDiffResult.DiffError = err - return componentDiffResult + return nil, err } if len(foundApps.Items) == 0 { - componentDiffResult.DiffError = fmt.Errorf("No ArgoCD application found for component path %s(repo %s), used this label selector: %s", componentPath, repo, labelSelector) + return nil, fmt.Errorf("No ArgoCD application found for component path sha1 %s(repo %s), used this label selector: %s", componentPathSha1, repo, labelSelector) + } + + // we expect only one app with this label and repo selectors + return &foundApps.Items[0], nil +} + +// This is the default method to find an ArgoCD application by the manifest-generate-paths annotation. +// It assume the ArgoCD (optional) manifest-generate-paths annotation is set on all relevent apps. +// Notice that this method include a full list all ArgoCD applications in the repo, this could be a performance issue if there are many apps in the repo. +func findArgocdAppByManifestPathAnnotation(ctx context.Context, componentPath string, repo string, appIf application.ApplicationServiceClient) (app *argoappv1.Application, err error) { + //argocd.argoproj.io/manifest-generate-paths + appQuery := application.ApplicationQuery{ + Repo: &repo, + } + // TODO Instrument this, we might have a lot of apps in a repo, performance might be an issue + allRepoApps, err := appIf.List(ctx, &appQuery) + if err != nil { + return nil, err + } + for _, app := range allRepoApps.Items { + // Check if the app has the annotation + // https://argo-cd.readthedocs.io/en/stable/operator-manual/high_availability/#manifest-paths-annotation + // Consider the annotation content can a semi-colon separated list of paths, an absolute path or a relative path(start with a ".") and the manifest-paths-annotation could be a subpath of componentPath. + // We need to check if the annotation is a subpath of componentPath + + appManifestPathsAnnotation := app.Annotations["argocd.argoproj.io/manifest-generate-paths"] + + for _, manifetsPathElement := range strings.Split(appManifestPathsAnnotation, ";") { + // if `manifest-generate-paths` element starts with a "." it is a relative path(relative to repo root), we need to join it with the app source path + if strings.HasPrefix(manifetsPathElement, ".") { + manifetsPathElement = filepath.Join(app.Spec.Source.Path, manifetsPathElement) + } + + // Checking is componentPath is a subpath of the manifetsPathElement + // Using filepath.Rel solves all kinds of path issues, like double slashes, etc. + rel, err := filepath.Rel(manifetsPathElement, componentPath) + if !strings.HasPrefix(rel, "..") && err == nil { + log.Debugf("Found app %s with manifest-generate-paths(\"%s\") annotation that matches %s", app.Name, appManifestPathsAnnotation, componentPath) + return &app, nil + } + + } + + } + return nil, fmt.Errorf("No ArgoCD application found with manifest-generate-paths annotation that matches %s(looked at repo %s, checked %v apps) ", componentPath, repo, len(allRepoApps.Items)) +} + +func generateDiffOfAComponent(ctx context.Context, componentPath string, prBranch string, repo string, appIf application.ApplicationServiceClient, projIf projectpkg.ProjectServiceClient, argoSettings *settings.Settings, usaSHALabelForArgoDicovery bool) (componentDiffResult DiffResult) { + componentDiffResult.ComponentPath = componentPath + + // Find ArgoCD application by the path SHA1 label selector and repo name + // At the moment we assume one to one mapping between Telefonistka components and ArgoCD application + + var foundApp *argoappv1.Application + var err error + if usaSHALabelForArgoDicovery { + foundApp, err = findArgocdAppBySHA1Label(ctx, componentPath, repo, appIf) + } else { + foundApp, err = findArgocdAppByManifestPathAnnotation(ctx, componentPath, repo, appIf) + + } + if err != nil { + componentDiffResult.DiffError = err return componentDiffResult } // Get the application and its resources, resources are the live state of the application objects. + // The 2nd "app fetch" is needed for the "refreshTypeHArd", we don't want to do that to non-relevant apps" refreshType := string(argoappv1.RefreshTypeHard) appNameQuery := application.ApplicationQuery{ - Name: &foundApps.Items[0].Name, // we expect only one app with this label and repo selectors + Name: &foundApp.Name, // we expect only one app with this label and repo selectors Refresh: &refreshType, } app, err := appIf.Get(ctx, &appNameQuery) if err != nil { componentDiffResult.DiffError = err - log.Errorf("Error getting app %s: %v", foundApps.Items[0].Name, err) + log.Errorf("Error getting app %s: %v", foundApp.Name, err) return componentDiffResult } componentDiffResult.ArgoCdAppName = app.Name @@ -232,7 +294,7 @@ func generateDiffOfAComponent(ctx context.Context, componentPath string, prBranc } // GenerateDiffOfChangedComponents generates diff of changed components -func GenerateDiffOfChangedComponents(ctx context.Context, componentPathList []string, prBranch string, repo string) (hasComponentDiff bool, hasComponentDiffErrors bool, diffResults []DiffResult, err error) { +func GenerateDiffOfChangedComponents(ctx context.Context, componentPathList []string, prBranch string, repo string, usaSHALabelForArgoDicovery bool) (hasComponentDiff bool, hasComponentDiffErrors bool, diffResults []DiffResult, err error) { hasComponentDiff = false hasComponentDiffErrors = false // env var should be centralized @@ -264,7 +326,7 @@ func GenerateDiffOfChangedComponents(ctx context.Context, componentPathList []st } for _, componentPath := range componentPathList { - currentDiffResult := generateDiffOfAComponent(ctx, componentPath, prBranch, repo, appIf, projIf, argoSettings) + currentDiffResult := generateDiffOfAComponent(ctx, componentPath, prBranch, repo, appIf, projIf, argoSettings, usaSHALabelForArgoDicovery) if currentDiffResult.DiffError != nil { hasComponentDiffErrors = true err = currentDiffResult.DiffError diff --git a/internal/pkg/argocd/argocd_test.go b/internal/pkg/argocd/argocd_test.go new file mode 100644 index 00000000..dd89ee05 --- /dev/null +++ b/internal/pkg/argocd/argocd_test.go @@ -0,0 +1,196 @@ +package argocd + +// @Title +// @Description +// @Author +// @Update + +import ( + "context" + "testing" + + argoappv1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" + "github.com/golang/mock/gomock" + argo_app_mock "github.com/wayfair-incubator/telefonistka/mocks/argocd" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestFindArgocdAppBySHA1Label(t *testing.T) { + // Here the filtering is done on the ArgoCD server side, so we are just testing the function returns a app + t.Parallel() + ctx := context.Background() + ctrl := gomock.NewController(t) + defer ctrl.Finish() + mockApplicationClient := argo_app_mock.NewMockApplicationServiceClient(ctrl) + expectedResponse := &argoappv1.ApplicationList{ + Items: []argoappv1.Application{ + { + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + "telefonistka.io/component-path-sha1": "111111", + }, + Name: "right-app", + }, + }, + }, + } + + mockApplicationClient.EXPECT().List(gomock.Any(), gomock.Any()).Return(expectedResponse, nil) + + app, err := findArgocdAppBySHA1Label(ctx, "random/path", "some-repo", mockApplicationClient) + if err != nil { + t.Errorf("Error: %v", err) + } + if app.Name != "right-app" { + t.Errorf("App name is not right-app") + } +} + +func TestFindArgocdAppByPathAnnotation(t *testing.T) { + t.Parallel() + ctx := context.Background() + ctrl := gomock.NewController(t) + defer ctrl.Finish() + mockApplicationClient := argo_app_mock.NewMockApplicationServiceClient(ctrl) + expectedResponse := &argoappv1.ApplicationList{ + Items: []argoappv1.Application{ + { + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "argocd.argoproj.io/manifest-generate-paths": "wrong/path/", + }, + Name: "wrong-app", + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "argocd.argoproj.io/manifest-generate-paths": "right/path/", + }, + Name: "right-app", + }, + }, + }, + } + + mockApplicationClient.EXPECT().List(gomock.Any(), gomock.Any()).Return(expectedResponse, nil) + + apps, err := findArgocdAppByManifestPathAnnotation(ctx, "right/path", "some-repo", mockApplicationClient) + if err != nil { + t.Errorf("Error: %v", err) + } + t.Logf("apps: %v", apps) + +} + +// Here I'm testing a ";" delimted path annotation +func TestFindArgocdAppByPathAnnotationSemiColon(t *testing.T) { + t.Parallel() + ctx := context.Background() + ctrl := gomock.NewController(t) + defer ctrl.Finish() + mockApplicationClient := argo_app_mock.NewMockApplicationServiceClient(ctrl) + expectedResponse := &argoappv1.ApplicationList{ + Items: []argoappv1.Application{ + { + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "argocd.argoproj.io/manifest-generate-paths": "wrong/path/;wrong/path2/", + }, + Name: "wrong-app", + }, + }, + { // This is the app we want to find - it has the right path as one of the elements in the annotation + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "argocd.argoproj.io/manifest-generate-paths": "wrong/path/;right/path/", + }, + Name: "right-app", + }, + }, + }, + } + + mockApplicationClient.EXPECT().List(gomock.Any(), gomock.Any()).Return(expectedResponse, nil) + + app, err := findArgocdAppByManifestPathAnnotation(ctx, "right/path", "some-repo", mockApplicationClient) + if err != nil { + t.Errorf("Error: %v", err) + } + if app.Name != "right-app" { + t.Errorf("App name is not right-app") + } + +} + +// Here I'm testing a "." path annotation - this is a special case where the path is relative to the repo root specified in the application .spec +func TestFindArgocdAppByPathAnnotationRelative(t *testing.T) { + t.Parallel() + ctx := context.Background() + ctrl := gomock.NewController(t) + defer ctrl.Finish() + mockApplicationClient := argo_app_mock.NewMockApplicationServiceClient(ctrl) + expectedResponse := &argoappv1.ApplicationList{ + Items: []argoappv1.Application{ + { + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "argocd.argoproj.io/manifest-generate-paths": ".", + }, + Name: "right-app", + }, + Spec: argoappv1.ApplicationSpec{ + Source: &argoappv1.ApplicationSource{ + RepoURL: "", + Path: "right/path", + }, + }, + }, + }, + } + + mockApplicationClient.EXPECT().List(gomock.Any(), gomock.Any()).Return(expectedResponse, nil) + app, err := findArgocdAppByManifestPathAnnotation(ctx, "right/path", "some-repo", mockApplicationClient) + if err != nil { + t.Errorf("Error: %v", err) + } else if app.Name != "right-app" { + t.Errorf("App name is not right-app") + } + +} + +// Here I'm testing a "." path annotation - this is a special case where the path is relative to the repo root specified in the application .spec +func TestFindArgocdAppByPathAnnotationRelative2(t *testing.T) { + t.Parallel() + ctx := context.Background() + ctrl := gomock.NewController(t) + defer ctrl.Finish() + mockApplicationClient := argo_app_mock.NewMockApplicationServiceClient(ctrl) + expectedResponse := &argoappv1.ApplicationList{ + Items: []argoappv1.Application{ + { + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "argocd.argoproj.io/manifest-generate-paths": "./path", + }, + Name: "right-app", + }, + Spec: argoappv1.ApplicationSpec{ + Source: &argoappv1.ApplicationSource{ + RepoURL: "", + Path: "right/", + }, + }, + }, + }, + } + + mockApplicationClient.EXPECT().List(gomock.Any(), gomock.Any()).Return(expectedResponse, nil) + app, err := findArgocdAppByManifestPathAnnotation(ctx, "right/path", "some-repo", mockApplicationClient) + if err != nil { + t.Errorf("Error: %v", err) + } else if app.Name != "right-app" { + t.Errorf("App name is not right-app") + } + +} diff --git a/internal/pkg/configuration/config.go b/internal/pkg/configuration/config.go index ea822829..a9b675e7 100644 --- a/internal/pkg/configuration/config.go +++ b/internal/pkg/configuration/config.go @@ -35,13 +35,14 @@ type Config struct { PromotionPaths []PromotionPath `yaml:"promotionPaths"` // Generic configuration - PromtionPrLables []string `yaml:"promtionPRlables"` - DryRunMode bool `yaml:"dryRunMode"` - AutoApprovePromotionPrs bool `yaml:"autoApprovePromotionPrs"` - ToggleCommitStatus map[string]string `yaml:"toggleCommitStatus"` - WebhookEndpointRegexs []WebhookEndpointRegex `yaml:"webhookEndpointRegexs"` - CommentArgocdDiffonPR bool `yaml:"commentArgocdDiffonPR"` - AutoMergeNoDiffPRs bool `yaml:"autoMergeNoDiffPRs"` + PromtionPrLables []string `yaml:"promtionPRlables"` + DryRunMode bool `yaml:"dryRunMode"` + AutoApprovePromotionPrs bool `yaml:"autoApprovePromotionPrs"` + ToggleCommitStatus map[string]string `yaml:"toggleCommitStatus"` + WebhookEndpointRegexs []WebhookEndpointRegex `yaml:"webhookEndpointRegexs"` + CommentArgocdDiffonPR bool `yaml:"commentArgocdDiffonPR"` + AutoMergeNoDiffPRs bool `yaml:"autoMergeNoDiffPRs"` + UsaSHALabelForArgoDicovery bool `yaml:"usaSHALabelForArgoDicovery"` } func ParseConfigFromYaml(y string) (*Config, error) { diff --git a/internal/pkg/githubapi/github.go b/internal/pkg/githubapi/github.go index fc2c24cc..eab86c60 100644 --- a/internal/pkg/githubapi/github.go +++ b/internal/pkg/githubapi/github.go @@ -120,7 +120,7 @@ func HandlePREvent(eventPayload *github.PullRequestEvent, ghPrClientDetails GhPr } } - hasComponentDiff, hasComponentDiffErrors, diffOfChangedComponents, err := argocd.GenerateDiffOfChangedComponents(ctx, componentPathList, ghPrClientDetails.Ref, ghPrClientDetails.RepoURL) + hasComponentDiff, hasComponentDiffErrors, diffOfChangedComponents, err := argocd.GenerateDiffOfChangedComponents(ctx, componentPathList, ghPrClientDetails.Ref, ghPrClientDetails.RepoURL, config.UsaSHALabelForArgoDicovery) if err != nil { prHandleError = err ghPrClientDetails.PrLogger.Errorf("Failed to get ArgoCD diff information: err=%s\n", err) diff --git a/internal/pkg/mocks/.gitignore b/internal/pkg/mocks/.gitignore new file mode 100644 index 00000000..c0847efd --- /dev/null +++ b/internal/pkg/mocks/.gitignore @@ -0,0 +1 @@ +/argocd_mock_application.go diff --git a/internal/pkg/mocks/mocks.go b/internal/pkg/mocks/mocks.go new file mode 100644 index 00000000..51abb0af --- /dev/null +++ b/internal/pkg/mocks/mocks.go @@ -0,0 +1,7 @@ +package mocks + +// This package contains generated mocks + +// mockgen -package=mocks -destination authenticator.go k8s.io/apiserver/pkg/authentication/authenticator Token +// mockgen -source=../../../vendor/github.com/argoproj/argo-cd/v2/pkg/apiclient/application/application.pb.go -destination=mock_argocd_application.go -package=mocks +//go:generate mockgen -destination=argocd_application.go -package=mocks github.com/argoproj/argo-cd/v2/pkg/apiclient/application ApplicationServiceClient From 8a157ff40394815293314f204f1ec6af815c4cb2 Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Thu, 30 May 2024 08:50:34 +0200 Subject: [PATCH 04/15] Fix relevant components discovery hannes (#9) * mocks: use go run to reduce dependency Using go run will automatically install the tool and run it if it is not installed already. * argocd: fix import path The import path for the mocks package is incorrect causing errors. The mocks package should be imported from internal/pkg/mocks instead of mocks/argocd. This also removes the aliased import and uses the package as named, which aligns with the directory name. --------- Co-authored-by: Hannes Gustafsson --- internal/pkg/argocd/argocd_test.go | 12 ++++++------ internal/pkg/mocks/mocks.go | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/internal/pkg/argocd/argocd_test.go b/internal/pkg/argocd/argocd_test.go index dd89ee05..08f292cf 100644 --- a/internal/pkg/argocd/argocd_test.go +++ b/internal/pkg/argocd/argocd_test.go @@ -11,7 +11,7 @@ import ( argoappv1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" "github.com/golang/mock/gomock" - argo_app_mock "github.com/wayfair-incubator/telefonistka/mocks/argocd" + "github.com/wayfair-incubator/telefonistka/internal/pkg/mocks" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -21,7 +21,7 @@ func TestFindArgocdAppBySHA1Label(t *testing.T) { ctx := context.Background() ctrl := gomock.NewController(t) defer ctrl.Finish() - mockApplicationClient := argo_app_mock.NewMockApplicationServiceClient(ctrl) + mockApplicationClient := mocks.NewMockApplicationServiceClient(ctrl) expectedResponse := &argoappv1.ApplicationList{ Items: []argoappv1.Application{ { @@ -51,7 +51,7 @@ func TestFindArgocdAppByPathAnnotation(t *testing.T) { ctx := context.Background() ctrl := gomock.NewController(t) defer ctrl.Finish() - mockApplicationClient := argo_app_mock.NewMockApplicationServiceClient(ctrl) + mockApplicationClient := mocks.NewMockApplicationServiceClient(ctrl) expectedResponse := &argoappv1.ApplicationList{ Items: []argoappv1.Application{ { @@ -89,7 +89,7 @@ func TestFindArgocdAppByPathAnnotationSemiColon(t *testing.T) { ctx := context.Background() ctrl := gomock.NewController(t) defer ctrl.Finish() - mockApplicationClient := argo_app_mock.NewMockApplicationServiceClient(ctrl) + mockApplicationClient := mocks.NewMockApplicationServiceClient(ctrl) expectedResponse := &argoappv1.ApplicationList{ Items: []argoappv1.Application{ { @@ -129,7 +129,7 @@ func TestFindArgocdAppByPathAnnotationRelative(t *testing.T) { ctx := context.Background() ctrl := gomock.NewController(t) defer ctrl.Finish() - mockApplicationClient := argo_app_mock.NewMockApplicationServiceClient(ctrl) + mockApplicationClient := mocks.NewMockApplicationServiceClient(ctrl) expectedResponse := &argoappv1.ApplicationList{ Items: []argoappv1.Application{ { @@ -165,7 +165,7 @@ func TestFindArgocdAppByPathAnnotationRelative2(t *testing.T) { ctx := context.Background() ctrl := gomock.NewController(t) defer ctrl.Finish() - mockApplicationClient := argo_app_mock.NewMockApplicationServiceClient(ctrl) + mockApplicationClient := mocks.NewMockApplicationServiceClient(ctrl) expectedResponse := &argoappv1.ApplicationList{ Items: []argoappv1.Application{ { diff --git a/internal/pkg/mocks/mocks.go b/internal/pkg/mocks/mocks.go index 51abb0af..24240dd3 100644 --- a/internal/pkg/mocks/mocks.go +++ b/internal/pkg/mocks/mocks.go @@ -4,4 +4,4 @@ package mocks // mockgen -package=mocks -destination authenticator.go k8s.io/apiserver/pkg/authentication/authenticator Token // mockgen -source=../../../vendor/github.com/argoproj/argo-cd/v2/pkg/apiclient/application/application.pb.go -destination=mock_argocd_application.go -package=mocks -//go:generate mockgen -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_application.go -package=mocks github.com/argoproj/argo-cd/v2/pkg/apiclient/application ApplicationServiceClient From 7a525524208d98397a2915c7722838547bf86d96 Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Thu, 30 May 2024 08:57:46 +0200 Subject: [PATCH 05/15] Add needed argodiff.NewDiffConfigBuilder param (after version upgrade) Log time of API call with questionable performance :) --- internal/pkg/argocd/argocd.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/internal/pkg/argocd/argocd.go b/internal/pkg/argocd/argocd.go index 0abe8925..e104fa47 100644 --- a/internal/pkg/argocd/argocd.go +++ b/internal/pkg/argocd/argocd.go @@ -10,6 +10,7 @@ import ( "path/filepath" "strconv" "strings" + "time" cmdutil "github.com/argoproj/argo-cd/v2/cmd/util" "github.com/argoproj/argo-cd/v2/pkg/apiclient" @@ -18,6 +19,7 @@ import ( "github.com/argoproj/argo-cd/v2/pkg/apiclient/settings" argoappv1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" argodiff "github.com/argoproj/argo-cd/v2/util/argo/diff" + "github.com/argoproj/argo-cd/v2/util/argo/normalizers" argoio "github.com/argoproj/argo-cd/v2/util/io" "github.com/argoproj/gitops-engine/pkg/sync/hook" "github.com/google/go-cmp/cmp" @@ -76,8 +78,9 @@ func generateArgocdAppDiff(ctx context.Context, app *argoappv1.Application, proj } ignoreAggregatedRoles := false + ignoreNormalizerOpts := normalizers.IgnoreNormalizerOpts{} diffConfig, err := argodiff.NewDiffConfigBuilder(). - WithDiffSettings(app.Spec.IgnoreDifferences, overrides, ignoreAggregatedRoles). + WithDiffSettings(app.Spec.IgnoreDifferences, overrides, ignoreAggregatedRoles, ignoreNormalizerOpts). WithTracking(argoSettings.AppLabelKey, argoSettings.TrackingMethod). WithNoCache(). Build() @@ -186,8 +189,12 @@ func findArgocdAppByManifestPathAnnotation(ctx context.Context, componentPath st appQuery := application.ApplicationQuery{ Repo: &repo, } - // TODO Instrument this, we might have a lot of apps in a repo, performance might be an issue + // AFAIKT I can't use standard grpc instrumentation here, since the argocd client abstracts to much (including the choice between Grpc and Grpc-web) + // I'll just manually log the time it takes to get the apps for now + getAppsStart := time.Now() allRepoApps, err := appIf.List(ctx, &appQuery) + getAppsDuration := time.Since(getAppsStart).Milliseconds + log.Infof("Got %v ArgoCD applications for repo %s in %v ms", len(allRepoApps.Items), repo, getAppsDuration) if err != nil { return nil, err } From 0e579b13550068f94083752f4714e7bf3524f54d Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Thu, 30 May 2024 09:39:19 +0200 Subject: [PATCH 06/15] Fix typo and add code generate step to makefile --- Makefile | 3 ++- internal/pkg/argocd/argocd.go | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 60fb3c13..26adb331 100644 --- a/Makefile +++ b/Makefile @@ -12,6 +12,7 @@ VENDOR_DIR = vendor get-deps: $(VENDOR_DIR) $(VENDOR_DIR): + go generate $$(go list ./internal/pkg/mocks/...) GO111MODULE=on go mod vendor .PHONY: build @@ -24,5 +25,5 @@ clean: .PHONY: test test: $(VENDOR_DIR) - go test -v -timeout 30s . + go test -v -timeout 30s ./... diff --git a/internal/pkg/argocd/argocd.go b/internal/pkg/argocd/argocd.go index e104fa47..0e1aab84 100644 --- a/internal/pkg/argocd/argocd.go +++ b/internal/pkg/argocd/argocd.go @@ -193,7 +193,7 @@ func findArgocdAppByManifestPathAnnotation(ctx context.Context, componentPath st // I'll just manually log the time it takes to get the apps for now getAppsStart := time.Now() allRepoApps, err := appIf.List(ctx, &appQuery) - getAppsDuration := time.Since(getAppsStart).Milliseconds + getAppsDuration := time.Since(getAppsStart).Milliseconds() log.Infof("Got %v ArgoCD applications for repo %s in %v ms", len(allRepoApps.Items), repo, getAppsDuration) if err != nil { return nil, err From e6c120c1e6a72aa7846c4c2ab17e239dbc7b96b5 Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Thu, 30 May 2024 09:50:22 +0200 Subject: [PATCH 07/15] Lint fixes, fix gitignore. upsate go.mod --- go.mod | 10 +++++----- go.sum | 17 ----------------- internal/pkg/argocd/argocd.go | 9 +++------ internal/pkg/argocd/argocd_test.go | 4 ---- internal/pkg/mocks/.gitignore | 2 +- 5 files changed, 9 insertions(+), 33 deletions(-) diff --git a/go.mod b/go.mod index d1872aa9..04bd3b00 100644 --- a/go.mod +++ b/go.mod @@ -10,19 +10,22 @@ require ( github.com/argoproj/gitops-engine v0.7.1-0.20240416142647-fbecbb86e412 github.com/bradleyfalzon/ghinstallation/v2 v2.10.0 github.com/go-test/deep v1.1.0 + github.com/golang/mock v1.6.0 + github.com/google/go-cmp v0.6.0 github.com/google/go-github/v52 v52.0.0 github.com/hashicorp/golang-lru/v2 v2.0.7 github.com/hexops/gotextdiff v1.0.3 github.com/migueleliasweb/go-github-mock v0.0.22 github.com/mikefarah/yq/v4 v4.43.1 github.com/prometheus/client_golang v1.19.0 - github.com/sergi/go-diff v1.3.2-0.20230802210424-5b0b94c5c0d3 github.com/shurcooL/githubv4 v0.0.0-20240120211514-18a1ae0e79dc github.com/sirupsen/logrus v1.9.3 github.com/spf13/cobra v1.8.0 golang.org/x/exp v0.0.0-20240506185415-9bf2ced13842 golang.org/x/oauth2 v0.19.0 + google.golang.org/grpc v1.63.2 gopkg.in/yaml.v2 v2.4.0 + k8s.io/api v0.26.11 k8s.io/apimachinery v0.26.11 ) @@ -77,12 +80,10 @@ require ( github.com/gogo/protobuf v1.3.2 // indirect github.com/golang-jwt/jwt/v4 v4.5.0 // indirect github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect - github.com/golang/mock v1.6.0 // indirect github.com/golang/protobuf v1.5.4 // indirect github.com/google/btree v1.1.2 // indirect github.com/google/gnostic v0.7.0 // indirect github.com/google/gnostic-models v0.6.9-0.20230804172637-c7be7c783f49 // indirect - github.com/google/go-cmp v0.6.0 // indirect github.com/google/go-github/v56 v56.0.0 // indirect github.com/google/go-github/v60 v60.0.0 // indirect github.com/google/go-querystring v1.1.0 // indirect @@ -135,6 +136,7 @@ require ( github.com/redis/go-redis/v9 v9.5.1 // indirect github.com/robfig/cron/v3 v3.0.1 // indirect github.com/russross/blackfriday/v2 v2.1.0 // indirect + github.com/sergi/go-diff v1.3.2-0.20230802210424-5b0b94c5c0d3 // indirect github.com/shurcooL/graphql v0.0.0-20230722043721-ed46e5a46466 // indirect github.com/skeema/knownhosts v1.2.2 // indirect github.com/spf13/pflag v1.0.5 // indirect @@ -162,13 +164,11 @@ require ( google.golang.org/genproto v0.0.0-20240401170217-c3f982113cda // indirect google.golang.org/genproto/googleapis/api v0.0.0-20240401170217-c3f982113cda // indirect google.golang.org/genproto/googleapis/rpc v0.0.0-20240401170217-c3f982113cda // indirect - google.golang.org/grpc v1.63.2 // indirect google.golang.org/protobuf v1.33.0 // indirect gopkg.in/inf.v0 v0.9.1 // indirect gopkg.in/op/go-logging.v1 v1.0.0-20160211212156-b2cb9fa56473 // indirect gopkg.in/warnings.v0 v0.1.2 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect - k8s.io/api v0.26.11 // indirect k8s.io/apiextensions-apiserver v0.26.10 // indirect k8s.io/apiserver v0.26.11 // indirect k8s.io/cli-runtime v0.26.11 // indirect diff --git a/go.sum b/go.sum index 79b5a6dd..abb4c1ba 100644 --- a/go.sum +++ b/go.sum @@ -649,12 +649,8 @@ github.com/antihax/optional v1.0.0/go.mod h1:uupD/76wgC+ih3iEmQUL+0Ugr19nfwCT1kd github.com/apache/arrow/go/v10 v10.0.1/go.mod h1:YvhnlEePVnBS4+0z3fhPfUy7W1Ikj0Ih0vcRo/gZ1M0= github.com/apache/arrow/go/v11 v11.0.0/go.mod h1:Eg5OsL5H+e299f7u5ssuXsuHQVEGC4xei5aX110hRiI= github.com/apache/thrift v0.16.0/go.mod h1:PHK3hniurgQaNMZYaCLEqXKsYK8upmhPbmdP2FXSqgU= -github.com/argoproj/argo-cd/v2 v2.11.0-rc1 h1:VCjpw0bwPcULNJ/FG8BnIyeesyOpT+1MUWuXPskbsWQ= -github.com/argoproj/argo-cd/v2 v2.11.0-rc1/go.mod h1:/KySYrOzPQupCh7E1pzg6011p4AaRLszbUtkaWyATTU= github.com/argoproj/argo-cd/v2 v2.11.2 h1:NygNrTFIMWUe1b48ddUuH+q2vRTHB+dFk3NcErx6GcM= github.com/argoproj/argo-cd/v2 v2.11.2/go.mod h1:nUOZqAT9f3GewdG/dzYgrpwqOSMj5ukoWw4yAV2/WXA= -github.com/argoproj/gitops-engine v0.7.1-0.20240411122334-1ade3a199867 h1:zMATM3uzAQHBLJ142MEGrZ4+3+xsXT36hzB1Dj2jptE= -github.com/argoproj/gitops-engine v0.7.1-0.20240411122334-1ade3a199867/go.mod h1:gWE8uROi7hIkWGNAVM+8FWkMfo0vZ03SLx/aFw/DBzg= github.com/argoproj/gitops-engine v0.7.1-0.20240416142647-fbecbb86e412 h1:je2wJpWtaoS55mA5MBPCeDnKMeF42pkxO9Oa5KbWrdg= github.com/argoproj/gitops-engine v0.7.1-0.20240416142647-fbecbb86e412/go.mod h1:gWE8uROi7hIkWGNAVM+8FWkMfo0vZ03SLx/aFw/DBzg= github.com/argoproj/pkg v0.13.7-0.20230626144333-d56162821bd1 h1:qsHwwOJ21K2Ao0xPju1sNuqphyMnMYkyB3ZLoLtxWpo= @@ -1317,8 +1313,6 @@ golang.org/x/crypto v0.7.0/go.mod h1:pYwdfH91IfpZVANVyUOhSIPZaFoJGxTFbZhFTx+dXZU golang.org/x/crypto v0.9.0/go.mod h1:yrmDGqONDYtNj3tH8X9dzUun2m2lzPa9ngI6/RUPGR0= golang.org/x/crypto v0.10.0/go.mod h1:o4eNf7Ede1fv+hwOwZsTHl9EsPFO6q6ZvYR8vYfY45I= golang.org/x/crypto v0.19.0/go.mod h1:Iy9bg/ha4yyC70EfRS8jz+B6ybOBKMaSxLj6P6oBDfU= -golang.org/x/crypto v0.22.0 h1:g1v0xeRhjcugydODzvb3mEM9SQ0HGp9s/nh3COQ/C30= -golang.org/x/crypto v0.22.0/go.mod h1:vr6Su+7cTlO45qkww3VDJlzDn0ctJvRgYbC2NvXHt+M= golang.org/x/crypto v0.23.0 h1:dIJU/v2J8Mdglj/8rJ6UUOM3Zc9zLZxVZwwxMooUSAI= golang.org/x/crypto v0.23.0/go.mod h1:CKFgDieR+mRhux2Lsu27y0fO304Db0wZe70UKqHu0v8= golang.org/x/exp v0.0.0-20180321215751-8460e604b9de/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA= @@ -1336,8 +1330,6 @@ golang.org/x/exp v0.0.0-20200119233911-0405dc783f0a/go.mod h1:2RIsYlXP63K8oxa1u0 golang.org/x/exp v0.0.0-20200207192155-f17229e696bd/go.mod h1:J/WKrq2StrnmMY6+EHIKF9dgMWnmCNThgcyBT1FY9mM= golang.org/x/exp v0.0.0-20200224162631-6cc2880d07d6/go.mod h1:3jZMyOhIsHpP37uCMkUooju7aAi5cS1Q23tOzKc+0MU= golang.org/x/exp v0.0.0-20220827204233-334a2380cb91/go.mod h1:cyybsKvd6eL0RnXn6p/Grxp8F5bW7iYuBgsNCOHpMYE= -golang.org/x/exp v0.0.0-20240409090435-93d18d7e34b8 h1:ESSUROHIBHg7USnszlcdmjBEwdMj9VUvU+OPk4yl2mc= -golang.org/x/exp v0.0.0-20240409090435-93d18d7e34b8/go.mod h1:/lliqkxwWAhPjf5oSOIJup2XcqJaw8RGS6k3TGEc7GI= golang.org/x/exp v0.0.0-20240506185415-9bf2ced13842 h1:vr/HnozRka3pE4EsMEg1lgkXJkTFJCVUX+S/ZT6wYzM= golang.org/x/exp v0.0.0-20240506185415-9bf2ced13842/go.mod h1:XtvwrStGgqGPLc4cjQfWqZHG1YFdYs6swckp8vpsjnc= golang.org/x/image v0.0.0-20180708004352-c73c2afc3b81/go.mod h1:ux5Hcp/YLpHSI86hEcLt0YII63i6oz57MZXIpbrjZUs= @@ -1451,8 +1443,6 @@ golang.org/x/net v0.7.0/go.mod h1:2Tu9+aMcznHK/AK1HMvgo6xiTLG5rD5rZLDS+rp2Bjs= golang.org/x/net v0.8.0/go.mod h1:QVkue5JL9kW//ek3r6jTKnTFis1tRmNAW2P1shuFdJc= golang.org/x/net v0.10.0/go.mod h1:0qNGK6F8kojg2nk9dLZ2mShWaEBan6FAoqfSigmmuDg= golang.org/x/net v0.11.0/go.mod h1:2L/ixqYpgIVXmeoSA/4Lu7BzTG4KIyPIryS4IsOd1oQ= -golang.org/x/net v0.24.0 h1:1PcaxkF854Fu3+lvBIx5SYn9wRlBzzcnHZSiaFFAb0w= -golang.org/x/net v0.24.0/go.mod h1:2Q7sJY5mzlzWjKtYUEXSlBWCdyaioyXzRB2RtU8KVE8= golang.org/x/net v0.25.0 h1:d/OCCoBEUq33pjydKrGQhw7IlUPI2Oylr+8qLx49kac= golang.org/x/net v0.25.0/go.mod h1:JkAGAh7GEvH74S6FOH42FLoXpXbE/aqXSrIQjXgsiwM= golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U= @@ -1600,8 +1590,6 @@ golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.8.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.9.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.17.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= -golang.org/x/sys v0.19.0 h1:q5f1RH2jigJ1MoAWp2KTp3gm5zAGFUTarQZ5U386+4o= -golang.org/x/sys v0.19.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= golang.org/x/sys v0.20.0 h1:Od9JTbYCk261bKm4M/mw7AklTlFYIa0bIp9BgSm1S8Y= golang.org/x/sys v0.20.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= @@ -1615,8 +1603,6 @@ golang.org/x/term v0.6.0/go.mod h1:m6U89DPEgQRMq3DNkDClhWw02AUbt2daBVO4cn4Hv9U= golang.org/x/term v0.8.0/go.mod h1:xPskH00ivmX89bAKVGSKKtLOWNx2+17Eiy94tnKShWo= golang.org/x/term v0.9.0/go.mod h1:M6DEAAIenWoTxdKrOltXcmDY3rSplQUkrvaDU5FcQyo= golang.org/x/term v0.17.0/go.mod h1:lLRBjIVuehSbZlaOtGMbcMncT+aqLLLmKrsjNrUguwk= -golang.org/x/term v0.19.0 h1:+ThwsDv+tYfnJFhF4L8jITxu1tdTWRTZpdsWgEgjL6Q= -golang.org/x/term v0.19.0/go.mod h1:2CuTdWZ7KHSQwUzKva0cbMg6q2DMI3Mmxp+gKJbskEk= golang.org/x/term v0.20.0 h1:VnkxpohqXaOBYJtBmEppKUG6mXpi+4O6purfc2+sMhw= golang.org/x/term v0.20.0/go.mod h1:8UkIAJTvZgivsXaD6/pH6U9ecQzZ45awqEOzuCvwpFY= golang.org/x/text v0.0.0-20160726164857-2910a502d2bf/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= @@ -1637,7 +1623,6 @@ golang.org/x/text v0.7.0/go.mod h1:mrYo+phRRbMaCq/xk9113O4dZlRixOauAjOtrjsXDZ8= golang.org/x/text v0.8.0/go.mod h1:e1OnstbJyHTd6l/uOt8jFFHp6TRDWZR/bV3emEE/zU8= golang.org/x/text v0.9.0/go.mod h1:e1OnstbJyHTd6l/uOt8jFFHp6TRDWZR/bV3emEE/zU8= golang.org/x/text v0.10.0/go.mod h1:TvPlkZtksWOMsz7fbANvkp4WM8x/WCo/om8BMLbz+aE= -golang.org/x/text v0.14.0 h1:ScX5w1eTa3QqT8oi6+ziP7dTV1S2+ALU0bI+0zXKWiQ= golang.org/x/text v0.14.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU= golang.org/x/text v0.15.0 h1:h1V/4gjBv8v9cjcR6+AR5+/cIYK5N/WAgiv4xlsEtAk= golang.org/x/text v0.15.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU= @@ -1718,8 +1703,6 @@ golang.org/x/tools v0.2.0/go.mod h1:y4OqIKeOV/fWJetJ8bXPU1sEVniLMIyDAZWeHdV+NTA= golang.org/x/tools v0.3.0/go.mod h1:/rWhSS2+zyEVwoJf8YAX6L2f0ntZ7Kn/mGgAWcipA5k= golang.org/x/tools v0.4.0/go.mod h1:UE5sM2OK9E/d67R0ANs2xJizIymRP5gJU295PvKXxjQ= golang.org/x/tools v0.6.0/go.mod h1:Xwgl3UAJ/d3gWutnCtw505GrjyAbvKui8lOU390QaIU= -golang.org/x/tools v0.20.0 h1:hz/CVckiOxybQvFw6h7b/q80NTr9IUQb4s1IIzW7KNY= -golang.org/x/tools v0.20.0/go.mod h1:WvitBU7JJf6A4jOdg4S1tviW9bhUxkgeCui/0JHctQg= golang.org/x/tools v0.21.0 h1:qc0xYgIbsSDt9EyWz05J5wfa7LOVW0YTLOXrqdLAWIw= golang.org/x/tools v0.21.0/go.mod h1:aiJjzUbINMkxbQROHiO6hDPo2LHcIPhhQsa9DLh0yGk= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= diff --git a/internal/pkg/argocd/argocd.go b/internal/pkg/argocd/argocd.go index 0e1aab84..a6951cab 100644 --- a/internal/pkg/argocd/argocd.go +++ b/internal/pkg/argocd/argocd.go @@ -156,7 +156,7 @@ func createArgoCdClient() (apiclient.Client, error) { return clientset, nil } -// This function is used to find an ArgoCD application by the SHA1 label of the component path its supposed to avoid perfromance issues with the "manifest-generate-paths" annotation method with requires pulling all ArgoCD applications(!) on every PR event. +// This function is used to find an ArgoCD application by the SHA1 label of the component path its supposed to avoid performance issues with the "manifest-generate-paths" annotation method with requires pulling all ArgoCD applications(!) on every PR event. // The SHA1 label is assumed to be populated by the ApplicationSet controller(or apps of apps or similar). func findArgocdAppBySHA1Label(ctx context.Context, componentPath string, repo string, appIf application.ApplicationServiceClient) (app *argoappv1.Application, err error) { // Calculate sha1 of component path to use in a label selector @@ -182,10 +182,10 @@ func findArgocdAppBySHA1Label(ctx context.Context, componentPath string, repo st } // This is the default method to find an ArgoCD application by the manifest-generate-paths annotation. -// It assume the ArgoCD (optional) manifest-generate-paths annotation is set on all relevent apps. +// It assume the ArgoCD (optional) manifest-generate-paths annotation is set on all relevant apps. // Notice that this method include a full list all ArgoCD applications in the repo, this could be a performance issue if there are many apps in the repo. func findArgocdAppByManifestPathAnnotation(ctx context.Context, componentPath string, repo string, appIf application.ApplicationServiceClient) (app *argoappv1.Application, err error) { - //argocd.argoproj.io/manifest-generate-paths + // argocd.argoproj.io/manifest-generate-paths appQuery := application.ApplicationQuery{ Repo: &repo, } @@ -219,9 +219,7 @@ func findArgocdAppByManifestPathAnnotation(ctx context.Context, componentPath st log.Debugf("Found app %s with manifest-generate-paths(\"%s\") annotation that matches %s", app.Name, appManifestPathsAnnotation, componentPath) return &app, nil } - } - } return nil, fmt.Errorf("No ArgoCD application found with manifest-generate-paths annotation that matches %s(looked at repo %s, checked %v apps) ", componentPath, repo, len(allRepoApps.Items)) } @@ -238,7 +236,6 @@ func generateDiffOfAComponent(ctx context.Context, componentPath string, prBranc foundApp, err = findArgocdAppBySHA1Label(ctx, componentPath, repo, appIf) } else { foundApp, err = findArgocdAppByManifestPathAnnotation(ctx, componentPath, repo, appIf) - } if err != nil { componentDiffResult.DiffError = err diff --git a/internal/pkg/argocd/argocd_test.go b/internal/pkg/argocd/argocd_test.go index 08f292cf..33dba88e 100644 --- a/internal/pkg/argocd/argocd_test.go +++ b/internal/pkg/argocd/argocd_test.go @@ -80,7 +80,6 @@ func TestFindArgocdAppByPathAnnotation(t *testing.T) { t.Errorf("Error: %v", err) } t.Logf("apps: %v", apps) - } // Here I'm testing a ";" delimted path annotation @@ -120,7 +119,6 @@ func TestFindArgocdAppByPathAnnotationSemiColon(t *testing.T) { if app.Name != "right-app" { t.Errorf("App name is not right-app") } - } // Here I'm testing a "." path annotation - this is a special case where the path is relative to the repo root specified in the application .spec @@ -156,7 +154,6 @@ func TestFindArgocdAppByPathAnnotationRelative(t *testing.T) { } else if app.Name != "right-app" { t.Errorf("App name is not right-app") } - } // Here I'm testing a "." path annotation - this is a special case where the path is relative to the repo root specified in the application .spec @@ -192,5 +189,4 @@ func TestFindArgocdAppByPathAnnotationRelative2(t *testing.T) { } else if app.Name != "right-app" { t.Errorf("App name is not right-app") } - } diff --git a/internal/pkg/mocks/.gitignore b/internal/pkg/mocks/.gitignore index c0847efd..191fc316 100644 --- a/internal/pkg/mocks/.gitignore +++ b/internal/pkg/mocks/.gitignore @@ -1 +1 @@ -/argocd_mock_application.go +/argocd_application.go From 29e72b08d96fc842047b9e2d8d79bff0d21dbffa Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Thu, 30 May 2024 10:17:39 +0200 Subject: [PATCH 08/15] Hack go genrate for linting --- .github/workflows/lint.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 8edb4465..d55d29a5 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -34,6 +34,7 @@ jobs: with: go-version: 1.21 - uses: actions/checkout@v4 + - run: go generate $(go list ./internal/pkg/mocks/...) - name: golangci-lint uses: golangci/golangci-lint-action@v4 with: From a9ab8296a8ff2fddf1a80ea7bb5f7e4985c74579 Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Thu, 30 May 2024 10:53:07 +0200 Subject: [PATCH 09/15] Cleanup and try to use make file config --- .github/workflows/lint.yml | 2 +- internal/pkg/mocks/mocks.go | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index d55d29a5..1c4d2cec 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -34,7 +34,7 @@ jobs: with: go-version: 1.21 - uses: actions/checkout@v4 - - run: go generate $(go list ./internal/pkg/mocks/...) + - run: make get-deps - name: golangci-lint uses: golangci/golangci-lint-action@v4 with: diff --git a/internal/pkg/mocks/mocks.go b/internal/pkg/mocks/mocks.go index 24240dd3..0fbed2e1 100644 --- a/internal/pkg/mocks/mocks.go +++ b/internal/pkg/mocks/mocks.go @@ -2,6 +2,4 @@ package mocks // This package contains generated mocks -// mockgen -package=mocks -destination authenticator.go k8s.io/apiserver/pkg/authentication/authenticator Token -// mockgen -source=../../../vendor/github.com/argoproj/argo-cd/v2/pkg/apiclient/application/application.pb.go -destination=mock_argocd_application.go -package=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 From 03b61cc7db65a6bb8418b18297edd109ac5c9d27 Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Thu, 30 May 2024 13:02:46 +0200 Subject: [PATCH 10/15] Document new configuration key --- docs/installation.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/installation.md b/docs/installation.md index 050eb35d..1e77a087 100644 --- a/docs/installation.md +++ b/docs/installation.md @@ -119,6 +119,7 @@ Configuration keys: |`toggleCommitStatus`| Map of strings, allow (non-repo-admin) users to change the [Github commit status](https://docs.github.com/en/rest/commits/statuses) state(from failure to success and back). This can be used to continue promotion of a change that doesn't pass repo checks. the keys are strings commented in the PRs, values are [Github commit status context](https://docs.github.com/en/rest/commits/statuses?apiVersion=2022-11-28#create-a-commit-status) to be overridden| |`commentArgocdDiffonPR`| Uses ArgoCD API to calculate expected changes to k8s state and comment the resulting "diff" as comment in the PR. Requires ARGOCD_* environment variables, see below. | |`autoMergeNoDiffPRs`| if true, Telefonistka will **merge** promotion PRs that are not expected to change the target clusters. Requires `commentArgocdDiffonPR` and possibly `autoApprovePromotionPrs`(depending on repo branch protection rules)| +|`usaSHALabelForArgoDicovery`| The Default method for discovering relevant ArgoCD applications(for a PR) rely on fetching all appliation in repo and checking the `argocd.argoproj.io/manifest-generate-paths` **annotation**, this might cause perfromance issue on repo with large number of ArgoCD applications. The alternative is to add SHA1 of the application path as a **label** and relay on ArgoCD server side filtering, label name is `telefonistka.io/component-path-sha1`.| Example: From 0c25bc9d413d52263beca5e33602fc4b874270f5 Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Wed, 12 Jun 2024 10:02:29 +0200 Subject: [PATCH 11/15] Update argocd_test.go - remove some editor completion artifacts --- internal/pkg/argocd/argocd_test.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/internal/pkg/argocd/argocd_test.go b/internal/pkg/argocd/argocd_test.go index 33dba88e..3bda870c 100644 --- a/internal/pkg/argocd/argocd_test.go +++ b/internal/pkg/argocd/argocd_test.go @@ -1,10 +1,5 @@ package argocd -// @Title -// @Description -// @Author -// @Update - import ( "context" "testing" From b35fdff50f26b48c35f798b2c29da39565c90af2 Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Wed, 12 Jun 2024 11:52:25 +0200 Subject: [PATCH 12/15] Fix two "bad conflict resolution" issues --- internal/pkg/argocd/argocd.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/internal/pkg/argocd/argocd.go b/internal/pkg/argocd/argocd.go index 00d4b4f3..63e12887 100644 --- a/internal/pkg/argocd/argocd.go +++ b/internal/pkg/argocd/argocd.go @@ -172,9 +172,8 @@ func findArgocdAppBySHA1Label(ctx context.Context, componentPath string, repo st } foundApps, err := appIf.List(ctx, &appLabelQuery) if err != nil { - log.Errorf("Error listing ArgoCD applications: %v", err) - componentDiffResult.DiffError = err - return componentDiffResult + return nil, fmt.Errorf("Error listing ArgoCD applications: %v", err) + } if len(foundApps.Items) == 0 { return nil, fmt.Errorf("No ArgoCD application found for component path sha1 %s(repo %s), used this label selector: %s", componentPathSha1, repo, labelSelector) @@ -245,7 +244,7 @@ func generateDiffOfAComponent(ctx context.Context, componentPath string, prBranc return componentDiffResult } - log.Debugf("Found ArgoCD application: %s", foundApps.Items[0].Name) + log.Debugf("Found ArgoCD application: %s", foundApp.Name) // Get the application and its resources, resources are the live state of the application objects. // The 2nd "app fetch" is needed for the "refreshTypeHArd", we don't want to do that to non-relevant apps" refreshType := string(argoappv1.RefreshTypeHard) From 134d7c3ca20e98e34e33ac204ef24f3eb5d6400c Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Wed, 12 Jun 2024 13:13:54 +0200 Subject: [PATCH 13/15] Apply suggestions from code review Co-authored-by: Yazdan Mohammadi --- docs/installation.md | 2 +- internal/pkg/argocd/argocd.go | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/installation.md b/docs/installation.md index 1e77a087..0ccdc27d 100644 --- a/docs/installation.md +++ b/docs/installation.md @@ -119,7 +119,7 @@ Configuration keys: |`toggleCommitStatus`| Map of strings, allow (non-repo-admin) users to change the [Github commit status](https://docs.github.com/en/rest/commits/statuses) state(from failure to success and back). This can be used to continue promotion of a change that doesn't pass repo checks. the keys are strings commented in the PRs, values are [Github commit status context](https://docs.github.com/en/rest/commits/statuses?apiVersion=2022-11-28#create-a-commit-status) to be overridden| |`commentArgocdDiffonPR`| Uses ArgoCD API to calculate expected changes to k8s state and comment the resulting "diff" as comment in the PR. Requires ARGOCD_* environment variables, see below. | |`autoMergeNoDiffPRs`| if true, Telefonistka will **merge** promotion PRs that are not expected to change the target clusters. Requires `commentArgocdDiffonPR` and possibly `autoApprovePromotionPrs`(depending on repo branch protection rules)| -|`usaSHALabelForArgoDicovery`| The Default method for discovering relevant ArgoCD applications(for a PR) rely on fetching all appliation in repo and checking the `argocd.argoproj.io/manifest-generate-paths` **annotation**, this might cause perfromance issue on repo with large number of ArgoCD applications. The alternative is to add SHA1 of the application path as a **label** and relay on ArgoCD server side filtering, label name is `telefonistka.io/component-path-sha1`.| +|`useSHALabelForArgoDicovery`| The default method for discovering relevant ArgoCD applications (for a PR) relies on fetching all applications in the repo and checking the `argocd.argoproj.io/manifest-generate-paths` **annotation**, this might cause a performance issue on a repo with a large number of ArgoCD applications. The alternative is to add SHA1 of the application path as a **label** and rely on ArgoCD server-side filtering, label name is `telefonistka.io/component-path-sha1`.| Example: diff --git a/internal/pkg/argocd/argocd.go b/internal/pkg/argocd/argocd.go index 63e12887..00e77273 100644 --- a/internal/pkg/argocd/argocd.go +++ b/internal/pkg/argocd/argocd.go @@ -156,7 +156,7 @@ func createArgoCdClient() (apiclient.Client, error) { return clientset, nil } -// This function is used to find an ArgoCD application by the SHA1 label of the component path its supposed to avoid performance issues with the "manifest-generate-paths" annotation method with requires pulling all ArgoCD applications(!) on every PR event. +// findArgocdAppBySHA1Label finds an ArgoCD application by the SHA1 label of the component path it's supposed to avoid performance issues with the "manifest-generate-paths" annotation method which requires pulling all ArgoCD applications(!) on every PR event. // The SHA1 label is assumed to be populated by the ApplicationSet controller(or apps of apps or similar). func findArgocdAppBySHA1Label(ctx context.Context, componentPath string, repo string, appIf application.ApplicationServiceClient) (app *argoappv1.Application, err error) { // Calculate sha1 of component path to use in a label selector @@ -183,15 +183,15 @@ func findArgocdAppBySHA1Label(ctx context.Context, componentPath string, repo st return &foundApps.Items[0], nil } -// This is the default method to find an ArgoCD application by the manifest-generate-paths annotation. -// It assume the ArgoCD (optional) manifest-generate-paths annotation is set on all relevant apps. -// Notice that this method include a full list all ArgoCD applications in the repo, this could be a performance issue if there are many apps in the repo. +// findArgocdAppByManifestPathAnnotation is the default method to find an ArgoCD application by the manifest-generate-paths annotation. +// It assumes the ArgoCD (optional) manifest-generate-paths annotation is set on all relevant apps. +// Notice that this method includes a full list of all ArgoCD applications in the repo, this could be a performance issue if there are many apps in the repo. func findArgocdAppByManifestPathAnnotation(ctx context.Context, componentPath string, repo string, appIf application.ApplicationServiceClient) (app *argoappv1.Application, err error) { // argocd.argoproj.io/manifest-generate-paths appQuery := application.ApplicationQuery{ Repo: &repo, } - // AFAIKT I can't use standard grpc instrumentation here, since the argocd client abstracts to much (including the choice between Grpc and Grpc-web) + // AFAIKT I can't use standard grpc instrumentation here, since the argocd client abstracts too much (including the choice between Grpc and Grpc-web) // I'll just manually log the time it takes to get the apps for now getAppsStart := time.Now() allRepoApps, err := appIf.List(ctx, &appQuery) From fc1df9367f39702cc232bbb9638625fe38595b66 Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Wed, 12 Jun 2024 13:16:06 +0200 Subject: [PATCH 14/15] I fix var name typo --- internal/pkg/argocd/argocd.go | 8 ++++---- internal/pkg/configuration/config.go | 2 +- internal/pkg/githubapi/github.go | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/internal/pkg/argocd/argocd.go b/internal/pkg/argocd/argocd.go index 00e77273..559c59d5 100644 --- a/internal/pkg/argocd/argocd.go +++ b/internal/pkg/argocd/argocd.go @@ -226,7 +226,7 @@ func findArgocdAppByManifestPathAnnotation(ctx context.Context, componentPath st return nil, fmt.Errorf("No ArgoCD application found with manifest-generate-paths annotation that matches %s(looked at repo %s, checked %v apps) ", componentPath, repo, len(allRepoApps.Items)) } -func generateDiffOfAComponent(ctx context.Context, componentPath string, prBranch string, repo string, appIf application.ApplicationServiceClient, projIf projectpkg.ProjectServiceClient, argoSettings *settings.Settings, usaSHALabelForArgoDicovery bool) (componentDiffResult DiffResult) { +func generateDiffOfAComponent(ctx context.Context, componentPath string, prBranch string, repo string, appIf application.ApplicationServiceClient, projIf projectpkg.ProjectServiceClient, argoSettings *settings.Settings, useSHALabelForArgoDicovery bool) (componentDiffResult DiffResult) { componentDiffResult.ComponentPath = componentPath // Find ArgoCD application by the path SHA1 label selector and repo name @@ -234,7 +234,7 @@ func generateDiffOfAComponent(ctx context.Context, componentPath string, prBranc var foundApp *argoappv1.Application var err error - if usaSHALabelForArgoDicovery { + if useSHALabelForArgoDicovery { foundApp, err = findArgocdAppBySHA1Label(ctx, componentPath, repo, appIf) } else { foundApp, err = findArgocdAppByManifestPathAnnotation(ctx, componentPath, repo, appIf) @@ -304,7 +304,7 @@ func generateDiffOfAComponent(ctx context.Context, componentPath string, prBranc } // GenerateDiffOfChangedComponents generates diff of changed components -func GenerateDiffOfChangedComponents(ctx context.Context, componentPathList []string, prBranch string, repo string, usaSHALabelForArgoDicovery bool) (hasComponentDiff bool, hasComponentDiffErrors bool, diffResults []DiffResult, err error) { +func GenerateDiffOfChangedComponents(ctx context.Context, componentPathList []string, prBranch string, repo string, useSHALabelForArgoDicovery bool) (hasComponentDiff bool, hasComponentDiffErrors bool, diffResults []DiffResult, err error) { hasComponentDiff = false hasComponentDiffErrors = false // env var should be centralized @@ -342,7 +342,7 @@ func GenerateDiffOfChangedComponents(ctx context.Context, componentPathList []st log.Debugf("Checking ArgoCD diff for components: %v", componentPathList) for _, componentPath := range componentPathList { - currentDiffResult := generateDiffOfAComponent(ctx, componentPath, prBranch, repo, appIf, projIf, argoSettings, usaSHALabelForArgoDicovery) + currentDiffResult := generateDiffOfAComponent(ctx, componentPath, prBranch, repo, appIf, projIf, argoSettings, useSHALabelForArgoDicovery) if currentDiffResult.DiffError != nil { log.Errorf("Error generating diff for component %s: %v", componentPath, currentDiffResult.DiffError) hasComponentDiffErrors = true diff --git a/internal/pkg/configuration/config.go b/internal/pkg/configuration/config.go index a9b675e7..379d9c8c 100644 --- a/internal/pkg/configuration/config.go +++ b/internal/pkg/configuration/config.go @@ -42,7 +42,7 @@ type Config struct { WebhookEndpointRegexs []WebhookEndpointRegex `yaml:"webhookEndpointRegexs"` CommentArgocdDiffonPR bool `yaml:"commentArgocdDiffonPR"` AutoMergeNoDiffPRs bool `yaml:"autoMergeNoDiffPRs"` - UsaSHALabelForArgoDicovery bool `yaml:"usaSHALabelForArgoDicovery"` + UseSHALabelForArgoDicovery bool `yaml:"useSHALabelForArgoDicovery"` } func ParseConfigFromYaml(y string) (*Config, error) { diff --git a/internal/pkg/githubapi/github.go b/internal/pkg/githubapi/github.go index 293ec6ad..a7e3cce7 100644 --- a/internal/pkg/githubapi/github.go +++ b/internal/pkg/githubapi/github.go @@ -123,7 +123,7 @@ func HandlePREvent(eventPayload *github.PullRequestEvent, ghPrClientDetails GhPr } } - hasComponentDiff, hasComponentDiffErrors, diffOfChangedComponents, err := argocd.GenerateDiffOfChangedComponents(ctx, componentPathList, ghPrClientDetails.Ref, ghPrClientDetails.RepoURL, config.UsaSHALabelForArgoDicovery) + hasComponentDiff, hasComponentDiffErrors, diffOfChangedComponents, err := argocd.GenerateDiffOfChangedComponents(ctx, componentPathList, ghPrClientDetails.Ref, ghPrClientDetails.RepoURL, config.UseSHALabelForArgoDicovery) if err != nil { prHandleError = err ghPrClientDetails.PrLogger.Errorf("Failed to get ArgoCD diff information: err=%s\n", err) From 6861102055022e8362f7ae7f6647dce0b75a5ded Mon Sep 17 00:00:00 2001 From: Oded Ben Ozer Date: Wed, 12 Jun 2024 14:06:07 +0200 Subject: [PATCH 15/15] Rename client var names --- internal/pkg/argocd/argocd.go | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/internal/pkg/argocd/argocd.go b/internal/pkg/argocd/argocd.go index 559c59d5..14ebc50b 100644 --- a/internal/pkg/argocd/argocd.go +++ b/internal/pkg/argocd/argocd.go @@ -158,7 +158,7 @@ func createArgoCdClient() (apiclient.Client, error) { // findArgocdAppBySHA1Label finds an ArgoCD application by the SHA1 label of the component path it's supposed to avoid performance issues with the "manifest-generate-paths" annotation method which requires pulling all ArgoCD applications(!) on every PR event. // The SHA1 label is assumed to be populated by the ApplicationSet controller(or apps of apps or similar). -func findArgocdAppBySHA1Label(ctx context.Context, componentPath string, repo string, appIf application.ApplicationServiceClient) (app *argoappv1.Application, err error) { +func findArgocdAppBySHA1Label(ctx context.Context, componentPath string, repo string, appClient application.ApplicationServiceClient) (app *argoappv1.Application, err error) { // Calculate sha1 of component path to use in a label selector cPathBa := []byte(componentPath) hasher := sha1.New() //nolint:gosec // G505: Blocklisted import crypto/sha1: weak cryptographic primitive (gosec), this is not a cryptographic use case @@ -170,10 +170,9 @@ func findArgocdAppBySHA1Label(ctx context.Context, componentPath string, repo st Selector: &labelSelector, Repo: &repo, } - foundApps, err := appIf.List(ctx, &appLabelQuery) + foundApps, err := appClient.List(ctx, &appLabelQuery) if err != nil { return nil, fmt.Errorf("Error listing ArgoCD applications: %v", err) - } if len(foundApps.Items) == 0 { return nil, fmt.Errorf("No ArgoCD application found for component path sha1 %s(repo %s), used this label selector: %s", componentPathSha1, repo, labelSelector) @@ -186,7 +185,7 @@ func findArgocdAppBySHA1Label(ctx context.Context, componentPath string, repo st // findArgocdAppByManifestPathAnnotation is the default method to find an ArgoCD application by the manifest-generate-paths annotation. // It assumes the ArgoCD (optional) manifest-generate-paths annotation is set on all relevant apps. // Notice that this method includes a full list of all ArgoCD applications in the repo, this could be a performance issue if there are many apps in the repo. -func findArgocdAppByManifestPathAnnotation(ctx context.Context, componentPath string, repo string, appIf application.ApplicationServiceClient) (app *argoappv1.Application, err error) { +func findArgocdAppByManifestPathAnnotation(ctx context.Context, componentPath string, repo string, appClient application.ApplicationServiceClient) (app *argoappv1.Application, err error) { // argocd.argoproj.io/manifest-generate-paths appQuery := application.ApplicationQuery{ Repo: &repo, @@ -194,7 +193,7 @@ func findArgocdAppByManifestPathAnnotation(ctx context.Context, componentPath st // AFAIKT I can't use standard grpc instrumentation here, since the argocd client abstracts too much (including the choice between Grpc and Grpc-web) // I'll just manually log the time it takes to get the apps for now getAppsStart := time.Now() - allRepoApps, err := appIf.List(ctx, &appQuery) + allRepoApps, err := appClient.List(ctx, &appQuery) getAppsDuration := time.Since(getAppsStart).Milliseconds() log.Infof("Got %v ArgoCD applications for repo %s in %v ms", len(allRepoApps.Items), repo, getAppsDuration) if err != nil { @@ -226,7 +225,7 @@ func findArgocdAppByManifestPathAnnotation(ctx context.Context, componentPath st return nil, fmt.Errorf("No ArgoCD application found with manifest-generate-paths annotation that matches %s(looked at repo %s, checked %v apps) ", componentPath, repo, len(allRepoApps.Items)) } -func generateDiffOfAComponent(ctx context.Context, componentPath string, prBranch string, repo string, appIf application.ApplicationServiceClient, projIf projectpkg.ProjectServiceClient, argoSettings *settings.Settings, useSHALabelForArgoDicovery bool) (componentDiffResult DiffResult) { +func generateDiffOfAComponent(ctx context.Context, componentPath string, prBranch string, repo string, appClient application.ApplicationServiceClient, projClient projectpkg.ProjectServiceClient, argoSettings *settings.Settings, useSHALabelForArgoDicovery bool) (componentDiffResult DiffResult) { componentDiffResult.ComponentPath = componentPath // Find ArgoCD application by the path SHA1 label selector and repo name @@ -235,9 +234,9 @@ func generateDiffOfAComponent(ctx context.Context, componentPath string, prBranc var foundApp *argoappv1.Application var err error if useSHALabelForArgoDicovery { - foundApp, err = findArgocdAppBySHA1Label(ctx, componentPath, repo, appIf) + foundApp, err = findArgocdAppBySHA1Label(ctx, componentPath, repo, appClient) } else { - foundApp, err = findArgocdAppByManifestPathAnnotation(ctx, componentPath, repo, appIf) + foundApp, err = findArgocdAppByManifestPathAnnotation(ctx, componentPath, repo, appClient) } if err != nil { componentDiffResult.DiffError = err @@ -252,7 +251,7 @@ func generateDiffOfAComponent(ctx context.Context, componentPath string, prBranc Name: &foundApp.Name, // we expect only one app with this label and repo selectors Refresh: &refreshType, } - app, err := appIf.Get(ctx, &appNameQuery) + app, err := appClient.Get(ctx, &appNameQuery) if err != nil { componentDiffResult.DiffError = err log.Errorf("Error getting app %s: %v", foundApp.Name, err) @@ -261,7 +260,7 @@ func generateDiffOfAComponent(ctx context.Context, componentPath string, prBranc log.Debugf("Got ArgoCD app %s", app.Name) componentDiffResult.ArgoCdAppName = app.Name componentDiffResult.ArgoCdAppURL = fmt.Sprintf("%s/applications/%s", argoSettings.URL, app.Name) - resources, err := appIf.ManagedResources(ctx, &application.ResourcesQuery{ApplicationName: &app.Name, AppNamespace: &app.Namespace}) + resources, err := appClient.ManagedResources(ctx, &application.ResourcesQuery{ApplicationName: &app.Name, AppNamespace: &app.Namespace}) if err != nil { componentDiffResult.DiffError = err log.Errorf("Error getting (live)resources for app %s: %v", app.Name, err) @@ -277,7 +276,7 @@ func generateDiffOfAComponent(ctx context.Context, componentPath string, prBranc Revision: &prBranch, AppNamespace: &app.Namespace, } - manifests, err := appIf.GetManifests(ctx, &manifestQuery) + manifests, err := appClient.GetManifests(ctx, &manifestQuery) if err != nil { componentDiffResult.DiffError = err log.Errorf("Error getting manifests for app %s, revision %s: %v", app.Name, prBranch, err) @@ -288,7 +287,7 @@ func generateDiffOfAComponent(ctx context.Context, componentPath string, prBranc diffOption.revision = prBranch // Now we diff the live state(resources) and target state of the application objects(diffOption.res) - detailedProject, err := projIf.GetDetailedProject(ctx, &projectpkg.ProjectQuery{Name: app.Spec.Project}) + detailedProject, err := projClient.GetDetailedProject(ctx, &projectpkg.ProjectQuery{Name: app.Spec.Project}) if err != nil { componentDiffResult.DiffError = err log.Errorf("Error getting project %s: %v", app.Spec.Project, err) @@ -314,27 +313,27 @@ func GenerateDiffOfChangedComponents(ctx context.Context, componentPathList []st return false, true, nil, err } - conn, appIf, err := client.NewApplicationClient() + conn, appClient, err := client.NewApplicationClient() if err != nil { log.Errorf("Error creating ArgoCD app client: %v", err) return false, true, nil, err } defer argoio.Close(conn) - conn, projIf, err := client.NewProjectClient() + conn, projClient, err := client.NewProjectClient() if err != nil { log.Errorf("Error creating ArgoCD project client: %v", err) return false, true, nil, err } defer argoio.Close(conn) - conn, settingsIf, err := client.NewSettingsClient() + conn, settingClient, err := client.NewSettingsClient() if err != nil { log.Errorf("Error creating ArgoCD settings client: %v", err) return false, true, nil, err } defer argoio.Close(conn) - argoSettings, err := settingsIf.Get(ctx, &settings.SettingsQuery{}) + argoSettings, err := settingClient.Get(ctx, &settings.SettingsQuery{}) if err != nil { log.Errorf("Error getting ArgoCD settings: %v", err) return false, true, nil, err @@ -342,7 +341,7 @@ func GenerateDiffOfChangedComponents(ctx context.Context, componentPathList []st log.Debugf("Checking ArgoCD diff for components: %v", componentPathList) for _, componentPath := range componentPathList { - currentDiffResult := generateDiffOfAComponent(ctx, componentPath, prBranch, repo, appIf, projIf, argoSettings, useSHALabelForArgoDicovery) + currentDiffResult := generateDiffOfAComponent(ctx, componentPath, prBranch, repo, appClient, projClient, argoSettings, useSHALabelForArgoDicovery) if currentDiffResult.DiffError != nil { log.Errorf("Error generating diff for component %s: %v", componentPath, currentDiffResult.DiffError) hasComponentDiffErrors = true