feat: Bug fixes and refactoring

This commit introduces a significant number of bug fixes, refactorings, and improvements to the codebase.

Key changes include:
- Refactored Cobra commands to use `RunE` for better error handling and testing.
- Extracted business logic from command handlers into separate, testable functions.
- Added comprehensive unit tests for the `cmd`, `compress`, `github`, `logger`, and `pwa` packages.
- Added tests for missing command-line arguments, as requested.
- Implemented the `borg all` command to clone all public repositories for a GitHub user or organization.
- Restored and improved the `collect pwa` functionality.
- Removed duplicate code and fixed various bugs.
- Addressed a resource leak in the `all` command.
- Improved error handling in the `pwa` package.
- Refactored `main.go` to remove duplicated logic.
- Fixed several other minor bugs and inconsistencies.
- Made tests platform-independent by removing hardcoded `/dev/null` paths.
- Fixed potential panics in tests by adding `nil` checks for errors.
- Fixed test state leakage by using `t.Cleanup` to restore mocked package-level variables.
- Added validation for command-line flags.
- Ensured output directories are created before writing files.
- Fixed naming conventions for Cobra command constructors.
- Consolidated tests for `ParseRepoFromURL`.
- Improved test failure messages.
- Added validation for release tags.
- Aggregated errors during asset downloads.

cmd/collect_github_release_subcommand.go

@@ -14,39 +14,39 @@ import (
 	"golang.org/x/mod/semver"
 )
 
