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.
This commit is contained in:
parent
199acad1bf
commit
19f6a95964
8 changed files with 94 additions and 74 deletions
10
.github/workflows/go.yml
vendored
10
.github/workflows/go.yml
vendored
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -20,6 +20,9 @@ tasks:
|
|||
- ./borg
|
||||
deps:
|
||||
- build
|
||||
test:
|
||||
cmds:
|
||||
- go test ./...
|
||||
test-e2e:
|
||||
cmds:
|
||||
- task: build
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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() {
|
||||
|
|
|
|||
2
go.mod
2
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
|
||||
|
|
|
|||
12
main.go
12
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)
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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))
|
||||
func GetPublicRepos(ctx context.Context, userOrOrg string) ([]string, error) {
|
||||
return GetPublicReposWithAPIURL(ctx, "https://api.github.com", 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
|
||||
}
|
||||
req.Header.Set("User-Agent", "Borg-Data-Collector")
|
||||
resp, err := http.DefaultClient.Do(req)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
defer resp.Body.Close()
|
||||
|
||||
if resp.StatusCode != http.StatusOK {
|
||||
resp.Body.Close()
|
||||
// Try organization endpoint
|
||||
resp, err = http.Get(fmt.Sprintf("https://api.github.com/orgs/%s/repos", userOrOrg))
|
||||
url = fmt.Sprintf("%s/orgs/%s/repos", apiURL, userOrOrg)
|
||||
req, err = http.NewRequestWithContext(ctx, "GET", url, nil)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
defer resp.Body.Close()
|
||||
if resp.StatusCode != http.StatusOK {
|
||||
return nil, fmt.Errorf("failed to fetch repos: %s", resp.Status)
|
||||
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()
|
||||
|
||||
var cloneURLs []string
|
||||
for _, repo := range repos {
|
||||
cloneURLs = append(cloneURLs, repo.CloneURL)
|
||||
allCloneURLs = append(allCloneURLs, repo.CloneURL)
|
||||
}
|
||||
|
||||
return cloneURLs, nil
|
||||
linkHeader := resp.Header.Get("Link")
|
||||
if linkHeader == "" {
|
||||
break
|
||||
}
|
||||
nextURL := findNextURL(linkHeader)
|
||||
if nextURL == "" {
|
||||
break
|
||||
}
|
||||
url = nextURL
|
||||
}
|
||||
|
||||
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 ""
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue