Skip to content

Commit fff0686

Browse files
authored
refactor(internal/github): remove dep on gitrepo package (#2393)
The git and github packages should be standalone and not depend on eachother. Instead the thing that uses both of them, librarian, is responsible for converting the types and whatnot. Updates: #2372
1 parent cd28ded commit fff0686

File tree

5 files changed

+160
-144
lines changed

5 files changed

+160
-144
lines changed

internal/github/github.go

Lines changed: 0 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@ import (
2626
"strings"
2727

2828
"github.com/google/go-github/v69/github"
29-
"github.com/googleapis/librarian/internal/gitrepo"
3029
)
3130

3231
// PullRequest is a type alias for the go-github type.
@@ -215,27 +214,6 @@ func (c *Client) AddLabelsToIssue(ctx context.Context, repo *Repository, number
215214
return err
216215
}
217216

218-
// FetchGitHubRepoFromRemote parses the GitHub repo name from the remote for this repository.
219-
// There must be a remote named 'origin' with a GitHub URL (as the first URL), in order to
220-
// provide an unambiguous result.
221-
// Remotes without any URLs, or where the first URL does not start with https://github.com/ are ignored.
222-
func FetchGitHubRepoFromRemote(repo gitrepo.Repository) (*Repository, error) {
223-
remotes, err := repo.Remotes()
224-
if err != nil {
225-
return nil, err
226-
}
227-
228-
for _, remote := range remotes {
229-
if remote.Name == "origin" {
230-
if len(remote.URLs) > 0 {
231-
return ParseRemote(remote.URLs[0])
232-
}
233-
}
234-
}
235-
236-
return nil, fmt.Errorf("could not find an 'origin' remote pointing to a GitHub https URL")
237-
}
238-
239217
// SearchPullRequests searches for pull requests in the repository using the provided raw query.
240218
func (c *Client) SearchPullRequests(ctx context.Context, query string) ([]*PullRequest, error) {
241219
var prs []*PullRequest

internal/github/github_test.go

Lines changed: 0 additions & 119 deletions
Original file line numberDiff line numberDiff line change
@@ -24,11 +24,8 @@ import (
2424
"testing"
2525
"time"
2626

27-
"github.com/go-git/go-git/v5"
28-
gogitConfig "github.com/go-git/go-git/v5/config"
2927
"github.com/google/go-cmp/cmp"
3028
"github.com/google/go-github/v69/github"
31-
"github.com/googleapis/librarian/internal/gitrepo"
3229
)
3330

3431
func TestToken(t *testing.T) {
@@ -122,122 +119,6 @@ func TestGetRawContent(t *testing.T) {
122119
}
123120
}
124121

125-
// newTestGitRepo creates a new git repository in a temporary directory with the given remotes.
126-
func newTestGitRepo(t *testing.T, remotes map[string][]string) *gitrepo.LocalRepository {
127-
t.Helper()
128-
dir := t.TempDir()
129-
130-
r, err := git.PlainInit(dir, false)
131-
if err != nil {
132-
t.Fatalf("git.PlainInit failed: %v", err)
133-
}
134-
135-
for name, urls := range remotes {
136-
_, err := r.CreateRemote(&gogitConfig.RemoteConfig{
137-
Name: name,
138-
URLs: urls,
139-
})
140-
if err != nil {
141-
t.Fatalf("CreateRemote failed: %v", err)
142-
}
143-
}
144-
145-
repo, err := gitrepo.NewRepository(&gitrepo.RepositoryOptions{Dir: dir})
146-
if err != nil {
147-
t.Fatalf("gitrepo.NewRepository failed: %v", err)
148-
}
149-
return repo
150-
}
151-
152-
func TestFetchGitHubRepoFromRemote(t *testing.T) {
153-
t.Parallel()
154-
for _, test := range []struct {
155-
name string
156-
remotes map[string][]string
157-
wantRepo *Repository
158-
wantErr bool
159-
wantErrSubstr string
160-
}{
161-
{
162-
name: "origin is a GitHub remote",
163-
remotes: map[string][]string{
164-
"origin": {"https://github.com/owner/repo.git"},
165-
},
166-
wantRepo: &Repository{Owner: "owner", Name: "repo"},
167-
},
168-
{
169-
name: "No remotes",
170-
remotes: map[string][]string{},
171-
wantErr: true,
172-
wantErrSubstr: "could not find an 'origin' remote",
173-
},
174-
{
175-
name: "origin is not a GitHub remote",
176-
remotes: map[string][]string{
177-
"origin": {"https://gitlab.com/owner/repo.git"},
178-
},
179-
wantErr: true,
180-
wantErrSubstr: "is not a GitHub remote",
181-
},
182-
{
183-
name: "upstream is GitHub, but no origin",
184-
remotes: map[string][]string{
185-
"gitlab": {"https://gitlab.com/owner/repo.git"},
186-
"upstream": {"https://github.com/gh-owner/gh-repo.git"},
187-
},
188-
wantErr: true,
189-
wantErrSubstr: "could not find an 'origin' remote",
190-
},
191-
{
192-
name: "origin and upstream are GitHub remotes, should use origin",
193-
remotes: map[string][]string{
194-
"origin": {"https://github.com/owner/repo.git"},
195-
"upstream": {"https://github.com/owner2/repo2.git"},
196-
},
197-
wantRepo: &Repository{Owner: "owner", Name: "repo"},
198-
},
199-
{
200-
name: "origin is not GitHub, but upstream is",
201-
remotes: map[string][]string{
202-
"origin": {"https://gitlab.com/owner/repo.git"},
203-
"upstream": {"https://github.com/gh-owner/gh-repo.git"},
204-
},
205-
wantErr: true,
206-
wantErrSubstr: "is not a GitHub remote",
207-
},
208-
{
209-
name: "origin has multiple URLs, first is GitHub",
210-
remotes: map[string][]string{
211-
"origin": {"https://github.com/owner/repo.git", "https://gitlab.com/owner/repo.git"},
212-
},
213-
wantRepo: &Repository{Owner: "owner", Name: "repo"},
214-
},
215-
} {
216-
t.Run(test.name, func(t *testing.T) {
217-
t.Parallel()
218-
repo := newTestGitRepo(t, test.remotes)
219-
220-
got, err := FetchGitHubRepoFromRemote(repo)
221-
222-
if test.wantErr {
223-
if err == nil {
224-
t.Fatalf("FetchGitHubRepoFromRemote() err = nil, want error containing %q", test.wantErrSubstr)
225-
}
226-
if !strings.Contains(err.Error(), test.wantErrSubstr) {
227-
t.Errorf("FetchGitHubRepoFromRemote() err = %v, want error containing %q", err, test.wantErrSubstr)
228-
}
229-
} else {
230-
if err != nil {
231-
t.Errorf("FetchGitHubRepoFromRemote() err = %v, want nil", err)
232-
}
233-
if diff := cmp.Diff(test.wantRepo, got); diff != "" {
234-
t.Errorf("FetchGitHubRepoFromRemote() repo mismatch (-want +got): %s", diff)
235-
}
236-
}
237-
})
238-
}
239-
}
240-
241122
func TestParseURL(t *testing.T) {
242123
t.Parallel()
243124
for _, test := range []struct {

internal/librarian/repository.go

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@
2020
package librarian
2121

2222
import (
23+
"fmt"
24+
2325
"github.com/googleapis/librarian/internal/config"
2426
"github.com/googleapis/librarian/internal/github"
2527
"github.com/googleapis/librarian/internal/gitrepo"
@@ -31,11 +33,24 @@ var GetGitHubRepository = func(cfg *config.Config, languageRepo gitrepo.Reposito
3133
if isURL(cfg.Repo) {
3234
return github.ParseRemote(cfg.Repo)
3335
}
34-
return github.FetchGitHubRepoFromRemote(languageRepo)
36+
return GetGitHubRepositoryFromGitRepo(languageRepo)
3537
}
3638

3739
// GetGitHubRepositoryFromGitRepo determines the GitHub repository from the
3840
// local git remote.
3941
var GetGitHubRepositoryFromGitRepo = func(languageRepo gitrepo.Repository) (*github.Repository, error) {
40-
return github.FetchGitHubRepoFromRemote(languageRepo)
42+
remotes, err := languageRepo.Remotes()
43+
if err != nil {
44+
return nil, err
45+
}
46+
47+
for _, remote := range remotes {
48+
if remote.Name == "origin" {
49+
if len(remote.URLs) > 0 {
50+
return github.ParseRemote(remote.URLs[0])
51+
}
52+
}
53+
}
54+
55+
return nil, fmt.Errorf("could not find an 'origin' remote pointing to a GitHub https URL")
4156
}
Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,142 @@
1+
// Copyright 2025 Google LLC
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// https://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package librarian
16+
17+
import (
18+
"strings"
19+
"testing"
20+
21+
"github.com/go-git/go-git/v5"
22+
gogitConfig "github.com/go-git/go-git/v5/config"
23+
"github.com/google/go-cmp/cmp"
24+
"github.com/googleapis/librarian/internal/github"
25+
"github.com/googleapis/librarian/internal/gitrepo"
26+
)
27+
28+
func TestGetGitHubRepositoryFromGitRepo(t *testing.T) {
29+
t.Parallel()
30+
for _, test := range []struct {
31+
name string
32+
remotes map[string][]string
33+
wantRepo *github.Repository
34+
wantErr bool
35+
wantErrSubstr string
36+
}{
37+
{
38+
name: "origin is a GitHub remote",
39+
remotes: map[string][]string{
40+
"origin": {"https://github.com/owner/repo.git"},
41+
},
42+
wantRepo: &github.Repository{Owner: "owner", Name: "repo"},
43+
},
44+
{
45+
name: "No remotes",
46+
remotes: map[string][]string{},
47+
wantErr: true,
48+
wantErrSubstr: "could not find an 'origin' remote",
49+
},
50+
{
51+
name: "origin is not a GitHub remote",
52+
remotes: map[string][]string{
53+
"origin": {"https://gitlab.com/owner/repo.git"},
54+
},
55+
wantErr: true,
56+
wantErrSubstr: "is not a GitHub remote",
57+
},
58+
{
59+
name: "upstream is GitHub, but no origin",
60+
remotes: map[string][]string{
61+
"gitlab": {"https://gitlab.com/owner/repo.git"},
62+
"upstream": {"https://github.com/gh-owner/gh-repo.git"},
63+
},
64+
wantErr: true,
65+
wantErrSubstr: "could not find an 'origin' remote",
66+
},
67+
{
68+
name: "origin and upstream are GitHub remotes, should use origin",
69+
remotes: map[string][]string{
70+
"origin": {"https://github.com/owner/repo.git"},
71+
"upstream": {"https://github.com/owner2/repo2.git"},
72+
},
73+
wantRepo: &github.Repository{Owner: "owner", Name: "repo"},
74+
},
75+
{
76+
name: "origin is not GitHub, but upstream is",
77+
remotes: map[string][]string{
78+
"origin": {"https://gitlab.com/owner/repo.git"},
79+
"upstream": {"https://github.com/gh-owner/gh-repo.git"},
80+
},
81+
wantErr: true,
82+
wantErrSubstr: "is not a GitHub remote",
83+
},
84+
{
85+
name: "origin has multiple URLs, first is GitHub",
86+
remotes: map[string][]string{
87+
"origin": {"https://github.com/owner/repo.git", "https://gitlab.com/owner/repo.git"},
88+
},
89+
wantRepo: &github.Repository{Owner: "owner", Name: "repo"},
90+
},
91+
} {
92+
t.Run(test.name, func(t *testing.T) {
93+
t.Parallel()
94+
repo := newTestGitRepoWithRemotes(t, test.remotes)
95+
96+
got, err := GetGitHubRepositoryFromGitRepo(repo)
97+
98+
if test.wantErr {
99+
if err == nil {
100+
t.Fatalf("FetchGitHubRepoFromRemote() err = nil, want error containing %q", test.wantErrSubstr)
101+
}
102+
if !strings.Contains(err.Error(), test.wantErrSubstr) {
103+
t.Errorf("FetchGitHubRepoFromRemote() err = %v, want error containing %q", err, test.wantErrSubstr)
104+
}
105+
} else {
106+
if err != nil {
107+
t.Errorf("FetchGitHubRepoFromRemote() err = %v, want nil", err)
108+
}
109+
if diff := cmp.Diff(test.wantRepo, got); diff != "" {
110+
t.Errorf("FetchGitHubRepoFromRemote() repo mismatch (-want +got): %s", diff)
111+
}
112+
}
113+
})
114+
}
115+
}
116+
117+
// newTestGitRepo creates a new git repository in a temporary directory with the given remotes.
118+
func newTestGitRepoWithRemotes(t *testing.T, remotes map[string][]string) *gitrepo.LocalRepository {
119+
t.Helper()
120+
dir := t.TempDir()
121+
122+
r, err := git.PlainInit(dir, false)
123+
if err != nil {
124+
t.Fatalf("git.PlainInit failed: %v", err)
125+
}
126+
127+
for name, urls := range remotes {
128+
_, err := r.CreateRemote(&gogitConfig.RemoteConfig{
129+
Name: name,
130+
URLs: urls,
131+
})
132+
if err != nil {
133+
t.Fatalf("CreateRemote failed: %v", err)
134+
}
135+
}
136+
137+
repo, err := gitrepo.NewRepository(&gitrepo.RepositoryOptions{Dir: dir})
138+
if err != nil {
139+
t.Fatalf("gitrepo.NewRepository failed: %v", err)
140+
}
141+
return repo
142+
}

internal/librarian/tag_and_release.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ func parseRemote(repo string) (*github.Repository, error) {
8888
if err != nil {
8989
return nil, err
9090
}
91-
return github.FetchGitHubRepoFromRemote(githubRepo)
91+
return GetGitHubRepositoryFromGitRepo(githubRepo)
9292
}
9393

9494
func (r *tagAndReleaseRunner) run(ctx context.Context) error {

0 commit comments

Comments
 (0)