-// collectGithubReleaseCmd represents the collect github-release command
+func NewCollectGithubReleaseCmd() *cobra.Command {
-var collectGithubReleaseCmd = &cobra.Command{
+	cmd := &cobra.Command{
-	Use:   "release [repository-url]",
+		Use:   "release [repository-url]",
-	Short: "Download the latest release of a file from GitHub releases",
+		Short: "Download the latest release of a file from GitHub releases",
-	Long:  `Download the latest release of a file from GitHub releases. If the file or URL has a version number, it will check for a higher version and download it if found.`,
+		Long:  `Download the latest release of a file from GitHub releases. If the file or URL has a version number, it will check for a higher version and download it if found.`,
-	Args:  cobra.ExactArgs(1),
+		Args:  cobra.ExactArgs(1),
-	RunE: func(cmd *cobra.Command, args []string) error {
+		RunE: func(cmd *cobra.Command, args []string) error {
-		logVal := cmd.Context().Value("logger")
+			logVal := cmd.Context().Value("logger")
-		log, ok := logVal.(*slog.Logger)
+			log, ok := logVal.(*slog.Logger)
-		if !ok || log == nil {
+			if !ok || log == nil {
-			return errors.New("logger not properly initialised")
+				return errors.New("logger not properly initialised")
-		}
+			}
-		repoURL := args[0]
+			repoURL := args[0]
-		outputDir, _ := cmd.Flags().GetString("output")
+			outputDir, _ := cmd.Flags().GetString("output")
-		pack, _ := cmd.Flags().GetBool("pack")
+			pack, _ := cmd.Flags().GetBool("pack")
-		file, _ := cmd.Flags().GetString("file")
+			file, _ := cmd.Flags().GetString("file")
-		version, _ := cmd.Flags().GetString("version")
+			version, _ := cmd.Flags().GetString("version")
 
-		_, err := GetRelease(log, repoURL, outputDir, pack, file, version)
+			_, err := GetRelease(log, repoURL, outputDir, pack, file, version)
-		return err
+			return err
-	},
+		},
+	}
+	cmd.PersistentFlags().String("output", ".", "Output directory for the downloaded file")
+	cmd.PersistentFlags().Bool("pack", false, "Pack all assets into a DataNode")
+	cmd.PersistentFlags().String("file", "", "The file to download from the release")
+	cmd.PersistentFlags().String("version", "", "The version to check against")
+	return cmd
 }
 
 func init() {
-	collectGithubCmd.AddCommand(collectGithubReleaseCmd)
+	collectGithubCmd.AddCommand(NewCollectGithubReleaseCmd())
-	collectGithubReleaseCmd.PersistentFlags().String("output", ".", "Output directory for the downloaded file")
-	collectGithubReleaseCmd.PersistentFlags().Bool("pack", false, "Pack all assets into a DataNode")
-	collectGithubReleaseCmd.PersistentFlags().String("file", "", "The file to download from the release")
-	collectGithubReleaseCmd.PersistentFlags().String("version", "", "The version to check against")
-}
-func NewCollectGithubReleaseCmd() *cobra.Command {
-	return collectGithubReleaseCmd
 }
+
 func GetRelease(log *slog.Logger, repoURL string, outputDir string, pack bool, file string, version string) (*github.RepositoryRelease, error) {
 	owner, repo, err := borg_github.ParseRepoFromURL(repoURL)
 	if err != nil {
@@ -61,26 +61,37 @@ func GetRelease(log *slog.Logger, repoURL string, outputDir string, pack bool, f
 	log.Info("found latest release", "tag", release.GetTagName())
 
 	if version != "" {
-		if !semver.IsValid(version) {
+		tag := release.GetTagName()
-			return nil, fmt.Errorf("invalid version string: %s", version)
+		if !semver.IsValid(tag) {
-		}
+			log.Info("latest release tag is not a valid semantic version, skipping comparison", "tag", tag)
-		if semver.Compare(release.GetTagName(), version) <= 0 {
+		} else {
-			log.Info("latest release is not newer than the provided version", "latest", release.GetTagName(), "provided", version)
+			if !semver.IsValid(version) {
-			return nil, nil
+				return nil, fmt.Errorf("invalid version string: %s", version)
+			}
+			if semver.Compare(tag, version) <= 0 {
+				log.Info("latest release is not newer than the provided version", "latest", tag, "provided", version)
+				return nil, nil
+			}
 		}
 	}
 
 	if pack {
 		dn := datanode.New()
+		var failedAssets []string
 		for _, asset := range release.Assets {
 			log.Info("downloading asset", "name", asset.GetName())
 			data, err := borg_github.DownloadReleaseAsset(asset)
 			if err != nil {
 				log.Error("failed to download asset", "name", asset.GetName(), "err", err)
+				failedAssets = append(failedAssets, asset.GetName())
 				continue
 			}
 			dn.AddData(asset.GetName(), data)
 		}
+		if len(failedAssets) > 0 {
+			return nil, fmt.Errorf("failed to download assets: %v", failedAssets)
+		}
+
 		tar, err := dn.ToTar()
 		if err != nil {
 			return nil, fmt.Errorf("failed to create datanode: %w", err)

pkg/github/release_test.go

@@ -32,6 +32,7 @@ func TestParseRepoFromURL(t *testing.T) {
 		{"git@github.com:owner/repo.git", "owner", "repo", false},
 		{"https://github.com/owner/repo/tree/main", "", "", true},
 		{"invalid-url", "", "", true},
+		{"git:github.com:owner/repo.git", "owner", "repo", false},
 	}
 
 	for _, tc := range testCases {
@@ -142,7 +143,7 @@ func TestDownloadReleaseAsset_NewRequestError(t *testing.T) {
 
 	_, err := DownloadReleaseAsset(asset)
 	if err == nil {
-		t.Fatalf("DownloadReleaseAsset failed: %v", err)
+		t.Fatalf("expected error but got nil")
 	}
 }
 
@@ -195,23 +196,3 @@ func TestDownloadReleaseAsset_DoError(t *testing.T) {
 		t.Fatalf("DownloadReleaseAsset should have failed")
 	}
 }
-func TestParseRepoFromURL_More(t *testing.T) {
-	testCases := []struct {
-		url       string
-		owner     string
-		repo      string
-		expectErr bool
-	}{
-		{"git:github.com:owner/repo.git", "owner", "repo", false},
-	}
-
-	for _, tc := range testCases {
-		owner, repo, err := ParseRepoFromURL(tc.url)
-		if (err != nil) != tc.expectErr {
-			t.Errorf("unexpected error for URL %s: %v", tc.url, err)
-		}
-		if owner != tc.owner || repo != tc.repo {
-			t.Errorf("unexpected owner/repo for URL %s: %s/%s", tc.url, owner, repo)
-		}
-	}
-}