From ede5882a15492c39cefdc6a8fe3f349f9861913a Mon Sep 17 00:00:00 2001
From: kat <28567881+hk21702@users.noreply.github.com>
Date: Mon, 13 Jan 2025 11:25:34 -0500
Subject: [PATCH] Add package documentation comments for clarity and improve
 error messages in collect command

---
 cmd/exposed-server/daemon/daemon.go  |  1 +
 cmd/exposed-server/main.go           |  1 +
 cmd/ingest-server/daemon/daemon.go   |  1 +
 cmd/ingest-server/main.go            |  1 +
 cmd/insights/commands/collect.go     |  7 +++----
 cmd/insights/commands/root.go        | 10 +++++-----
 cmd/insights/commands/root_test.go   |  3 +--
 cmd/insights/main.go                 |  4 ++--
 cmd/insights/main_test.go            |  1 +
 internal/constants/constants.go      | 18 +++++++++++-------
 internal/constants/constants_test.go | 16 +++++++++-------
 internal/constants/export_test.go    |  2 +-
 internal/testutils/cmd.go            |  4 ++++
 13 files changed, 41 insertions(+), 28 deletions(-)

diff --git a/cmd/exposed-server/daemon/daemon.go b/cmd/exposed-server/daemon/daemon.go
index 9d30720..d0a1614 100644
--- a/cmd/exposed-server/daemon/daemon.go
+++ b/cmd/exposed-server/daemon/daemon.go
@@ -1 +1,2 @@
+// Package daemon is responsible for running the exposed-server in the background.
 package daemon
diff --git a/cmd/exposed-server/main.go b/cmd/exposed-server/main.go
index 06ab7d0..d34e041 100644
--- a/cmd/exposed-server/main.go
+++ b/cmd/exposed-server/main.go
@@ -1 +1,2 @@
+// Package main for the exposed-server.
 package main
diff --git a/cmd/ingest-server/daemon/daemon.go b/cmd/ingest-server/daemon/daemon.go
index 9d30720..9b21ba0 100644
--- a/cmd/ingest-server/daemon/daemon.go
+++ b/cmd/ingest-server/daemon/daemon.go
@@ -1 +1,2 @@
+// Package daemon is responsible for running the ingest-server in the background.
 package daemon
diff --git a/cmd/ingest-server/main.go b/cmd/ingest-server/main.go
index 06ab7d0..b7f21ba 100644
--- a/cmd/ingest-server/main.go
+++ b/cmd/ingest-server/main.go
@@ -1 +1,2 @@
+// Package main for the ingest-server.
 package main
diff --git a/cmd/insights/commands/collect.go b/cmd/insights/commands/collect.go
index 3382738..c9fcc03 100644
--- a/cmd/insights/commands/collect.go
+++ b/cmd/insights/commands/collect.go
@@ -24,7 +24,7 @@ var defaultCollectConfig = collectConfig{
 	extraMetrics: "",
 }
 
