From b3bf7aa154ab3be430cf0234c68800be183325d2 Mon Sep 17 00:00:00 2001 From: kim-codefresh Date: Tue, 10 Jan 2023 16:58:19 +0200 Subject: [PATCH 1/4] from repo CR-15777 Signed-off-by: kim-codefresh --- pkg/git/repository.go | 50 +++++++++++++++++++++++ pkg/git/repository_test.go | 82 ++++++++++++++++++++++++++++++++++---- 2 files changed, 125 insertions(+), 7 deletions(-) diff --git a/pkg/git/repository.go b/pkg/git/repository.go index 82167966..79a64e6a 100644 --- a/pkg/git/repository.go +++ b/pkg/git/repository.go @@ -244,6 +244,13 @@ func (o *CloneOptions) GetRepo(ctx context.Context) (Repository, fs.FS, error) { } } + if o.CloneForWrite { + err = validateRepoWritePermission(ctx, r) + if err != nil { + return nil, nil, fmt.Errorf("failed to validate repository write permissions: %w", err) + } + } + bootstrapFS, err := o.FS.Chroot(o.path) if err != nil { return nil, nil, err @@ -252,6 +259,49 @@ func (o *CloneOptions) GetRepo(ctx context.Context) (Repository, fs.FS, error) { return r, fs.Create(bootstrapFS), nil } +var validateRepoWritePermission = func(ctx context.Context, r *repo) error { + cert, err := r.auth.GetCertificate() + if err != nil { + return fmt.Errorf("failed getting repository certificates: %w", err) + } + + err = r.Repository.PushContext(ctx, &gg.PushOptions{ + Auth: getAuth(r.auth), + Progress: r.progress, + CABundle: cert, + }) + if err != nil { + if !errors.Is(err, gg.NoErrAlreadyUpToDate) { + // means there was already a commit to push, and it failed to push it + return err + } + // no commit to push, all up to date + if err := r.PushDummyCommit("Validating repository write permissions", ctx); err != nil { + return fmt.Errorf("failed to push dummy commit: %w", err) + } + } + + return nil +} + +func (r *repo) PushDummyCommit(commitMsg string, ctx context.Context) error { + + _, err := r.Persist(ctx, &PushOptions{ + CommitMsg: commitMsg, + }) + + if err != nil { + return fmt.Errorf("failed pushing commit to repository: %w", err) + } + + if err != nil { + log.G().Warnf("failed to reset commit: %v", err) + } + + return nil + +} + func (o *CloneOptions) URL() string { return o.url } diff --git a/pkg/git/repository_test.go b/pkg/git/repository_test.go index 22995ba3..0ad8ab4c 100644 --- a/pkg/git/repository_test.go +++ b/pkg/git/repository_test.go @@ -596,12 +596,13 @@ func Test_clone(t *testing.T) { func TestGetRepo(t *testing.T) { tests := map[string]struct { - opts *CloneOptions - wantErr string - cloneFn func(context.Context, *CloneOptions) (*repo, error) - createRepoFn func(context.Context, *CloneOptions) (defaultBranch string, err error) - initRepoFn func(context.Context, *CloneOptions, string) (*repo, error) - assertFn func(*testing.T, Repository, fs.FS, error) + opts *CloneOptions + wantErr string + cloneFn func(context.Context, *CloneOptions) (*repo, error) + validateRepoWritePermissionFn func(ctx context.Context, r *repo) error + createRepoFn func(context.Context, *CloneOptions) (defaultBranch string, err error) + initRepoFn func(context.Context, *CloneOptions, string) (*repo, error) + assertFn func(*testing.T, Repository, fs.FS, error) }{ "Should get a repo": { opts: &CloneOptions{ @@ -611,6 +612,9 @@ func TestGetRepo(t *testing.T) { cloneFn: func(_ context.Context, opts *CloneOptions) (*repo, error) { return &repo{}, nil }, + validateRepoWritePermissionFn: func(ctx context.Context, r *repo) error { + return nil + }, assertFn: func(t *testing.T, r Repository, f fs.FS, e error) { assert.NotNil(t, r) assert.NotNil(t, f) @@ -708,20 +712,25 @@ func TestGetRepo(t *testing.T) { assert.NotNil(t, f) assert.Nil(t, e) }, + validateRepoWritePermissionFn: func(ctx context.Context, r *repo) error { + return nil + }, }, } - origClone, origCreateRepo, origInitRepo := clone, createRepo, initRepo + origClone, origCreateRepo, origInitRepo, origValidateRepoWritePermission := clone, createRepo, initRepo, validateRepoWritePermission defer func() { clone = origClone createRepo = origCreateRepo initRepo = origInitRepo + validateRepoWritePermission = origValidateRepoWritePermission }() for tname, tt := range tests { t.Run(tname, func(t *testing.T) { clone = tt.cloneFn createRepo = tt.createRepoFn initRepo = tt.initRepoFn + validateRepoWritePermission = tt.validateRepoWritePermissionFn if tt.opts != nil { tt.opts.Parse() } @@ -1550,3 +1559,62 @@ func Test_repo_commit(t *testing.T) { }) } } +func Test_validateRepoWritePermission(t *testing.T) { + tests := map[string]struct { + opts *PushOptions + wantErr bool + beforeFn func(r *mocks.MockRepository, w *mocks.MockWorktree) + }{ + "Should fail getting git log": { + opts: nil, + wantErr: true, + beforeFn: func(r *mocks.MockRepository, w *mocks.MockWorktree) { + r.EXPECT().PushContext(gomock.Any(), &gg.PushOptions{ + Auth: nil, + Progress: os.Stderr, + }). + Times(1). + Return(gg.NoErrAlreadyUpToDate). + Times(1). + Return(fmt.Errorf("some error")) + }, + }, + } + + gitConfig := &config.Config{ + User: struct { + Name string + Email string + }{ + Name: "name", + Email: "email", + }, + } + + for tname, tt := range tests { + t.Run(tname, func(t *testing.T) { + ctrl := gomock.NewController(t) + mockRepo := mocks.NewMockRepository(ctrl) + mockWt := mocks.NewMockWorktree(ctrl) + mockProvider := &mockProvider{} + + mockRepo.EXPECT().ConfigScoped(gomock.Any()).Return(gitConfig, nil).AnyTimes() + getProvider = func(providerType, repoURL string, auth *Auth) (Provider, error) { return mockProvider, nil } + worktree = func(r gogit.Repository) (gogit.Worktree, error) { return mockWt, nil } + + r := &repo{ + Repository: mockRepo, + progress: os.Stderr, + } + + tt.beforeFn(mockRepo, mockWt) + + err := validateRepoWritePermission(context.Background(), r) + if (err != nil) != tt.wantErr { + t.Errorf("error = %v, wantErr %v", err, tt.wantErr) + return + } + + }) + } +} From c990267b83d1737e39eb7cc232c8adf3ef6879e2 Mon Sep 17 00:00:00 2001 From: kim-codefresh Date: Tue, 10 Jan 2023 17:35:02 +0200 Subject: [PATCH 2/4] bump Signed-off-by: kim-codefresh --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 7f00a5fa..29eb00c2 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -VERSION=v0.4.9 +VERSION=v0.4.10 OUT_DIR=dist CLI_NAME?=argocd-autopilot From 53d3ca517e4aaeac6b61a28c717c4d2f30393280 Mon Sep 17 00:00:00 2001 From: kim-codefresh Date: Wed, 11 Jan 2023 09:41:01 +0200 Subject: [PATCH 3/4] resolve comments Signed-off-by: kim-codefresh --- Makefile | 2 +- pkg/git/repository.go | 39 +++------------------------------------ 2 files changed, 4 insertions(+), 37 deletions(-) diff --git a/Makefile b/Makefile index 29eb00c2..7f00a5fa 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -VERSION=v0.4.10 +VERSION=v0.4.9 OUT_DIR=dist CLI_NAME?=argocd-autopilot diff --git a/pkg/git/repository.go b/pkg/git/repository.go index 79a64e6a..0d4cecfe 100644 --- a/pkg/git/repository.go +++ b/pkg/git/repository.go @@ -242,12 +242,10 @@ func (o *CloneOptions) GetRepo(ctx context.Context) (Repository, fs.FS, error) { default: return nil, nil, err } - } - - if o.CloneForWrite { + } else if o.CloneForWrite { err = validateRepoWritePermission(ctx, r) if err != nil { - return nil, nil, fmt.Errorf("failed to validate repository write permissions: %w", err) + return nil, nil, fmt.Errorf("failed to validate repository write permission: %w", err) } } @@ -260,46 +258,15 @@ func (o *CloneOptions) GetRepo(ctx context.Context) (Repository, fs.FS, error) { } var validateRepoWritePermission = func(ctx context.Context, r *repo) error { - cert, err := r.auth.GetCertificate() - if err != nil { - return fmt.Errorf("failed getting repository certificates: %w", err) - } - - err = r.Repository.PushContext(ctx, &gg.PushOptions{ - Auth: getAuth(r.auth), - Progress: r.progress, - CABundle: cert, - }) - if err != nil { - if !errors.Is(err, gg.NoErrAlreadyUpToDate) { - // means there was already a commit to push, and it failed to push it - return err - } - // no commit to push, all up to date - if err := r.PushDummyCommit("Validating repository write permissions", ctx); err != nil { - return fmt.Errorf("failed to push dummy commit: %w", err) - } - } - - return nil -} - -func (r *repo) PushDummyCommit(commitMsg string, ctx context.Context) error { - _, err := r.Persist(ctx, &PushOptions{ - CommitMsg: commitMsg, + CommitMsg: "Validating repository write permission", }) if err != nil { return fmt.Errorf("failed pushing commit to repository: %w", err) } - if err != nil { - log.G().Warnf("failed to reset commit: %v", err) - } - return nil - } func (o *CloneOptions) URL() string { From bec397a3cd153d5edc4cc150d725670ed689e176 Mon Sep 17 00:00:00 2001 From: kim-codefresh Date: Wed, 11 Jan 2023 09:58:02 +0200 Subject: [PATCH 4/4] fix test Signed-off-by: kim-codefresh --- pkg/git/repository_test.go | 40 +++++++++++++++++++++++++++++++++++--- 1 file changed, 37 insertions(+), 3 deletions(-) diff --git a/pkg/git/repository_test.go b/pkg/git/repository_test.go index 0ad8ab4c..76662618 100644 --- a/pkg/git/repository_test.go +++ b/pkg/git/repository_test.go @@ -1565,7 +1565,7 @@ func Test_validateRepoWritePermission(t *testing.T) { wantErr bool beforeFn func(r *mocks.MockRepository, w *mocks.MockWorktree) }{ - "Should fail getting git log": { + "Should fail if push context failed": { opts: nil, wantErr: true, beforeFn: func(r *mocks.MockRepository, w *mocks.MockWorktree) { @@ -1573,10 +1573,44 @@ func Test_validateRepoWritePermission(t *testing.T) { Auth: nil, Progress: os.Stderr, }). - Times(1). - Return(gg.NoErrAlreadyUpToDate). Times(1). Return(fmt.Errorf("some error")) + w.EXPECT().AddGlob(gomock.Any()). + Times(1). + Return(nil) + w.EXPECT().Commit("Validating repository write permission", gomock.Any()). + Times(1). + Return(plumbing.Hash{}, nil) + }, + }, + "Should fail if commit failed": { + opts: nil, + wantErr: true, + beforeFn: func(r *mocks.MockRepository, w *mocks.MockWorktree) { + w.EXPECT().AddGlob(gomock.Any()). + Times(1). + Return(nil) + w.EXPECT().Commit("Validating repository write permission", gomock.Any()). + Times(1). + Return(plumbing.Hash{}, fmt.Errorf("some error")) + }, + }, + "Should succeed if push context succeed": { + opts: nil, + wantErr: false, + beforeFn: func(r *mocks.MockRepository, w *mocks.MockWorktree) { + r.EXPECT().PushContext(gomock.Any(), &gg.PushOptions{ + Auth: nil, + Progress: os.Stderr, + }). + Times(1). + Return(nil) + w.EXPECT().AddGlob(gomock.Any()). + Times(1). + Return(nil) + w.EXPECT().Commit("Validating repository write permission", gomock.Any()). + Times(1). + Return(plumbing.Hash{}, nil) }, }, }