From baca7c250e4d1eca7a6ee6f416280edb30caed0d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roberto=20Jim=C3=A9nez=20S=C3=A1nchez?= Date: Tue, 26 Nov 2024 12:26:04 +0100 Subject: [PATCH] [Prototyping] Ensure git branch exists in write operations for github repositories (#96998) * Create branch when ref is passed * Change approach to ensure branch already exists * Fix error handling branch not found --- .../apis/provisioning/repository/github.go | 71 +++++++++++++++++++ .../provisioning/repository/github/client.go | 5 ++ .../repository/github/client_mock.go | 41 ++++++++++- .../provisioning/repository/github/real.go | 37 ++++++++++ .../apis/provisioning/repository/local.go | 12 ++++ 5 files changed, 165 insertions(+), 1 deletion(-) diff --git a/pkg/registry/apis/provisioning/repository/github.go b/pkg/registry/apis/provisioning/repository/github.go index be3662f0ed6..93580add86e 100644 --- a/pkg/registry/apis/provisioning/repository/github.go +++ b/pkg/registry/apis/provisioning/repository/github.go @@ -120,6 +120,14 @@ func (r *githubRepository) Read(ctx context.Context, logger *slog.Logger, filePa } func (r *githubRepository) Create(ctx context.Context, logger *slog.Logger, path, ref string, data []byte, comment string) error { + if ref == "" { + ref = r.config.Spec.GitHub.Branch + } + + if err := r.ensureBranchExists(ctx, ref); err != nil { + return fmt.Errorf("create branch on create: %w", err) + } + owner := r.config.Spec.GitHub.Owner repo := r.config.Spec.GitHub.Repository @@ -136,6 +144,14 @@ func (r *githubRepository) Create(ctx context.Context, logger *slog.Logger, path } func (r *githubRepository) Update(ctx context.Context, logger *slog.Logger, path, ref string, data []byte, comment string) error { + if ref == "" { + ref = r.config.Spec.GitHub.Branch + } + + if err := r.ensureBranchExists(ctx, ref); err != nil { + return fmt.Errorf("create branch on update: %w", err) + } + owner := r.config.Spec.GitHub.Owner repo := r.config.Spec.GitHub.Repository @@ -160,6 +176,14 @@ func (r *githubRepository) Update(ctx context.Context, logger *slog.Logger, path } func (r *githubRepository) Delete(ctx context.Context, logger *slog.Logger, path, ref, comment string) error { + if ref == "" { + ref = r.config.Spec.GitHub.Branch + } + + if err := r.ensureBranchExists(ctx, ref); err != nil { + return fmt.Errorf("create branch on delete: %w", err) + } + owner := r.config.Spec.GitHub.Owner repo := r.config.Spec.GitHub.Repository @@ -179,6 +203,53 @@ func (r *githubRepository) Delete(ctx context.Context, logger *slog.Logger, path return r.gh.DeleteFile(ctx, owner, repo, path, ref, comment, file.GetSHA()) } +// isValidGitBranchName checks if a branch name is valid. +func isValidGitBranchName(branch string) bool { + // TODO: add branch validation here as well as in the frontend using the same restrictions + return branch != "" +} + +func (r *githubRepository) ensureBranchExists(ctx context.Context, branchName string) error { + if !isValidGitBranchName(branchName) { + return &apierrors.StatusError{ + ErrStatus: metav1.Status{ + Code: http.StatusBadRequest, + Message: "invalid branch name", + }, + } + } + + ok, err := r.gh.BranchExists(ctx, r.config.Spec.GitHub.Owner, r.config.Spec.GitHub.Repository, branchName) + if err != nil { + return fmt.Errorf("check branch exists: %w", err) + } + + if ok { + r.logger.InfoContext(ctx, "branch already exists", "branch", branchName) + + return nil + } + + owner := r.config.Spec.GitHub.Owner + repo := r.config.Spec.GitHub.Repository + + srcBranch := r.config.Spec.GitHub.Branch + if err := r.gh.CreateBranch(ctx, owner, repo, srcBranch, branchName); err != nil { + if errors.Is(err, pgh.ErrResourceAlreadyExists) { + return &apierrors.StatusError{ + ErrStatus: metav1.Status{ + Code: http.StatusConflict, + Message: "branch already exists", + }, + } + } + + return fmt.Errorf("create branch: %w", err) + } + + return nil +} + // Webhook implements provisioning.Repository. func (r *githubRepository) Webhook(ctx context.Context, logger *slog.Logger, responder rest.Responder) http.HandlerFunc { return func(w http.ResponseWriter, req *http.Request) { diff --git a/pkg/registry/apis/provisioning/repository/github/client.go b/pkg/registry/apis/provisioning/repository/github/client.go index 361ae5d1b87..0d14b9ab9a7 100644 --- a/pkg/registry/apis/provisioning/repository/github/client.go +++ b/pkg/registry/apis/provisioning/repository/github/client.go @@ -59,6 +59,11 @@ type Client interface { // If ".." appears in the "path", this method will return an error. DeleteFile(ctx context.Context, owner, repository, path, branch, message, hash string) error + // CreateBranch creates a new branch in the repository. + CreateBranch(ctx context.Context, owner, repository, sourceBranch, branchName string) error + // BranchExists checks if a branch exists in the repository. + BranchExists(ctx context.Context, owner, repository, branchName string) (bool, error) + ListWebhooks(ctx context.Context, owner, repository string) ([]WebhookConfig, error) CreateWebhook(ctx context.Context, owner, repository string, cfg WebhookConfig) error DeleteWebhook(ctx context.Context, owner, repository string, webhookID int64) error diff --git a/pkg/registry/apis/provisioning/repository/github/client_mock.go b/pkg/registry/apis/provisioning/repository/github/client_mock.go index c50f3641251..e1faa8f7655 100644 --- a/pkg/registry/apis/provisioning/repository/github/client_mock.go +++ b/pkg/registry/apis/provisioning/repository/github/client_mock.go @@ -13,6 +13,44 @@ type MockClient struct { mock.Mock } +// CreateBranch provides a mock function with given fields: ctx, owner, repository, sourceBranch, branchName +func (_m *MockClient) CreateBranch(ctx context.Context, owner string, repository string, sourceBranch string, branchName string) error { + ret := _m.Called(ctx, owner, repository, sourceBranch, branchName) + + if len(ret) == 0 { + panic("no return value specified for CreateBranch") + } + + var r0 error + if rf, ok := ret.Get(0).(func(context.Context, string, string, string, string) error); ok { + r0 = rf(ctx, owner, repository, sourceBranch, branchName) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +func (_m *MockClient) BranchExists(ctx context.Context, owner string, repository string, branchName string) (bool, error) { + ret := _m.Called(ctx, owner, repository, branchName) + if len(ret) == 0 { + panic("no return value specified for BranchExists") + } + var r0 bool + if rf, ok := ret.Get(0).(func(context.Context, string, string, string) bool); ok { + r0 = rf(ctx, owner, repository, branchName) + } else { + r0 = ret.Bool(0) + } + var r1 error + if rf, ok := ret.Get(1).(func(context.Context, string, string, string) error); ok { + r1 = rf(ctx, owner, repository, branchName) + } else { + r1 = ret.Error(1) + } + return r0, r1 +} + // CreateFile provides a mock function with given fields: ctx, owner, repository, path, branch, message, content func (_m *MockClient) CreateFile(ctx context.Context, owner string, repository string, path string, branch string, message string, content []byte) error { ret := _m.Called(ctx, owner, repository, path, branch, message, content) @@ -195,7 +233,8 @@ func (_m *MockClient) UpdateFile(ctx context.Context, owner string, repository s func NewMockClient(t interface { mock.TestingT Cleanup(func()) -}) *MockClient { +}, +) *MockClient { mock := &MockClient{} mock.Mock.Test(t) diff --git a/pkg/registry/apis/provisioning/repository/github/real.go b/pkg/registry/apis/provisioning/repository/github/real.go index 01b7c9cdc9b..d9de5587df8 100644 --- a/pkg/registry/apis/provisioning/repository/github/real.go +++ b/pkg/registry/apis/provisioning/repository/github/real.go @@ -148,6 +148,43 @@ func (r *realImpl) DeleteFile(ctx context.Context, owner, repository, path, bran return err } +func (r *realImpl) CreateBranch(ctx context.Context, owner, repository, sourceBranch, branchName string) error { + // Fail if the branch already exists + if _, _, err := r.gh.Repositories.GetBranch(ctx, owner, repository, branchName, 0); err == nil { + return ErrResourceAlreadyExists + } + + // Branch out based on the repository branch + baseRef, _, err := r.gh.Repositories.GetBranch(ctx, owner, repository, sourceBranch, 0) + if err != nil { + return fmt.Errorf("get base branch: %w", err) + } + + if _, _, err := r.gh.Git.CreateRef(ctx, owner, repository, &github.Reference{ + Ref: github.String(fmt.Sprintf("refs/heads/%s", branchName)), + Object: &github.GitObject{ + SHA: baseRef.Commit.SHA, + }, + }); err != nil { + return fmt.Errorf("create branch ref: %w", err) + } + + return nil +} + +func (r *realImpl) BranchExists(ctx context.Context, owner, repository, branchName string) (bool, error) { + _, resp, err := r.gh.Repositories.GetBranch(ctx, owner, repository, branchName, 0) + if err == nil { + return true, nil + } + + if resp.StatusCode == http.StatusNotFound { + return false, nil + } + + return false, err +} + func (r *realImpl) ListWebhooks(ctx context.Context, owner, repository string) ([]WebhookConfig, error) { hooks, _, err := r.gh.Repositories.ListHooks(ctx, owner, repository, nil) if err != nil { diff --git a/pkg/registry/apis/provisioning/repository/local.go b/pkg/registry/apis/provisioning/repository/local.go index 23e4469e3e4..5ef1de58869 100644 --- a/pkg/registry/apis/provisioning/repository/local.go +++ b/pkg/registry/apis/provisioning/repository/local.go @@ -152,6 +152,10 @@ func (r *localRepository) Read(ctx context.Context, logger *slog.Logger, path st } func (r *localRepository) Create(ctx context.Context, logger *slog.Logger, path string, ref string, data []byte, comment string) error { + if ref != "" { + return apierrors.NewBadRequest("local repository does not support ref") + } + if r.path == "" { return &apierrors.StatusError{ ErrStatus: metav1.Status{ @@ -172,6 +176,10 @@ func (r *localRepository) Create(ctx context.Context, logger *slog.Logger, path } func (r *localRepository) Update(ctx context.Context, logger *slog.Logger, path string, ref string, data []byte, comment string) error { + if ref != "" { + return apierrors.NewBadRequest("local repository does not support ref") + } + if r.path == "" { return &apierrors.StatusError{ ErrStatus: metav1.Status{ @@ -189,6 +197,10 @@ func (r *localRepository) Update(ctx context.Context, logger *slog.Logger, path } func (r *localRepository) Delete(ctx context.Context, logger *slog.Logger, path string, ref string, comment string) error { + if ref != "" { + return apierrors.NewBadRequest("local repository does not support ref") + } + if r.path == "" { return &apierrors.StatusError{ ErrStatus: metav1.Status{