Merge pull request #169 from fluxcd/edit-prs

makkes · web-flow · commit 4b08724750fb · 2022-11-08T16:37:56.000+01:00
Add API for editing pull requests
diff --git a/github/client_repository_file.go b/github/client_repository_file.go
@@ -19,7 +19,7 @@ package github
 import (
 	"context"
 	"fmt"
-	"io/ioutil"
+	"io"
 
 	"github.com/fluxcd/go-git-providers/gitprovider"
 	"github.com/google/go-github/v47/github"
@@ -65,7 +65,7 @@ func (c *FileClient) Get(ctx context.Context, path, branch string, optFns ...git
 		if err != nil {
 			return nil, err
 		}
-		content, err := ioutil.ReadAll(output)
+		content, err := io.ReadAll(output)
 		if err != nil {
 			return nil, err
 		}
diff --git a/github/client_repository_pullrequest.go b/github/client_repository_pullrequest.go
@@ -66,6 +66,17 @@ func (c *PullRequestClient) Create(ctx context.Context, title, branch, baseBranc
 	return newPullRequest(c.clientContext, pr), nil
 }
 
+// Edit modifies an existing PR. Please refer to "EditOptions" for details on which data can be edited.
+func (c *PullRequestClient) Edit(ctx context.Context, number int, opts gitprovider.EditOptions) (gitprovider.PullRequest, error) {
+	editPR := &github.PullRequest{}
+	editPR.Title = opts.Title
+	editedPR, _, err := c.c.Client().PullRequests.Edit(ctx, c.ref.GetIdentity(), c.ref.GetRepository(), number, editPR)
+	if err != nil {
+		return nil, err
+	}
+	return newPullRequest(c.clientContext, editedPR), nil
+}
+
 // Get retrieves an existing pull request by number
 func (c *PullRequestClient) Get(ctx context.Context, number int) (gitprovider.PullRequest, error) {
 
diff --git a/github/integration_test.go b/github/integration_test.go
@@ -23,6 +23,7 @@ import (
 	"math/rand"
 	"net/http"
 	"os"
+	"reflect"
 	"strings"
 	"sync"
 	"testing"
@@ -427,7 +428,7 @@ var _ = Describe("GitHub Provider", func() {
 		Expect(hasPermission).To(Equal(true))
 	})
 
-	It("should be possible to create a pr for a user repository", func() {
+	It("should be possible to create and edit a pr for a user repository", func() {
 
 		userRepoRef := newUserRepoRef(testUser, testUserRepoName)
 
@@ -480,6 +481,7 @@ var _ = Describe("GitHub Provider", func() {
 		Expect(pr.Get().Merged).To(BeFalse())
 
 		prs, err := userRepo.PullRequests().List(ctx)
+		Expect(err).NotTo(HaveOccurred())
 		Expect(len(prs)).To(Equal(1))
 		Expect(prs[0].Get().WebURL).To(Equal(pr.Get().WebURL))
 
@@ -508,13 +510,23 @@ var _ = Describe("GitHub Provider", func() {
 		Expect(pr.Get().WebURL).ToNot(BeEmpty())
 		Expect(pr.Get().Merged).To(BeFalse())
 
-		err = userRepo.PullRequests().Merge(ctx, pr.Get().Number, gitprovider.MergeMethodMerge, "merged")
+		editedPR, err := userRepo.PullRequests().Edit(ctx, pr.Get().Number, gitprovider.EditOptions{
+			Title: gitprovider.StringVar("a new title"),
+		})
+		Expect(err).ToNot(HaveOccurred())
+
+		err = userRepo.PullRequests().Merge(ctx, editedPR.Get().Number, gitprovider.MergeMethodMerge, "merged")
 		Expect(err).ToNot(HaveOccurred())
 
-		getPR, err = userRepo.PullRequests().Get(ctx, pr.Get().Number)
+		getPR, err = userRepo.PullRequests().Get(ctx, editedPR.Get().Number)
 		Expect(err).ToNot(HaveOccurred())
 
 		Expect(getPR.Get().Merged).To(BeTrue())
+		apiObject := getPR.APIObject()
+		githubPR, ok := apiObject.(*github.PullRequest)
+		Expect(ok).To(BeTrue(), "API object of PullRequest has unexpected type %q", reflect.TypeOf(apiObject))
+		Expect(githubPR.Title).ToNot(BeNil())
+		Expect(*githubPR.Title).To(Equal("a new title"))
 	})
 
 	It("should be possible to download files from path and branch specified", func() {
diff --git a/gitlab/client_repositories_org.go b/gitlab/client_repositories_org.go
@@ -118,7 +118,7 @@ func (c *OrgRepositoriesClient) Reconcile(ctx context.Context, ref gitprovider.O
 	return actual, actionTaken, err
 }
 
-//nolint
+// nolint
 func createProject(ctx context.Context, c gitlabClient, ref gitprovider.RepositoryRef, groupName string, req gitprovider.RepositoryInfo, opts ...gitprovider.RepositoryCreateOption) (*gitlab.Project, error) {
 	// First thing, validate and default the request to ensure a valid and fully-populated object
 	// (to minimize any possible diffs between desired and actual state)
diff --git a/gitlab/client_repository_file.go b/gitlab/client_repository_file.go
@@ -19,7 +19,7 @@ package gitlab
 import (
 	"context"
 	"encoding/base64"
-	"io/ioutil"
+	"io"
 	"strings"
 
 	"github.com/fluxcd/go-git-providers/gitprovider"
@@ -72,7 +72,7 @@ func (c *FileClient) Get(ctx context.Context, path, branch string, optFns ...git
 		}
 		filePath := fileDownloaded.FilePath
 		fileContentDecoded := base64.NewDecoder(base64.RawStdEncoding, strings.NewReader(fileDownloaded.Content))
-		fileBytes, err := ioutil.ReadAll(fileContentDecoded)
+		fileBytes, err := io.ReadAll(fileContentDecoded)
 		if err != nil {
 			return nil, err
 		}
diff --git a/gitlab/client_repository_pullrequest.go b/gitlab/client_repository_pullrequest.go
@@ -71,6 +71,18 @@ func (c *PullRequestClient) Create(_ context.Context, title, branch, baseBranch,
 	return newPullRequest(c.clientContext, mr), nil
 }
 
+// Edit modifies an existing MR. Please refer to "EditOptions" for details on which data can be edited.
+func (c *PullRequestClient) Edit(ctx context.Context, number int, opts gitprovider.EditOptions) (gitprovider.PullRequest, error) {
+	mrUpdate := &gitlab.UpdateMergeRequestOptions{
+		Title: opts.Title,
+	}
+	editedMR, _, err := c.c.Client().MergeRequests.UpdateMergeRequest(getRepoPath(c.ref), number, mrUpdate)
+	if err != nil {
+		return nil, err
+	}
+	return newPullRequest(c.clientContext, editedMR), nil
+}
+
 // Get retrieves an existing pull request by number
 func (c *PullRequestClient) Get(_ context.Context, number int) (gitprovider.PullRequest, error) {
 
diff --git a/gitlab/integration_test.go b/gitlab/integration_test.go
@@ -26,6 +26,7 @@ import (
 	"math/rand"
 	"net/http"
 	"os"
+	"reflect"
 	"strings"
 	"sync"
 	"testing"
@@ -548,7 +549,7 @@ var _ = Describe("GitLab Provider", func() {
 			Permission: &pushPermission,
 		}
 
-		ta, actionTaken, err = repo.TeamAccess().Reconcile(ctx, teamInfo)
+		_, actionTaken, err = repo.TeamAccess().Reconcile(ctx, teamInfo)
 		Expect(err).ToNot(HaveOccurred())
 		Expect(actionTaken).To(Equal(false))
 	})
@@ -752,7 +753,7 @@ var _ = Describe("GitLab Provider", func() {
 		validateUserRepo(newRepo, repoRef)
 	})
 
-	It("should be possible to create a pr for a user repository", func() {
+	It("should be possible to create and edit a pr for a user repository", func() {
 
 		testRepoName = fmt.Sprintf("test-repo2-%03d", rand.Intn(1000))
 		repoRef := newUserRepoRef(testUserName, testRepoName)
@@ -865,8 +866,20 @@ var _ = Describe("GitLab Provider", func() {
 		Expect(pr.Get().WebURL).ToNot(BeEmpty())
 		Expect(pr.Get().Merged).To(BeFalse())
 
-		err = userRepo.PullRequests().Merge(ctx, pr.Get().Number, gitprovider.MergeMethodMerge, "merged")
+		editedPR, err := userRepo.PullRequests().Edit(ctx, pr.Get().Number, gitprovider.EditOptions{
+			Title: gitprovider.StringVar("a new title"),
+		})
+		Expect(err).ToNot(HaveOccurred())
+
+		err = userRepo.PullRequests().Merge(ctx, editedPR.Get().Number, gitprovider.MergeMethodMerge, "merged")
+		Expect(err).ToNot(HaveOccurred())
+
+		getPR, err := userRepo.PullRequests().Get(ctx, editedPR.Get().Number)
 		Expect(err).ToNot(HaveOccurred())
+		apiObject := getPR.APIObject()
+		gitlabMR, ok := apiObject.(*gitlab.MergeRequest)
+		Expect(ok).To(BeTrue(), "API object of PullRequest has unexpected type %q", reflect.TypeOf(apiObject))
+		Expect(gitlabMR.Title).To(Equal("a new title"))
 
 		expectPRToBeMerged(ctx, userRepo, pr.Get().Number)
 
diff --git a/gitlab/resource_repository.go b/gitlab/resource_repository.go
@@ -296,7 +296,7 @@ func (s *gitlabProjectSpec) Equals(other *gitlabProjectSpec) bool {
 	return cmp.Equal(s, other)
 }
 
-//nolint
+// nolint
 var gitlabVisibilityMap = map[gitprovider.RepositoryVisibility]gogitlab.VisibilityValue{
 	gitprovider.RepositoryVisibilityInternal: gogitlab.InternalVisibility,
 	gitprovider.RepositoryVisibilityPrivate:  gogitlab.PrivateVisibility,
diff --git a/gitlab/resource_teamaccess.go b/gitlab/resource_teamaccess.go
@@ -119,7 +119,7 @@ func (ta *teamAccess) Reconcile(ctx context.Context) (bool, error) {
 	return true, ta.Update(ctx)
 }
 
-//nolint
+// nolint
 var permissionPriority = map[int]gitprovider.RepositoryPermission{
 	10: gitprovider.RepositoryPermissionPull,
 	20: gitprovider.RepositoryPermissionTriage,
diff --git a/gitprovider/client.go b/gitprovider/client.go
@@ -231,12 +231,21 @@ type PullRequestClient interface {
 	List(ctx context.Context) ([]PullRequest, error)
 	// Create creates a pull request with the given specifications.
 	Create(ctx context.Context, title, branch, baseBranch, description string) (PullRequest, error)
+	// Edit allows for changing an existing pull request using the given options. Please refer to "EditOptions" for details on which data can be
+	// edited.
+	Edit(ctx context.Context, number int, opts EditOptions) (PullRequest, error)
 	// Get retrieves an existing pull request by number
 	Get(ctx context.Context, number int) (PullRequest, error)
 	// Merge merges a pull request with via either the "Squash" or "Merge" method
 	Merge(ctx context.Context, number int, mergeMethod MergeMethod, message string) error
 }
 
+// EditOptions is provided to a PullRequestClient's "Edit" method for updating an existing pull request.
+type EditOptions struct {
+	// Title is set to a non-nil value to request a pull request's title to be changed.
+	Title *string
+}
+
 // FileClient operates on the branches for a specific repository.
 // This client can be accessed through Repository.Branches().
 type FileClient interface {
diff --git a/gitprovider/enums.go b/gitprovider/enums.go
@@ -49,6 +49,7 @@ const (
 )
 
 // knownRepositoryVisibilityValues is a map of known RepositoryVisibility values, used for validation.
+//
 //nolint:gochecknoglobals
 var knownRepositoryVisibilityValues = map[RepositoryVisibility]struct{}{
 	RepositoryVisibilityPublic:   {},
@@ -98,6 +99,7 @@ const (
 )
 
 // knownRepositoryVisibilityValues is a map of known RepositoryPermission values, used for validation.
+//
 //nolint:gochecknoglobals
 var knownRepositoryPermissionValues = map[RepositoryPermission]struct{}{
 	RepositoryPermissionPull:     {},
@@ -140,6 +142,7 @@ const (
 )
 
 // knownLicenseTemplateValues is a map of known LicenseTemplate values, used for validation
+//
 //nolint:gochecknoglobals
 var knownLicenseTemplateValues = map[LicenseTemplate]struct{}{
 	LicenseTemplateApache2: {},
diff --git a/stash/client.go b/stash/client.go
@@ -155,18 +155,19 @@ func WithAuth(username string, token string) ClientOptionsFunc {
 // If the http.Header is nil, a default http.Header is used.
 // ClientOptionsFunc is an optional function and can be used to configure the client.
 // Example:
-//  c, err := NewClient(
-//  	&http.Client {
-//  		Transport: defaultTransport,
-//  		Timeout:   defaultTimeout,
-//  		}, "https://github.com",
-//  		&http.Header {
-//  			"Content-Type": []string{"application/json"},
-//  		},
-//  		logr.Logger{},
-//  		func(c *Client) {
-//  			c.DisableRetries = true
-//  	})
+//
+//	c, err := NewClient(
+//		&http.Client {
+//			Transport: defaultTransport,
+//			Timeout:   defaultTimeout,
+//			}, "https://github.com",
+//			&http.Header {
+//				"Content-Type": []string{"application/json"},
+//			},
+//			logr.Logger{},
+//			func(c *Client) {
+//				c.DisableRetries = true
+//		})
 func NewClient(httpClient *http.Client, host string, header *http.Header, logger logr.Logger, opts ...ClientOptionsFunc) (*Client, error) {
 	if host == "" {
 		return nil, errors.New("host is required")
diff --git a/stash/client_repository_pullrequest.go b/stash/client_repository_pullrequest.go
@@ -143,6 +143,40 @@ func (c *PullRequestClient) Create(ctx context.Context, title, branch, baseBranc
 	return newPullRequest(created), nil
 }
 
+// Edit modifies an existing PR. Please refer to "EditOptions" for details on which data can be edited.
+func (c *PullRequestClient) Edit(ctx context.Context, number int, opts gitprovider.EditOptions) (gitprovider.PullRequest, error) {
+	projectKey, repoSlug := getStashRefs(c.ref)
+
+	// check if it is a user repository
+	// if yes, we need to add a tilde to the user login and use it as the project key
+	if r, ok := c.ref.(gitprovider.UserRepositoryRef); ok {
+		projectKey = addTilde(r.UserLogin)
+	}
+
+	// need to fetch the PR first to get the right version number
+	pr, err := c.Get(ctx, number)
+	if err != nil {
+		return nil, fmt.Errorf("failed to get PR %d: %w", number, err)
+	}
+	apiObject, ok := pr.APIObject().(*PullRequest)
+	if !ok {
+		return nil, fmt.Errorf("API object is of unexpected type") // this should never happen!
+	}
+
+	if opts.Title != nil {
+		apiObject.Title = *opts.Title
+	}
+	// the REST API doesn't accept the following fields to be set for update requests
+	apiObject.Author = nil
+	apiObject.Participants = nil
+	edited, err := c.client.PullRequests.Update(ctx, projectKey, repoSlug, apiObject)
+	if err != nil {
+		return nil, fmt.Errorf("failed to edit pull request: %w", err)
+	}
+
+	return newPullRequest(edited), nil
+}
+
 func validatePullRequestsAPI(apiObj *PullRequest) error {
 	return validateAPIObject("Stash.PullRequest", func(validator validation.Validator) {
 		// Make sure there is a version and a title
diff --git a/stash/client_test.go b/stash/client_test.go
@@ -346,7 +346,7 @@ func (f RoundTripFunc) RoundTrip(req *http.Request) (*http.Response, error) {
 	return f(req)
 }
 
-//NewTestClient returns a Client with Transport replaced to avoid making real calls
+// NewTestClient returns a Client with Transport replaced to avoid making real calls
 func NewTestClient(t *testing.T, fn RoundTripFunc, opts ...ClientOptionsFunc) *Client {
 	c, err := NewClient(nil, defaultHost, nil, initLogger(t))
 	if err != nil {
diff --git a/stash/deploy_keys.go b/stash/deploy_keys.go
@@ -219,7 +219,6 @@ func (s *DeployKeysService) Delete(ctx context.Context, projectKey, repositorySl
 
 // UpdateKeyPermission updates the given access key permission
 // UpdateKeyPermission uses the endpoint "PUT /rest/keys/1.0/projects/{projectKey}/ssh/{keyId}/permission/{permission}".
-//
 func (s *DeployKeysService) UpdateKeyPermission(ctx context.Context, projectKey, repositorySlug string, keyID int, permission string) (*DeployKey, error) {
 	req, err := s.Client.NewRequest(ctx, http.MethodPut, newKeysURI(projectsURI, projectKey, RepositoriesURI, repositorySlug, deployKeysURI, strconv.Itoa(keyID),
 		keyPermisionsURI, permission))
diff --git a/stash/integration_repositories_user_test.go b/stash/integration_repositories_user_test.go
@@ -21,6 +21,7 @@ import (
 	"errors"
 	"fmt"
 	"math/rand"
+	"reflect"
 
 	"github.com/fluxcd/go-git-providers/gitprovider"
 	"github.com/fluxcd/go-git-providers/gitprovider/testutils"
@@ -220,11 +221,30 @@ var _ = Describe("Stash Provider", func() {
 		Expect(err).ToNot(HaveOccurred())
 		Expect(pr.Get().WebURL).ToNot(BeEmpty())
 
+		editedPR, err := userRepo.PullRequests().Edit(ctx, pr.Get().Number, gitprovider.EditOptions{
+			Title: gitprovider.StringVar("a new title"),
+		})
+		Expect(err).ToNot(HaveOccurred(), "error editing PR")
+		Expect(editedPR).ToNot(BeNil(), "returned PR should never be nil if no error was returned")
+
+		// edit one more time to make sure the version number is taken into account
+
+		editedPR, err = userRepo.PullRequests().Edit(ctx, pr.Get().Number, gitprovider.EditOptions{
+			Title: gitprovider.StringVar("another new title"),
+		})
+		Expect(err).ToNot(HaveOccurred(), "error editing PR a second time")
+		Expect(editedPR).ToNot(BeNil(), "returned PR should never be nil if no error was returned")
+
 		// List PRs
 		prs, err := userRepo.PullRequests().List(ctx)
 		Expect(err).ToNot(HaveOccurred())
 		Expect(len(prs)).To(Equal(1))
 
+		apiObject := prs[0].APIObject()
+		stashPR, ok := apiObject.(*PullRequest)
+		Expect(ok).To(BeTrue(), "API object of PullRequest has unexpected type %q", reflect.TypeOf(apiObject))
+		Expect(stashPR.Title).To(Equal("another new title"))
+
 		// Merge PR
 		id := pr.APIObject().(*PullRequest).ID
 		err = userRepo.PullRequests().Merge(ctx, id, "merge", "merged")
diff --git a/stash/pull_requests.go b/stash/pull_requests.go
diff --git a/stash/pull_requests_test.go b/stash/pull_requests_test.go
diff --git a/stash/resource_organization.go b/stash/resource_organization.go
diff --git a/stash/resource_pullrequest.go b/stash/resource_pullrequest.go
diff --git a/validation/multierror.go b/validation/multierror.go

Original file line number	Diff line number	Diff line change
`@@ -118,7 +118,7 @@ func (c *OrgRepositoriesClient) Reconcile(ctx context.Context, ref gitprovider.O`
`118`	`118`	`return actual, actionTaken, err`
`119`	`119`	`}`
`120`	`120`
`121`		`-//nolint`
	`121`	`+// nolint`
`122`	`122`	`func createProject(ctx context.Context, c gitlabClient, ref gitprovider.RepositoryRef, groupName string, req gitprovider.RepositoryInfo, opts ...gitprovider.RepositoryCreateOption) (*gitlab.Project, error) {`
`123`	`123`	`// First thing, validate and default the request to ensure a valid and fully-populated object`
`124`	`124`	`// (to minimize any possible diffs between desired and actual state)`
Original file line number	Diff line number	Diff line change
`@@ -296,7 +296,7 @@ func (s gitlabProjectSpec) Equals(other gitlabProjectSpec) bool {`
`296`	`296`	`return cmp.Equal(s, other)`
`297`	`297`	`}`
`298`	`298`
`299`		`-//nolint`
	`299`	`+// nolint`
`300`	`300`	`var gitlabVisibilityMap = map[gitprovider.RepositoryVisibility]gogitlab.VisibilityValue{`
`301`	`301`	`gitprovider.RepositoryVisibilityInternal: gogitlab.InternalVisibility,`
`302`	`302`	`gitprovider.RepositoryVisibilityPrivate: gogitlab.PrivateVisibility,`
Original file line number	Diff line number	Diff line change
`@@ -119,7 +119,7 @@ func (ta *teamAccess) Reconcile(ctx context.Context) (bool, error) {`
`119`	`119`	`return true, ta.Update(ctx)`
`120`	`120`	`}`
`121`	`121`
`122`		`-//nolint`
	`122`	`+// nolint`
`123`	`123`	`var permissionPriority = map[int]gitprovider.RepositoryPermission{`
`124`	`124`	`10: gitprovider.RepositoryPermissionPull,`
`125`	`125`	`20: gitprovider.RepositoryPermissionTriage,`