Skip to content

Commit

Permalink
fix(appset): Don't use revision cache when reconciling after webhook(#…
Browse files Browse the repository at this point in the history
…16062)

Signed-off-by: dhruvang1 <dhruvang1@users.noreply.github.com>
  • Loading branch information
dhruvang1 committed Nov 7, 2023
1 parent 5c74d73 commit 95ec130
Show file tree
Hide file tree
Showing 12 changed files with 278 additions and 202 deletions.
1 change: 0 additions & 1 deletion applicationset/controllers/applicationset_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,6 @@ func (r *ApplicationSetReconciler) Reconcile(ctx context.Context, req ctrl.Reque

if applicationSetInfo.RefreshRequired() {
delete(applicationSetInfo.Annotations, common.AnnotationApplicationSetRefresh)
delete(applicationSetInfo.Annotations, common.AnnotationApplicationSetRefreshSHA)
err := r.Client.Update(ctx, &applicationSetInfo)
if err != nil {
logCtx.Warnf("error occurred while updating ApplicationSet: %v", err)
Expand Down
20 changes: 7 additions & 13 deletions applicationset/generators/git.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import (

"github.com/argoproj/argo-cd/v2/applicationset/services"
"github.com/argoproj/argo-cd/v2/applicationset/utils"
"github.com/argoproj/argo-cd/v2/common"
argoprojiov1alpha1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
)

Expand Down Expand Up @@ -57,19 +56,14 @@ func (g *GitGenerator) GenerateParams(appSetGenerator *argoprojiov1alpha1.Applic
return nil, EmptyAppSetGeneratorError
}

revision := appSetGenerator.Git.Revision
if appSet.RefreshRequired() {
if sha, ok := appSet.Annotations[common.AnnotationApplicationSetRefreshSHA]; ok {
revision = sha
}
}
noRevisionCache := appSet.RefreshRequired()

var err error
var res []map[string]interface{}
if len(appSetGenerator.Git.Directories) != 0 {
res, err = g.generateParamsForGitDirectories(appSetGenerator, revision, appSet.Spec.GoTemplate, appSet.Spec.GoTemplateOptions)
res, err = g.generateParamsForGitDirectories(appSetGenerator, noRevisionCache, appSet.Spec.GoTemplate, appSet.Spec.GoTemplateOptions)
} else if len(appSetGenerator.Git.Files) != 0 {
res, err = g.generateParamsForGitFiles(appSetGenerator, revision, appSet.Spec.GoTemplate, appSet.Spec.GoTemplateOptions)
res, err = g.generateParamsForGitFiles(appSetGenerator, noRevisionCache, appSet.Spec.GoTemplate, appSet.Spec.GoTemplateOptions)
} else {
return nil, EmptyAppSetGeneratorError
}
Expand All @@ -80,10 +74,10 @@ func (g *GitGenerator) GenerateParams(appSetGenerator *argoprojiov1alpha1.Applic
return res, nil
}

func (g *GitGenerator) generateParamsForGitDirectories(appSetGenerator *argoprojiov1alpha1.ApplicationSetGenerator, revision string, useGoTemplate bool, goTemplateOptions []string) ([]map[string]interface{}, error) {
func (g *GitGenerator) generateParamsForGitDirectories(appSetGenerator *argoprojiov1alpha1.ApplicationSetGenerator, noRevisionCache bool, useGoTemplate bool, goTemplateOptions []string) ([]map[string]interface{}, error) {

// Directories, not files
allPaths, err := g.repos.GetDirectories(context.TODO(), appSetGenerator.Git.RepoURL, revision)
allPaths, err := g.repos.GetDirectories(context.TODO(), appSetGenerator.Git.RepoURL, appSetGenerator.Git.Revision, noRevisionCache)
if err != nil {
return nil, fmt.Errorf("error getting directories from repo: %w", err)
}
Expand All @@ -106,12 +100,12 @@ func (g *GitGenerator) generateParamsForGitDirectories(appSetGenerator *argoproj
return res, nil
}

func (g *GitGenerator) generateParamsForGitFiles(appSetGenerator *argoprojiov1alpha1.ApplicationSetGenerator, revision string, useGoTemplate bool, goTemplateOptions []string) ([]map[string]interface{}, error) {
func (g *GitGenerator) generateParamsForGitFiles(appSetGenerator *argoprojiov1alpha1.ApplicationSetGenerator, noRevisionCache bool, useGoTemplate bool, goTemplateOptions []string) ([]map[string]interface{}, error) {

// Get all files that match the requested path string, removing duplicates
allFiles := make(map[string][]byte)
for _, requestedPath := range appSetGenerator.Git.Files {
files, err := g.repos.GetFiles(context.TODO(), appSetGenerator.Git.RepoURL, revision, requestedPath.Path)
files, err := g.repos.GetFiles(context.TODO(), appSetGenerator.Git.RepoURL, appSetGenerator.Git.Revision, requestedPath.Path, noRevisionCache)
if err != nil {
return nil, err
}
Expand Down
8 changes: 4 additions & 4 deletions applicationset/generators/git_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,7 @@ func TestGitGenerateParamsFromDirectories(t *testing.T) {

argoCDServiceMock := mocks.Repos{}

argoCDServiceMock.On("GetDirectories", mock.Anything, mock.Anything, mock.Anything).Return(testCaseCopy.repoApps, testCaseCopy.repoError)
argoCDServiceMock.On("GetDirectories", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(testCaseCopy.repoApps, testCaseCopy.repoError)

var gitGenerator = NewGitGenerator(&argoCDServiceMock)
applicationSetInfo := argoprojiov1alpha1.ApplicationSet{
Expand Down Expand Up @@ -613,7 +613,7 @@ func TestGitGenerateParamsFromDirectoriesGoTemplate(t *testing.T) {

argoCDServiceMock := mocks.Repos{}

argoCDServiceMock.On("GetDirectories", mock.Anything, mock.Anything, mock.Anything).Return(testCaseCopy.repoApps, testCaseCopy.repoError)
argoCDServiceMock.On("GetDirectories", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(testCaseCopy.repoApps, testCaseCopy.repoError)

var gitGenerator = NewGitGenerator(&argoCDServiceMock)
applicationSetInfo := argoprojiov1alpha1.ApplicationSet{
Expand Down Expand Up @@ -972,7 +972,7 @@ cluster:
t.Parallel()

argoCDServiceMock := mocks.Repos{}
argoCDServiceMock.On("GetFiles", mock.Anything, mock.Anything, mock.Anything, mock.Anything).
argoCDServiceMock.On("GetFiles", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).
Return(testCaseCopy.repoFileContents, testCaseCopy.repoPathsError)

var gitGenerator = NewGitGenerator(&argoCDServiceMock)
Expand Down Expand Up @@ -1322,7 +1322,7 @@ cluster:
t.Parallel()

argoCDServiceMock := mocks.Repos{}
argoCDServiceMock.On("GetFiles", mock.Anything, mock.Anything, mock.Anything, mock.Anything).
argoCDServiceMock.On("GetFiles", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).
Return(testCaseCopy.repoFileContents, testCaseCopy.repoPathsError)

var gitGenerator = NewGitGenerator(&argoCDServiceMock)
Expand Down
2 changes: 1 addition & 1 deletion applicationset/generators/matrix_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1108,7 +1108,7 @@ func TestGitGenerator_GenerateParams_list_x_git_matrix_generator(t *testing.T) {
}

repoServiceMock := &mocks.Repos{}
repoServiceMock.On("GetFiles", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(map[string][]byte{
repoServiceMock.On("GetFiles", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(map[string][]byte{
"some/path.json": []byte("test: content"),
}, nil)
gitGenerator := NewGitGenerator(repoServiceMock)
Expand Down
36 changes: 18 additions & 18 deletions applicationset/services/mocks/Repos.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 10 additions & 4 deletions applicationset/services/repo_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import (
"github.com/argoproj/argo-cd/v2/util/io"
)

//go:generate go run github.com/vektra/mockery/v2@v2.25.1 --name=RepositoryDB

// RepositoryDB Is a lean facade for ArgoDB,
// Using a lean interface makes it easier to test the functionality of the git generator
type RepositoryDB interface {
Expand All @@ -25,13 +27,15 @@ type argoCDService struct {
newFileGlobbingEnabled bool
}

//go:generate go run github.com/vektra/mockery/v2@v2.25.1 --name=Repos

type Repos interface {

// GetFiles returns content of files (not directories) within the target repo
GetFiles(ctx context.Context, repoURL string, revision string, pattern string) (map[string][]byte, error)
GetFiles(ctx context.Context, repoURL string, revision string, pattern string, noRevisionCache bool) (map[string][]byte, error)

// GetDirectories returns a list of directories (not files) within the target repo
GetDirectories(ctx context.Context, repoURL string, revision string) ([]string, error)
GetDirectories(ctx context.Context, repoURL string, revision string, noRevisionCache bool) ([]string, error)
}

func NewArgoCDService(db db.ArgoDB, submoduleEnabled bool, repoClientset apiclient.Clientset, newFileGlobbingEnabled bool) (Repos, error) {
Expand All @@ -43,7 +47,7 @@ func NewArgoCDService(db db.ArgoDB, submoduleEnabled bool, repoClientset apiclie
}, nil
}

func (a *argoCDService) GetFiles(ctx context.Context, repoURL string, revision string, pattern string) (map[string][]byte, error) {
func (a *argoCDService) GetFiles(ctx context.Context, repoURL string, revision string, pattern string, noRevisionCache bool) (map[string][]byte, error) {
repo, err := a.repositoriesDB.GetRepository(ctx, repoURL)
if err != nil {
return nil, fmt.Errorf("error in GetRepository: %w", err)
Expand All @@ -55,6 +59,7 @@ func (a *argoCDService) GetFiles(ctx context.Context, repoURL string, revision s
Revision: revision,
Path: pattern,
NewGitFileGlobbingEnabled: a.newFileGlobbingEnabled,
NoRevisionCache: noRevisionCache,
}
closer, client, err := a.repoServerClientSet.NewRepoServerClient()
if err != nil {
Expand All @@ -69,7 +74,7 @@ func (a *argoCDService) GetFiles(ctx context.Context, repoURL string, revision s
return fileResponse.GetMap(), nil
}

func (a *argoCDService) GetDirectories(ctx context.Context, repoURL string, revision string) ([]string, error) {
func (a *argoCDService) GetDirectories(ctx context.Context, repoURL string, revision string, noRevisionCache bool) ([]string, error) {
repo, err := a.repositoriesDB.GetRepository(ctx, repoURL)
if err != nil {
return nil, fmt.Errorf("error in GetRepository: %w", err)
Expand All @@ -79,6 +84,7 @@ func (a *argoCDService) GetDirectories(ctx context.Context, repoURL string, revi
Repo: repo,
SubmoduleEnabled: a.submoduleEnabled,
Revision: revision,
NoRevisionCache: noRevisionCache,
}

closer, client, err := a.repoServerClientSet.NewRepoServerClient()
Expand Down
28 changes: 15 additions & 13 deletions applicationset/services/repo_service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,10 @@ func TestGetDirectories(t *testing.T) {
repoServerClientFuncs []func(*repo_mocks.RepoServerServiceClient)
}
type args struct {
ctx context.Context
repoURL string
revision string
ctx context.Context
repoURL string
revision string
noRevisionCache bool
}
tests := []struct {
name string
Expand Down Expand Up @@ -88,11 +89,11 @@ func TestGetDirectories(t *testing.T) {
submoduleEnabled: tt.fields.submoduleEnabled,
repoServerClientSet: &repo_mocks.Clientset{RepoServerServiceClient: mockRepoClient},
}
got, err := a.GetDirectories(tt.args.ctx, tt.args.repoURL, tt.args.revision)
if !tt.wantErr(t, err, fmt.Sprintf("GetDirectories(%v, %v, %v)", tt.args.ctx, tt.args.repoURL, tt.args.revision)) {
got, err := a.GetDirectories(tt.args.ctx, tt.args.repoURL, tt.args.revision, tt.args.noRevisionCache)
if !tt.wantErr(t, err, fmt.Sprintf("GetDirectories(%v, %v, %v, %v)", tt.args.ctx, tt.args.repoURL, tt.args.revision, tt.args.noRevisionCache)) {
return
}
assert.Equalf(t, tt.want, got, "GetDirectories(%v, %v, %v)", tt.args.ctx, tt.args.repoURL, tt.args.revision)
assert.Equalf(t, tt.want, got, "GetDirectories(%v, %v, %v, %v)", tt.args.ctx, tt.args.repoURL, tt.args.revision, tt.args.noRevisionCache)
})
}
}
Expand All @@ -105,10 +106,11 @@ func TestGetFiles(t *testing.T) {
repoServerClientFuncs []func(*repo_mocks.RepoServerServiceClient)
}
type args struct {
ctx context.Context
repoURL string
revision string
pattern string
ctx context.Context
repoURL string
revision string
pattern string
noRevisionCache bool
}
tests := []struct {
name string
Expand Down Expand Up @@ -175,11 +177,11 @@ func TestGetFiles(t *testing.T) {
submoduleEnabled: tt.fields.submoduleEnabled,
repoServerClientSet: &repo_mocks.Clientset{RepoServerServiceClient: mockRepoClient},
}
got, err := a.GetFiles(tt.args.ctx, tt.args.repoURL, tt.args.revision, tt.args.pattern)
if !tt.wantErr(t, err, fmt.Sprintf("GetFiles(%v, %v, %v, %v)", tt.args.ctx, tt.args.repoURL, tt.args.revision, tt.args.pattern)) {
got, err := a.GetFiles(tt.args.ctx, tt.args.repoURL, tt.args.revision, tt.args.pattern, tt.args.noRevisionCache)
if !tt.wantErr(t, err, fmt.Sprintf("GetFiles(%v, %v, %v, %v, %v)", tt.args.ctx, tt.args.repoURL, tt.args.revision, tt.args.pattern, tt.args.noRevisionCache)) {
return
}
assert.Equalf(t, tt.want, got, "GetFiles(%v, %v, %v, %v)", tt.args.ctx, tt.args.repoURL, tt.args.revision, tt.args.pattern)
assert.Equalf(t, tt.want, got, "GetFiles(%v, %v, %v, %v, %v)", tt.args.ctx, tt.args.repoURL, tt.args.revision, tt.args.pattern, tt.args.noRevisionCache)
})
}
}
Expand Down
12 changes: 2 additions & 10 deletions applicationset/webhook/webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ type WebhookHandler struct {
}

type gitGeneratorInfo struct {
CommitSHA string
Revision string
TouchedHead bool
RepoRegexp *regexp.Regexp
Expand Down Expand Up @@ -136,7 +135,7 @@ func (h *WebhookHandler) HandleEvent(payload interface{}) {
}
}
if shouldRefresh {
err := refreshApplicationSet(h.client, &appSet, gitGenInfo.CommitSHA)
err := refreshApplicationSet(h.client, &appSet)
if err != nil {
log.Errorf("Failed to refresh ApplicationSet '%s' for controller reprocessing", appSet.Name)
continue
Expand Down Expand Up @@ -189,25 +188,21 @@ func parseRevision(ref string) string {

func getGitGeneratorInfo(payload interface{}) *gitGeneratorInfo {
var (
commitSHA string
webURL string
revision string
touchedHead bool
)
switch payload := payload.(type) {
case github.PushPayload:
commitSHA = payload.After
webURL = payload.Repository.HTMLURL
revision = parseRevision(payload.Ref)
touchedHead = payload.Repository.DefaultBranch == revision
case gitlab.PushEventPayload:
commitSHA = payload.After
webURL = payload.Project.WebURL
revision = parseRevision(payload.Ref)
touchedHead = payload.Project.DefaultBranch == revision
case azuredevops.GitPushEvent:
// See: https://learn.microsoft.com/en-us/azure/devops/service-hooks/events?view=azure-devops#git.push
commitSHA = payload.Resource.RefUpdates[0].NewObjectID
webURL = payload.Resource.Repository.RemoteURL
revision = parseRevision(payload.Resource.RefUpdates[0].Name)
touchedHead = payload.Resource.RefUpdates[0].Name == payload.Resource.Repository.DefaultBranch
Expand All @@ -233,7 +228,6 @@ func getGitGeneratorInfo(payload interface{}) *gitGeneratorInfo {
RepoRegexp: repoRegexp,
TouchedHead: touchedHead,
Revision: revision,
CommitSHA: commitSHA,
}
}

Expand Down Expand Up @@ -644,7 +638,7 @@ func (h *WebhookHandler) shouldRefreshMergeGenerator(gen *v1alpha1.MergeGenerato
return false
}

func refreshApplicationSet(c client.Client, appSet *v1alpha1.ApplicationSet, commitSHA string) error {
func refreshApplicationSet(c client.Client, appSet *v1alpha1.ApplicationSet) error {
// patch the ApplicationSet with the refresh annotation to reconcile
return retry.RetryOnConflict(retry.DefaultBackoff, func() error {
err := c.Get(context.Background(), types.NamespacedName{Name: appSet.Name, Namespace: appSet.Namespace}, appSet)
Expand All @@ -655,8 +649,6 @@ func refreshApplicationSet(c client.Client, appSet *v1alpha1.ApplicationSet, com
appSet.Annotations = map[string]string{}
}
appSet.Annotations[common.AnnotationApplicationSetRefresh] = "true"
appSet.Annotations[common.AnnotationApplicationSetRefreshSHA] = commitSHA

return c.Patch(context.Background(), appSet, client.Merge)
})
}
2 changes: 0 additions & 2 deletions common/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -349,8 +349,6 @@ func GetCMPWorkDir() string {
const (
// AnnotationApplicationSetRefresh is an annotation that is added when an ApplicationSet is requested to be refreshed by a webhook. The ApplicationSet controller will remove this annotation at the end of reconciliation.
AnnotationApplicationSetRefresh = "argocd.argoproj.io/application-set-refresh"
// AnnotationApplicationSetRefreshSHA is an annotation used to note the latest commit-sha from webhook. It follows the same lifecycle as AnnotationApplicationSetRefresh.
AnnotationApplicationSetRefreshSHA = "argocd.argoproj.io/application-set-refresh-sha"
)

// gRPC settings
Expand Down
Loading

0 comments on commit 95ec130

Please sign in to comment.