-func installCollectCmd(app *App) error {
+func installCollectCmd(app *App) {
 	app.collectConfig = defaultCollectConfig
 
 	collectCmd := &cobra.Command{
@@ -45,11 +45,11 @@ func installCollectCmd(app *App) error {
 
 				fileInfo, err := os.Stat(args[1])
 				if err != nil {
-					return fmt.Errorf("the second argument, SOURCE-METRICS-PATH, should be a valid JSON file. Error: %s", err.Error())
+					return fmt.Errorf("the second argument, source-metrics-path, should be a valid JSON file. Error: %s", err.Error())
 				}
 
 				if fileInfo.IsDir() {
-					return fmt.Errorf("the second argument, SOURCE-METRICS-PATH, should be a valid JSON file, not a directory.")
+					return fmt.Errorf("the second argument, source-metrics-path, should be a valid JSON file, not a directory")
 				}
 			}
 
@@ -73,5 +73,4 @@ func installCollectCmd(app *App) error {
 	collectCmd.Flags().BoolVarP(&app.collectConfig.dryRun, "dry-run", "d", false, "perform a dry-run where a report is collected, but not written to disk")
 
 	app.rootCmd.AddCommand(collectCmd)
-	return nil
 }
diff --git a/cmd/insights/commands/root.go b/cmd/insights/commands/root.go
index b8c9986..45ead25 100644
--- a/cmd/insights/commands/root.go
+++ b/cmd/insights/commands/root.go
@@ -1,3 +1,4 @@
+// Package commands contains the commands for the Ubuntu Insights CLI.
 package commands
 
 import (
@@ -7,8 +8,7 @@ import (
 	"github.com/ubuntu/ubuntu-insights/internal/constants"
 )
 
-const cmdName = "ubuntu-insights"
-
+// App represents the application.
 type App struct {
 	rootCmd *cobra.Command
 
@@ -30,11 +30,11 @@ var defaultRootConfig = rootConfig{
 	InsightsDir: constants.GetDefaultCachePath(),
 }
 
-// Registers commands and returns a new app
+// New registers commands and returns a new App.
 func New() (*App, error) {
 	a := App{}
 	a.rootCmd = &cobra.Command{
-		Use:           "ubuntu-insights [COMMAND]",
+		Use:           constants.CmdName + " [COMMAND]",
 		Short:         "",
 		Long:          "",
 		SilenceErrors: true,
@@ -48,7 +48,7 @@ func New() (*App, error) {
 	}
 
 	err := installRootCmd(&a)
-	err = installCollectCmd(&a)
+	installCollectCmd(&a)
 	installUploadCmd(&a)
 	installConsentCmd(&a)
 
diff --git a/cmd/insights/commands/root_test.go b/cmd/insights/commands/root_test.go
index e8100e5..061a6d0 100644
--- a/cmd/insights/commands/root_test.go
+++ b/cmd/insights/commands/root_test.go
@@ -2,9 +2,8 @@ package commands
 
 import (
 	"context"
-	"testing"
-
 	"log/slog"
+	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/ubuntu/ubuntu-insights/internal/constants"
diff --git a/cmd/insights/main.go b/cmd/insights/main.go
index 9376f1e..59ea311 100644
--- a/cmd/insights/main.go
+++ b/cmd/insights/main.go
@@ -1,9 +1,9 @@
+// Main package for the insights command line tool.
 package main
 
 import (
-	"os"
-
 	"log/slog"
+	"os"
 
 	"github.com/ubuntu/ubuntu-insights/cmd/insights/commands"
 	"github.com/ubuntu/ubuntu-insights/internal/constants"
diff --git a/cmd/insights/main_test.go b/cmd/insights/main_test.go
index cbcd04c..f536967 100644
--- a/cmd/insights/main_test.go
+++ b/cmd/insights/main_test.go
@@ -45,6 +45,7 @@ func TestRun(t *testing.T) {
 
 	for name, tc := range tests {
 		t.Run(name, func(t *testing.T) {
+			t.Parallel()
 			a := testApp{
 				done:            make(chan struct{}),
 				runError:        tc.runError,
diff --git a/internal/constants/constants.go b/internal/constants/constants.go
index 72da79b..be7cf36 100644
--- a/internal/constants/constants.go
+++ b/internal/constants/constants.go
@@ -1,16 +1,20 @@
+// Package constants is responsible for defining the constants used in the application.
+// It also provides utility functions to get the default configuration and cache paths.
 package constants
 
 import (
-	"os"
-
 	"log/slog"
+	"os"
 )
 
 const (
-	// DefaultAppFolder is the name of the default root folder
+	// CmdName is the name of the command line tool.
+	CmdName = "ubuntu-insights"
+
+	// DefaultAppFolder is the name of the default root folder.
 	DefaultAppFolder = "ubuntu-insights"
 
-	// DefaultLogLevel is the default log level selected without any verbosity flags
+	// DefaultLogLevel is the default log level selected without any verbosity flags.
 	DefaultLogLevel = slog.LevelInfo
 )
 
@@ -20,7 +24,7 @@ type options struct {
 
 type option func(*options)
 
-// GetDefaultConfigPath is the default path to the configuration file
+// GetDefaultConfigPath is the default path to the configuration file.
 func GetDefaultConfigPath(opts ...option) string {
 	o := options{baseDir: os.UserCacheDir}
 	for _, opt := range opts {
@@ -30,7 +34,7 @@ func GetDefaultConfigPath(opts ...option) string {
 	return getBaseDir(o.baseDir) + string(os.PathSeparator) + DefaultAppFolder
 }
 
-// GetDefaultCachePath is the default path to the cache directory
+// GetDefaultCachePath is the default path to the cache directory.
 func GetDefaultCachePath(opts ...option) string {
 	o := options{baseDir: os.UserConfigDir}
 	for _, opt := range opts {
@@ -40,7 +44,7 @@ func GetDefaultCachePath(opts ...option) string {
 	return getBaseDir(o.baseDir) + string(os.PathSeparator) + DefaultAppFolder
 }
 
-// getBaseDir is a helper function to handle the case where the baseDir function returns an error, and instead return an empty string
+// getBaseDir is a helper function to handle the case where the baseDir function returns an error, and instead return an empty string.
 func getBaseDir(baseDirFunc func() (string, error)) string {
 	dir, err := baseDirFunc()
 	if err != nil {
diff --git a/internal/constants/constants_test.go b/internal/constants/constants_test.go
index 179d718..694e4af 100644
--- a/internal/constants/constants_test.go
+++ b/internal/constants/constants_test.go
@@ -9,7 +9,8 @@ import (
 	"github.com/ubuntu/ubuntu-insights/internal/constants"
 )
 
-func Test_GetUserConfigDir(t *testing.T) {
+//nolint:dupl //Tests for GetDefaultConfigPath is very similar to GetDefaultCachePath.
+func Test_GetDefaultConfigPath(t *testing.T) {
 	t.Parallel()
 
 	tests := map[string]struct {
@@ -25,13 +26,13 @@ func Test_GetUserConfigDir(t *testing.T) {
 		"os.UserConfigDir error": {
 			want: string(os.PathSeparator) + constants.DefaultAppFolder,
 			mock: func() (string, error) {
-				return "", fmt.Errorf("error")
+				return "", fmt.Errorf("os.UserCacheDir error")
 			},
 		},
 		"os.UserConfigDir error 2": {
 			want: string(os.PathSeparator) + constants.DefaultAppFolder,
 			mock: func() (string, error) {
-				return "abc", fmt.Errorf("error")
+				return "abc", fmt.Errorf("os.UserCacheDir error")
 			},
 		},
 	}
@@ -45,7 +46,8 @@ func Test_GetUserConfigDir(t *testing.T) {
 	}
 }
 
-func Test_userCacheDir(t *testing.T) {
+//nolint:dupl //Tests for GetDefaultConfigPath is very similar to GetDefaultCachePath.
+func Test_GetDefaultCachePath(t *testing.T) {
 	t.Parallel()
 
 	tests := map[string]struct {
@@ -61,13 +63,13 @@ func Test_userCacheDir(t *testing.T) {
 		"os.UserCacheDir error": {
 			want: string(os.PathSeparator) + constants.DefaultAppFolder,
 			mock: func() (string, error) {
-				return "", fmt.Errorf("error")
+				return "", fmt.Errorf("os.UserCacheDir error")
 			},
 		},
-		"os.UserCacheDir error 2": {
+		"os.UserCacheDir error with return": {
 			want: string(os.PathSeparator) + constants.DefaultAppFolder,
 			mock: func() (string, error) {
-				return "abc", fmt.Errorf("error")
+				return "return", fmt.Errorf("os.UserCacheDir error")
 			},
 		},
 	}
diff --git a/internal/constants/export_test.go b/internal/constants/export_test.go
index 4d67430..b5fb71e 100644
--- a/internal/constants/export_test.go
+++ b/internal/constants/export_test.go
@@ -2,7 +2,7 @@ package constants
 
 type Option = option
 
-func WithBaseDir(baseDir func() (string, error)) option {
+func WithBaseDir(baseDir func() (string, error)) func(o *options) {
 	return func(o *options) {
 		o.baseDir = baseDir
 	}
diff --git a/internal/testutils/cmd.go b/internal/testutils/cmd.go
index 6593071..1d4f8bd 100644
--- a/internal/testutils/cmd.go
+++ b/internal/testutils/cmd.go
@@ -1,3 +1,4 @@
+// Package testutils provides helper functions for testing
 package testutils
 
 import (
@@ -8,6 +9,7 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
+// CmdTestCase is a test case for testing cobra CMD flags.
 type CmdTestCase struct {
 	Name           string
 	Short          string
@@ -17,7 +19,9 @@ type CmdTestCase struct {
 	BaseCmd        *cobra.Command
 }
 
+// FlagTestHelper is a helper function to test cobra CMD flags.
 func FlagTestHelper(t *testing.T, testCase CmdTestCase) {
+	t.Helper()
 	var flag *pflag.Flag
 
 	if testCase.PersistentFlag {