Skip to content

Commit

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

* fix(appset): store sha from webhook to get latest change during reconcile (argoproj#16062)

Signed-off-by: dhruvang1 <dhruvang1@users.noreply.github.com>

* fix(appset): Don't use revision cache when reconciling after webhook(argoproj#16062)

Signed-off-by: dhruvang1 <dhruvang1@users.noreply.github.com>

---------

Signed-off-by: dhruvang1 <dhruvang1@users.noreply.github.com>
dhruvang1 authored and vladfr committed Dec 13, 2023

Verified

This commit was signed with the committer’s verified signature.
lonnieezell Lonnie Ezell
1 parent f4d4b69 commit b2f1f8c
Showing 9 changed files with 277 additions and 182 deletions.
14 changes: 8 additions & 6 deletions applicationset/generators/git.go
Original file line number Diff line number Diff line change
@@ -56,12 +56,14 @@ func (g *GitGenerator) GenerateParams(appSetGenerator *argoprojiov1alpha1.Applic
return nil, EmptyAppSetGeneratorError
}

noRevisionCache := appSet.RefreshRequired()

var err error
var res []map[string]interface{}
if len(appSetGenerator.Git.Directories) != 0 {
res, err = g.generateParamsForGitDirectories(appSetGenerator, 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, appSet.Spec.GoTemplate, appSet.Spec.GoTemplateOptions)
res, err = g.generateParamsForGitFiles(appSetGenerator, noRevisionCache, appSet.Spec.GoTemplate, appSet.Spec.GoTemplateOptions)
} else {
return nil, EmptyAppSetGeneratorError
}
@@ -72,10 +74,10 @@ func (g *GitGenerator) GenerateParams(appSetGenerator *argoprojiov1alpha1.Applic
return res, nil
}

func (g *GitGenerator) generateParamsForGitDirectories(appSetGenerator *argoprojiov1alpha1.ApplicationSetGenerator, 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, appSetGenerator.Git.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)
}
@@ -98,12 +100,12 @@ func (g *GitGenerator) generateParamsForGitDirectories(appSetGenerator *argoproj
return res, nil
}

func (g *GitGenerator) generateParamsForGitFiles(appSetGenerator *argoprojiov1alpha1.ApplicationSetGenerator, 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, appSetGenerator.Git.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
}
8 changes: 4 additions & 4 deletions applicationset/generators/git_test.go
Original file line number Diff line number Diff line change
@@ -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{
@@ -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{
@@ -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)
@@ -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)
2 changes: 1 addition & 1 deletion applicationset/generators/matrix_test.go
Original file line number Diff line number Diff line change
@@ -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)
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
@@ -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 {
@@ -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) {
@@ -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)
@@ -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 {
@@ -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)
@@ -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()
28 changes: 15 additions & 13 deletions applicationset/services/repo_service_test.go
Original file line number Diff line number Diff line change
@@ -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
@@ -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)
})
}
}
@@ -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
@@ -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)
})
}
}
348 changes: 215 additions & 133 deletions reposerver/apiclient/repository.pb.go

Large diffs are not rendered by default.

7 changes: 4 additions & 3 deletions reposerver/repository/repository.go
Original file line number Diff line number Diff line change
@@ -2506,6 +2506,7 @@ func (s *Service) GetGitFiles(_ context.Context, request *apiclient.GitFilesRequ
repo := request.GetRepo()
revision := request.GetRevision()
gitPath := request.GetPath()
noRevisionCache := request.GetNoRevisionCache()
enableNewGitFileGlobbing := request.GetNewGitFileGlobbingEnabled()
if gitPath == "" {
gitPath = "."
@@ -2515,7 +2516,7 @@ func (s *Service) GetGitFiles(_ context.Context, request *apiclient.GitFilesRequ
return nil, status.Error(codes.InvalidArgument, "must pass a valid repo")
}

gitClient, revision, err := s.newClientResolveRevision(repo, revision, git.WithCache(s.cache, true))
gitClient, revision, err := s.newClientResolveRevision(repo, revision, git.WithCache(s.cache, !noRevisionCache))
if err != nil {
return nil, status.Errorf(codes.Internal, "unable to resolve git revision %s: %v", revision, err)
}
@@ -2568,12 +2569,12 @@ func (s *Service) GetGitFiles(_ context.Context, request *apiclient.GitFilesRequ
func (s *Service) GetGitDirectories(_ context.Context, request *apiclient.GitDirectoriesRequest) (*apiclient.GitDirectoriesResponse, error) {
repo := request.GetRepo()
revision := request.GetRevision()

noRevisionCache := request.GetNoRevisionCache()
if repo == nil {
return nil, status.Error(codes.InvalidArgument, "must pass a valid repo")
}

gitClient, revision, err := s.newClientResolveRevision(repo, revision, git.WithCache(s.cache, true))
gitClient, revision, err := s.newClientResolveRevision(repo, revision, git.WithCache(s.cache, !noRevisionCache))
if err != nil {
return nil, status.Errorf(codes.Internal, "unable to resolve git revision %s: %v", revision, err)
}
2 changes: 2 additions & 0 deletions reposerver/repository/repository.proto
Original file line number Diff line number Diff line change
@@ -236,6 +236,7 @@ message GitFilesRequest {
string revision = 3;
string path = 4;
bool NewGitFileGlobbingEnabled = 5;
bool noRevisionCache = 6;
}

message GitFilesResponse {
@@ -247,6 +248,7 @@ message GitDirectoriesRequest {
github.com.argoproj.argo_cd.v2.pkg.apis.application.v1alpha1.Repository repo = 1;
bool submoduleEnabled = 2;
string revision = 3;
bool noRevisionCache = 4;
}

message GitDirectoriesResponse {

0 comments on commit b2f1f8c

Please sign in to comment.