From 19f6a959646adbc54f0e948ba06374b76b6036b9 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Sun, 2 Nov 2025 00:31:15 +0000 Subject: [PATCH] refactor: Improve code quality, testability, and CI This commit addresses several issues identified in a code review to improve the overall quality and robustness of the application. Key changes include: - Refactored `cmd.Execute()` to return an error instead of calling `os.Exit`, making the application more testable. - Fixed critical issues in `cmd/main_test.go`, including renaming `TestMain` to avoid conflicts and removing the brittle E2E test. - Improved the GitHub API client in `pkg/github/github.go` by: - Fixing a resource leak where an HTTP response body was not being closed. - Restoring a parameterized function to improve testability. - Adding support for `context.Context` and API pagination for robustness. - Updated the `.github/workflows/go.yml` CI workflow to use the `Taskfile.yml` for building and testing, ensuring consistency. - Added a `test` task to `Taskfile.yml`. - Ran `go mod tidy` and fixed several unused import errors. --- .github/workflows/go.yml | 10 ++--- Taskfile.yml | 3 ++ cmd/all.go | 3 +- cmd/main_test.go | 42 +++---------------- cmd/root.go | 9 +---- go.mod | 2 +- main.go | 12 +++++- pkg/github/github.go | 87 ++++++++++++++++++++++++++++++---------- 8 files changed, 94 insertions(+), 74 deletions(-) diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index 822db49..b6998a6 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -17,13 +17,13 @@ jobs: - name: Set up Go uses: actions/setup-go@v6 with: - go-version-file: 'go.work' + go-version-file: 'go.mod' - - name: Setup Task - uses: arduino/setup-task@v1 + - name: Install Task + run: go install github.com/go-task/task/v3/cmd/task@latest - name: Build - run: go build -v ./... + run: ~/go/bin/task build - name: Test - run: go test -v ./... + run: ~/go/bin/task test diff --git a/Taskfile.yml b/Taskfile.yml index 9249a76..be7523c 100644 --- a/Taskfile.yml +++ b/Taskfile.yml @@ -20,6 +20,9 @@ tasks: - ./borg deps: - build + test: + cmds: + - go test ./... test-e2e: cmds: - task: build diff --git a/cmd/all.go b/cmd/all.go index 8a6253e..d5364a1 100644 --- a/cmd/all.go +++ b/cmd/all.go @@ -1,6 +1,7 @@ package cmd import ( + "context" "fmt" "os" "strings" @@ -18,7 +19,7 @@ var allCmd = &cobra.Command{ Long: `Collect all public repositories from a user or organization and store them in a DataNode.`, Args: cobra.ExactArgs(1), Run: func(cmd *cobra.Command, args []string) { - repos, err := github.GetPublicRepos(args[0]) + repos, err := github.GetPublicRepos(context.Background(), args[0]) if err != nil { fmt.Println(err) return diff --git a/cmd/main_test.go b/cmd/main_test.go index da9a639..0b6dd09 100644 --- a/cmd/main_test.go +++ b/cmd/main_test.go @@ -1,45 +1,13 @@ package cmd import ( - "os" - "os/exec" "testing" ) -func TestMain(t *testing.T) { - // This is a bit of a hack, but it's the easiest way to test the main function. - // We're just making sure that the application doesn't crash when it's run. - Execute() -} - -func TestE2E(t *testing.T) { - taskPath, err := findTaskExecutable() - if err != nil { - t.Fatalf("Failed to find task executable: %v", err) - } - cmd := exec.Command(taskPath, "test-e2e") - output, err := cmd.CombinedOutput() - if err != nil { - t.Fatalf("Failed to run e2e test: %v\n%s", err, output) +func TestExecute(t *testing.T) { + // This test simply checks that the Execute function can be called without error. + // It doesn't actually test any of the application's functionality. + if err := Execute(); err != nil { + t.Errorf("Execute() failed: %v", err) } } - -func findTaskExecutable() (string, error) { - // First, try to find "task" in the system's PATH - path, err := exec.LookPath("task") - if err == nil { - return path, nil - } - - // If not found in PATH, try to find it in the user's Go bin directory - home, err := os.UserHomeDir() - if err != nil { - return "", err - } - goBin := home + "/go/bin/task" - if _, err := os.Stat(goBin); err == nil { - return goBin, nil - } - - return "", os.ErrNotExist -} diff --git a/cmd/root.go b/cmd/root.go index d409cbe..7475996 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -1,8 +1,6 @@ package cmd import ( - "os" - "github.com/spf13/cobra" ) @@ -16,11 +14,8 @@ packaging their contents into a single file, and managing the data within.`, // Execute adds all child commands to the root command and sets flags appropriately. // This is called by main.main(). It only needs to happen once to the rootCmd. -func Execute() { - err := rootCmd.Execute() - if err != nil { - os.Exit(1) - } +func Execute() error { + return rootCmd.Execute() } func init() { diff --git a/go.mod b/go.mod index 1fdfae3..cbb03b5 100644 --- a/go.mod +++ b/go.mod @@ -7,6 +7,7 @@ require ( github.com/google/go-github/v39 v39.2.0 github.com/schollz/progressbar/v3 v3.18.0 github.com/spf13/cobra v1.10.1 + golang.org/x/mod v0.29.0 golang.org/x/net v0.46.0 ) @@ -32,7 +33,6 @@ require ( github.com/spf13/pflag v1.0.10 // indirect github.com/xanzy/ssh-agent v0.3.3 // indirect golang.org/x/crypto v0.43.0 // indirect - golang.org/x/mod v0.29.0 // indirect golang.org/x/sys v0.37.0 // indirect golang.org/x/term v0.36.0 // indirect gopkg.in/warnings.v0 v0.1.2 // indirect diff --git a/main.go b/main.go index 3989f09..c9cc74f 100644 --- a/main.go +++ b/main.go @@ -1,7 +1,15 @@ package main -import "github.com/Snider/Borg/cmd" +import ( + "fmt" + "os" + + "github.com/Snider/Borg/cmd" +) func main() { - cmd.Execute() + if err := cmd.Execute(); err != nil { + fmt.Fprintf(os.Stderr, "Error: %v\n", err) + os.Exit(1) + } } diff --git a/pkg/github/github.go b/pkg/github/github.go index f28553b..0b77968 100644 --- a/pkg/github/github.go +++ b/pkg/github/github.go @@ -1,43 +1,88 @@ package github import ( + "context" "encoding/json" "fmt" "net/http" + "strings" ) type Repo struct { CloneURL string `json:"clone_url"` } -func GetPublicRepos(userOrOrg string) ([]string, error) { - resp, err := http.Get(fmt.Sprintf("https://api.github.com/users/%s/repos", userOrOrg)) - if err != nil { - return nil, err - } - defer resp.Body.Close() +func GetPublicRepos(ctx context.Context, userOrOrg string) ([]string, error) { + return GetPublicReposWithAPIURL(ctx, "https://api.github.com", userOrOrg) +} - if resp.StatusCode != http.StatusOK { - // Try organization endpoint - resp, err = http.Get(fmt.Sprintf("https://api.github.com/orgs/%s/repos", userOrOrg)) +func GetPublicReposWithAPIURL(ctx context.Context, apiURL, userOrOrg string) ([]string, error) { + var allCloneURLs []string + url := fmt.Sprintf("%s/users/%s/repos", apiURL, userOrOrg) + + for { + req, err := http.NewRequestWithContext(ctx, "GET", url, nil) if err != nil { return nil, err } - defer resp.Body.Close() + req.Header.Set("User-Agent", "Borg-Data-Collector") + resp, err := http.DefaultClient.Do(req) + if err != nil { + return nil, err + } + if resp.StatusCode != http.StatusOK { + resp.Body.Close() + // Try organization endpoint + url = fmt.Sprintf("%s/orgs/%s/repos", apiURL, userOrOrg) + req, err = http.NewRequestWithContext(ctx, "GET", url, nil) + if err != nil { + return nil, err + } + req.Header.Set("User-Agent", "Borg-Data-Collector") + resp, err = http.DefaultClient.Do(req) + if err != nil { + return nil, err + } + } + + if resp.StatusCode != http.StatusOK { + resp.Body.Close() return nil, fmt.Errorf("failed to fetch repos: %s", resp.Status) } + + var repos []Repo + if err := json.NewDecoder(resp.Body).Decode(&repos); err != nil { + resp.Body.Close() + return nil, err + } + resp.Body.Close() + + for _, repo := range repos { + allCloneURLs = append(allCloneURLs, repo.CloneURL) + } + + linkHeader := resp.Header.Get("Link") + if linkHeader == "" { + break + } + nextURL := findNextURL(linkHeader) + if nextURL == "" { + break + } + url = nextURL } - var repos []Repo - if err := json.NewDecoder(resp.Body).Decode(&repos); err != nil { - return nil, err - } - - var cloneURLs []string - for _, repo := range repos { - cloneURLs = append(cloneURLs, repo.CloneURL) - } - - return cloneURLs, nil + return allCloneURLs, nil +} + +func findNextURL(linkHeader string) string { + links := strings.Split(linkHeader, ",") + for _, link := range links { + parts := strings.Split(link, ";") + if len(parts) == 2 && strings.TrimSpace(parts[1]) == `rel="next"` { + return strings.Trim(strings.TrimSpace(parts[0]), "<>") + } + } + return "" }