Merge pull request #229 from fluxcd/fix-userlogin-usage

makkes · web-flow · commit 7161fd0db7fe · 2023-06-21T16:05:36.000+02:00
Return correct repo info after creation
diff --git a/gitea/client_repositories_org.go b/gitea/client_repositories_org.go
@@ -77,7 +77,7 @@ func (c *OrgRepositoriesClient) List(ctx context.Context, ref gitprovider.Organi
 		// apiObj is already validated at ListOrgRepos
 		repos = append(repos, newOrgRepository(c.clientContext, apiObj, gitprovider.OrgRepositoryRef{
 			OrganizationRef: ref,
-			RepositoryName:  *&apiObj.Name,
+			RepositoryName:  apiObj.Name,
 		}))
 	}
 	return repos, nil
diff --git a/gitea/client_repositories_user.go b/gitea/client_repositories_user.go
@@ -19,6 +19,7 @@ package gitea
 import (
 	"context"
 	"errors"
+	"fmt"
 
 	"code.gitea.io/sdk/gitea"
 	"github.com/fluxcd/go-git-providers/gitprovider"
@@ -111,6 +112,12 @@ func (c *UserRepositoriesClient) Create(ctx context.Context,
 	if err != nil {
 		return nil, err
 	}
+
+	if apiObj.Owner == nil {
+		return nil, fmt.Errorf("returned API object doesn't have an owner")
+	}
+	ref.UserLogin = apiObj.Owner.UserName
+
 	return newUserRepository(c.clientContext, apiObj, ref), nil
 }
 
diff --git a/gitea/client_repository_commit.go b/gitea/client_repository_commit.go
@@ -24,9 +24,6 @@ import (
 	"github.com/fluxcd/go-git-providers/gitprovider"
 )
 
-var giteaNewFileMode = "100644"
-var giteaBlobTypeFile = "blob"
-
 // CommitClient implements the gitprovider.CommitClient interface.
 var _ gitprovider.CommitClient = &CommitClient{}
 
diff --git a/gitea/integration_repositories_org_test.go b/gitea/integration_repositories_org_test.go
@@ -61,9 +61,9 @@ var _ = Describe("Gitea Provider", func() {
 		Expect(err).ToNot(HaveOccurred())
 
 		// Generate a repository name which doesn't exist already
-		testOrgRepoName = fmt.Sprintf("test-repo-%03d", rand.Intn(1000))
+		testOrgRepoName = fmt.Sprintf("test-org-repo-%03d", rand.Intn(1000))
 		for findOrgRepo(repos, testOrgRepoName) != nil {
-			testOrgRepoName = fmt.Sprintf("test-repo-%03d", rand.Intn(1000))
+			testOrgRepoName = fmt.Sprintf("test-org-repo-%03d", rand.Intn(1000))
 		}
 
 		// We know that a repo with this name doesn't exist in the organization, let's verify we get an
diff --git a/gitea/integration_repositories_user_test.go b/gitea/integration_repositories_user_test.go
@@ -70,7 +70,7 @@ var _ = Describe("Gitea Provider", func() {
 		}
 
 		fmt.Print("Creating repository ", testRepoName, "...")
-		repoRef := newUserRepoRef(giteaUser, testRepoName)
+		repoRef := newUserRepoRef(testRepoName)
 
 		// Check that the repository doesn't exist
 		_, err = c.UserRepositories().Get(ctx, repoRef)
@@ -100,8 +100,21 @@ var _ = Describe("Gitea Provider", func() {
 		Expect(getSpec.Equals(postSpec)).To(BeTrue())
 	})
 
+	It("should return correct repo info when creating a repository with wrong UserLogin", func() {
+		repoName := fmt.Sprintf("test-user-repo-creation-%03d", rand.Intn(1000))
+		repoRef := newUserRepoRef(repoName)
+		repoRef.UserLogin = "yadda-yadda-yada"
+
+		repo, err := c.UserRepositories().Create(ctx, repoRef, gitprovider.RepositoryInfo{})
+
+		Expect(err).To(BeNil())
+		Expect(
+			repo.Repository().GetCloneURL(gitprovider.TransportTypeHTTPS)).
+			To(Equal(fmt.Sprintf("%s/%s/%s.git", giteaBaseUrl, giteaUser, repoName)))
+	})
+
 	It("should error at creation time if the repo already does exist", func() {
-		repoRef := newUserRepoRef(giteaUser, testRepoName)
+		repoRef := newUserRepoRef(testRepoName)
 		_, err := c.UserRepositories().Get(ctx, repoRef)
 		Expect(err).ToNot(HaveOccurred())
 
@@ -110,7 +123,7 @@ var _ = Describe("Gitea Provider", func() {
 	})
 
 	It("should update if the repository already exists when reconciling", func() {
-		repoRef := newUserRepoRef(giteaUser, testRepoName)
+		repoRef := newUserRepoRef(testRepoName)
 		// No-op reconcile
 		resp, actionTaken, err := c.UserRepositories().Reconcile(ctx, repoRef, gitprovider.RepositoryInfo{
 			Description:   gitprovider.StringVar(defaultDescription),
@@ -189,7 +202,7 @@ var _ = Describe("Gitea Provider", func() {
 
 	It("should be possible to create a pr for a user repository", func() {
 		testRepoName = fmt.Sprintf("test-user-repo2-%03d", rand.Intn(1000))
-		repoRef := newUserRepoRef(giteaUser, testRepoName)
+		repoRef := newUserRepoRef(testRepoName)
 		description := "test description"
 		// Create a new repo
 		userRepo, err := c.UserRepositories().Create(ctx, repoRef,
@@ -281,7 +294,7 @@ var _ = Describe("Gitea Provider", func() {
 
 	It("should be possible to download files from path and branch specified", func() {
 		testRepoName = fmt.Sprintf("test-repo-tree-%03d", rand.Intn(1000))
-		userRepoRef := newUserRepoRef(giteaUser, testRepoName)
+		userRepoRef := newUserRepoRef(testRepoName)
 		repo, err := c.UserRepositories().Create(ctx, userRepoRef, gitprovider.RepositoryInfo{
 			DefaultBranch: gitprovider.StringVar(defaultBranch),
 			Description:   gitprovider.StringVar(defaultDescription),
diff --git a/gitea/integration_suite_test.go b/gitea/integration_suite_test.go
@@ -243,9 +243,9 @@ func newUserRef(userLogin string) gitprovider.UserRef {
 	}
 }
 
-func newUserRepoRef(userLogin, repoName string) gitprovider.UserRepositoryRef {
+func newUserRepoRef(repoName string) gitprovider.UserRepositoryRef {
 	return gitprovider.UserRepositoryRef{
-		UserRef:        newUserRef(userLogin),
+		UserRef:        newUserRef(giteaUser),
 		RepositoryName: repoName,
 	}
 }
diff --git a/gitea/resource_repository.go b/gitea/resource_repository.go
@@ -164,7 +164,7 @@ func (r *userRepository) Update(ctx context.Context) error {
 		Archived:                  &r.r.Archived,
 		DefaultMergeStyle:         &r.r.DefaultMergeStyle,
 	}
-	if r.r.Mirror == true {
+	if r.r.Mirror {
 		opts.MirrorInterval = &r.r.MirrorInterval
 	}
 	apiObj, err := updateRepo(r.c, r.ref.GetIdentity(), r.ref.GetRepository(), &opts)
@@ -261,7 +261,7 @@ func validateRepositoryAPI(apiObj *gitea.Repository) error {
 			validator.Required("Name")
 		}
 		// Make sure visibility is valid if set
-		if apiObj.Private != true {
+		if !apiObj.Private {
 			v := gitprovider.RepositoryVisibility("public")
 			validator.Append(gitprovider.ValidateRepositoryVisibility(v), v, "Visibility")
 		} else {
@@ -276,7 +276,7 @@ func repositoryFromAPI(apiObj *gitea.Repository) gitprovider.RepositoryInfo {
 		Description:   &apiObj.Description,
 		DefaultBranch: &apiObj.DefaultBranch,
 	}
-	if apiObj.Private != true {
+	if !apiObj.Private {
 		repo.Visibility = gitprovider.RepositoryVisibilityVar(gitprovider.RepositoryVisibility("public"))
 	} else {
 		repo.Visibility = gitprovider.RepositoryVisibilityVar(gitprovider.RepositoryVisibility("private"))
diff --git a/gitea/util.go b/gitea/util.go
@@ -26,13 +26,6 @@ import (
 	"github.com/fluxcd/go-git-providers/validation"
 )
 
-const (
-	alreadyExistsMagicString = "name already exists on this account"
-)
-
-// TODO: Guard better against nil pointer dereference panics in this package, also
-// validate data coming from the server
-
 // validateUserRepositoryRef makes sure the UserRepositoryRef is valid for Gitea's usage.
 func validateUserRepositoryRef(ref gitprovider.UserRepositoryRef, expectedDomain string) error {
 	// Make sure the RepositoryRef fields are valid
diff --git a/github/client_repositories_user.go b/github/client_repositories_user.go
@@ -19,6 +19,7 @@ package github
 import (
 	"context"
 	"errors"
+	"fmt"
 
 	"github.com/fluxcd/go-git-providers/gitprovider"
 )
@@ -91,6 +92,13 @@ func (c *UserRepositoriesClient) Create(ctx context.Context,
 	if err != nil {
 		return nil, err
 	}
+
+	owner := apiObj.GetOwner()
+	if owner == nil {
+		return nil, fmt.Errorf("returned API object doesn't have an owner")
+	}
+	ref.UserLogin = *owner.Login
+
 	return newUserRepository(c.clientContext, apiObj, ref), nil
 }
 
diff --git a/github/integration_test.go b/github/integration_test.go
@@ -345,6 +345,18 @@ var _ = Describe("GitHub Provider", func() {
 		Expect(getSpec.Equals(postSpec)).To(BeTrue())
 	})
 
+	It("should return correct repo info when creating a repository with wrong UserLogin", func() {
+		repoName := fmt.Sprintf("test-user-repo-creation-%03d", rand.Intn(1000))
+		repoRef := newUserRepoRef("yadda-yadda-yada", repoName)
+
+		repo, err := c.UserRepositories().Create(ctx, repoRef, gitprovider.RepositoryInfo{})
+
+		Expect(err).To(BeNil())
+		Expect(
+			repo.Repository().GetCloneURL(gitprovider.TransportTypeHTTPS)).
+			To(Equal(fmt.Sprintf("https://%s/%s/%s.git", githubDomain, testUser, repoName)))
+	})
+
 	It("should error at creation time if the repo already does exist", func() {
 		repoRef := newOrgRepoRef(testOrgName, testOrgRepoName)
 		_, err := c.OrgRepositories().Create(ctx, repoRef, gitprovider.RepositoryInfo{})
diff --git a/gitlab/client_repositories_user.go b/gitlab/client_repositories_user.go
@@ -19,6 +19,7 @@ package gitlab
 import (
 	"context"
 	"errors"
+	"fmt"
 
 	"github.com/fluxcd/go-git-providers/gitprovider"
 )
@@ -82,6 +83,7 @@ func (c *UserRepositoriesClient) Create(ctx context.Context,
 	req gitprovider.RepositoryInfo,
 	opts ...gitprovider.RepositoryCreateOption,
 ) (gitprovider.UserRepository, error) {
+
 	// Make sure the RepositoryRef is valid
 	if err := validateUserRepositoryRef(ref, c.domain); err != nil {
 		return nil, err
@@ -91,6 +93,12 @@ func (c *UserRepositoriesClient) Create(ctx context.Context,
 	if err != nil {
 		return nil, err
 	}
+
+	if apiObj.Owner == nil {
+		return nil, fmt.Errorf("returned API object doesn't have an owner")
+	}
+	ref.UserLogin = apiObj.Owner.Username
+
 	return newUserProject(c.clientContext, apiObj, ref), nil
 }
 
diff --git a/gitlab/integration_test.go b/gitlab/integration_test.go
@@ -796,6 +796,18 @@ var _ = Describe("GitLab Provider", func() {
 		Expect(errors.Is(err, gitprovider.ErrAlreadyExists)).To(BeTrue())
 	})
 
+	It("should return correct repo info when creating a repository with wrong UserLogin", func() {
+		repoName := fmt.Sprintf("test-user-repo-creation-%03d", rand.Intn(1000))
+		repoRef := newUserRepoRef(testBaseUrl, "yadda-yadda-yada", repoName)
+
+		repo, err := c.UserRepositories().Create(ctx, repoRef, gitprovider.RepositoryInfo{})
+
+		Expect(err).To(BeNil())
+		Expect(
+			repo.Repository().GetCloneURL(gitprovider.TransportTypeHTTPS)).
+			To(Equal(fmt.Sprintf("%s/%s/%s.git", testBaseUrl, testUserName, repoName)))
+	})
+
 	It("should update if the user repo already exists when reconciling", func() {
 		repoRef := newUserRepoRef(testBaseUrl, testUserName, testRepoName)
 		// No-op reconcile
diff --git a/stash/client_repositories_user.go b/stash/client_repositories_user.go
@@ -128,7 +128,7 @@ func (c *UserRepositoriesClient) Create(ctx context.Context,
 		return nil, err
 	}
 
-	apiObj, err := createRepository(ctx, c.client, addTilde(ref.UserLogin), ref, req, opts...)
+	apiObj, err := createRepository(ctx, c.client, addTilde(c.client.username), ref, req, opts...)
 	if err != nil {
 		if errors.Is(err, ErrAlreadyExists) {
 			return nil, gitprovider.ErrAlreadyExists
@@ -137,6 +137,7 @@ func (c *UserRepositoriesClient) Create(ctx context.Context,
 	}
 
 	ref.SetSlug(apiObj.Slug)
+	ref.UserLogin = apiObj.UserName
 
 	return newUserRepository(c.clientContext, apiObj, ref), nil
 }
diff --git a/stash/integration_repositories_user_test.go b/stash/integration_repositories_user_test.go
@@ -107,6 +107,18 @@ var _ = Describe("Stash Provider", func() {
 		Expect(getSpec.Equals(postSpec)).To(BeTrue())
 	})
 
+	It("should return correct repo info when creating a repository with wrong UserLogin", func() {
+		repoName := fmt.Sprintf("test-user-repo-creation-%03d", rand.Intn(1000))
+		repoRef := newUserRepoRef("yadda-yadda-yadda", repoName)
+
+		repo, err := client.UserRepositories().Create(ctx, repoRef, gitprovider.RepositoryInfo{})
+
+		Expect(err).To(BeNil(), "repository with different UserLogin failed")
+		Expect(
+			repo.Repository().GetCloneURL(gitprovider.TransportTypeHTTPS)).
+			To(Equal(fmt.Sprintf("https://%s/%s/%s.git", stashDomain, stashUser, repoName)))
+	})
+
 	It("should error at creation time if the user repo already does exist", func() {
 		repoRef := newUserRepoRef(stashUser, testRepoName)
 		_, err := client.UserRepositories().Get(ctx, repoRef)
diff --git a/stash/repositories.go b/stash/repositories.go
@@ -229,7 +229,7 @@ func (s *RepositoriesService) Create(ctx context.Context, projectKey string, rep
 		return nil, fmt.Errorf("create respository failed: %w", err)
 	}
 
-	if resp != nil && resp.StatusCode == http.StatusBadRequest {
+	if resp != nil && resp.StatusCode != http.StatusCreated && resp.StatusCode != http.StatusOK {
 		return nil, fmt.Errorf("create repository failed: %s", resp.Status)
 	}
 

Original file line number	Diff line number	Diff line change
`@@ -77,7 +77,7 @@ func (c *OrgRepositoriesClient) List(ctx context.Context, ref gitprovider.Organi`
`77`	`77`	`// apiObj is already validated at ListOrgRepos`
`78`	`78`	`repos = append(repos, newOrgRepository(c.clientContext, apiObj, gitprovider.OrgRepositoryRef{`
`79`	`79`	`OrganizationRef: ref,`
`80`		`- RepositoryName: *&apiObj.Name,`
	`80`	`+ RepositoryName: apiObj.Name,`
`81`	`81`	`}))`
`82`	`82`	`}`
`83`	`83`	`return repos, nil`
Original file line number	Diff line number	Diff line change
`@@ -24,9 +24,6 @@ import (`
`24`	`24`	`"github.com/fluxcd/go-git-providers/gitprovider"`
`25`	`25`	`)`
`26`	`26`
`27`		`-var giteaNewFileMode = "100644"`
`28`		`-var giteaBlobTypeFile = "blob"`
`29`		`-`
`30`	`27`	`// CommitClient implements the gitprovider.CommitClient interface.`
`31`	`28`	`var _ gitprovider.CommitClient = &CommitClient{}`
`32`	`29`
Original file line number	Diff line number	Diff line change
`@@ -243,9 +243,9 @@ func newUserRef(userLogin string) gitprovider.UserRef {`
`243`	`243`	`}`
`244`	`244`	`}`
`245`	`245`
`246`		`-func newUserRepoRef(userLogin, repoName string) gitprovider.UserRepositoryRef {`
	`246`	`+func newUserRepoRef(repoName string) gitprovider.UserRepositoryRef {`
`247`	`247`	`return gitprovider.UserRepositoryRef{`
`248`		`- UserRef: newUserRef(userLogin),`
	`248`	`+ UserRef: newUserRef(giteaUser),`
`249`	`249`	`RepositoryName: repoName,`
`250`	`250`	`}`
`251`	`251`	`}`
Original file line number	Diff line number	Diff line change
`@@ -19,6 +19,7 @@ package github`
`19`	`19`	`import (`
`20`	`20`	`"context"`
`21`	`21`	`"errors"`
	`22`	`+ "fmt"`
`22`	`23`
`23`	`24`	`"github.com/fluxcd/go-git-providers/gitprovider"`
`24`	`25`	`)`
`@@ -91,6 +92,13 @@ func (c *UserRepositoriesClient) Create(ctx context.Context,`
`91`	`92`	`if err != nil {`
`92`	`93`	`return nil, err`
`93`	`94`	`}`
	`95`	`+`
	`96`	`+ owner := apiObj.GetOwner()`
	`97`	`+ if owner == nil {`
	`98`	`+ return nil, fmt.Errorf("returned API object doesn't have an owner")`
	`99`	`+ }`
	`100`	`+ ref.UserLogin = *owner.Login`
	`101`	`+`
`94`	`102`	`return newUserRepository(c.clientContext, apiObj, ref), nil`
`95`	`103`	`}`
`96`	`104`