Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DO NOT MERGE (needs review and testing). Break error into parts #715

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
+58 −3
Draft

DO NOT MERGE (needs review and testing). Break error into parts #715

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
f19ad69
Set the log level for IError interface
haitham911 Oct 12, 2024
d36cbf0
add error interface
haitham911 Oct 12, 2024
92678a9
Refactor error handling use error interface
haitham911 Oct 12, 2024
83674d1
add comment to Ierror
haitham911 Oct 12, 2024
2bc991d
Merge branch 'main' into DEV-340
aknysh Oct 12, 2024
a08d257
Refactor error handling and logging configuration
haitham911 Oct 12, 2024
4496b1c
modify check error handling to empty
haitham911 Oct 12, 2024
f2ba630
Merge branch 'main' into DEV-340
aknysh Oct 14, 2024
0ec4c03
Merge branch 'main' into DEV-340
aknysh Oct 14, 2024
f7cc62e
Merge branch 'main' into DEV-340
aknysh Oct 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion cmd/root.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,12 @@ func Execute() error {
if err != nil && !errors.Is(err, cfg.NotFound) {
u.LogErrorAndExit(schema.CliConfiguration{}, err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
u.LogErrorAndExit(schema.CliConfiguration{}, err)
u.LogErrorAndExit(cliConfig, err)

We have cliConfig, let's use it (I missed this line in the previous PR)

}

// Set the log level for ExtendedError interface . must be set on root level to avoid missing log level
if cliConfig.Logs.Level == u.LogLevelDebug || cliConfig.Logs.Level == u.LogLevelTrace {
if err := u.SetPrintDebugPart(cliConfig.Logs.Level); err != nil {
u.LogErrorAndExit(schema.CliConfiguration{}, err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
u.LogErrorAndExit(schema.CliConfiguration{}, err)
u.LogErrorAndExit(cliConfig, err)

We already have cliConfig, let's use it

}
}
// If CLI configuration was found, process its custom commands and command aliases
if err == nil {
err = processCustomCommands(cliConfig, cliConfig.Commands, RootCmd, true)
Expand Down
1 change: 1 addition & 0 deletions pkg/config/const.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,4 +68,5 @@ const (
AtmosProTokenEnvVarName = "ATMOS_PRO_TOKEN"
AtmosProDefaultBaseUrl = "https://app.cloudposse.com"
AtmosProDefaultEndpoint = "api"
CliConfigPrefix = "CLI config:"
)
11 changes: 9 additions & 2 deletions pkg/config/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,10 @@ func FindAllStackConfigsInPathsForStack(
matches, err = u.GetGlobMatches(pathWithExt)
if err != nil {
y, _ := u.ConvertToYAML(cliConfig)
return nil, nil, false, fmt.Errorf("%v\n\n\nCLI config:\n\n%v", err, y)
return nil, nil, false, &u.ExtendedError{
Message: err.Error(),
DebugInfo: fmt.Sprintf(CliConfigPrefix+"\n\n%v", y),
}
aknysh marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down Expand Up @@ -119,7 +122,11 @@ func FindAllStackConfigsInPaths(
matches, err = u.GetGlobMatches(pathWithExt)
if err != nil {
y, _ := u.ConvertToYAML(cliConfig)
return nil, nil, fmt.Errorf("%v\n\n\nCLI config:\n\n%v", err, y)
// Return an error with the debug info
return nil, nil, &u.ExtendedError{
Message: err.Error(),
DebugInfo: fmt.Sprintf(CliConfigPrefix+"\n\n%v", y),
}
aknysh marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down
42 changes: 42 additions & 0 deletions pkg/utils/log_utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"os"
"os/exec"
"runtime/debug"
"sync"

"github.com/fatih/color"

Expand All @@ -19,6 +20,41 @@ const (
LogLevelWarning = "Warning"
)

var (
PrintDebugPart bool
mu sync.Mutex
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider encapsulating global variables PrintDebugPart and mu to avoid global state

The use of global variables PrintDebugPart and mu can lead to unintended side effects in concurrent environments and make testing more difficult. Encapsulating these variables within a struct or providing controlled access through getter and setter functions can enhance modularity and maintainability.


// set PrintDebugPart to true if log level is Debug or Trace
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// set PrintDebugPart to true if log level is Debug or Trace
// SetPrintDebugPart .....

In Go, comments must start with the function name

func SetPrintDebugPart(val string) error {
mu.Lock()
haitham911 marked this conversation as resolved.
Show resolved Hide resolved
defer mu.Unlock()
if val == "" {
return fmt.Errorf("log level not set")
}
if val == string(LogLevelDebug) || val == string(LogLevelTrace) {
PrintDebugPart = true
} else {
PrintDebugPart = false
}
return nil
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Simplify SetPrintDebugPart function and remove unnecessary type conversions

In the SetPrintDebugPart function, the string() type conversions are unnecessary because LogLevelDebug and LogLevelTrace are already string constants. Removing these redundant conversions simplifies the code.

Apply this diff to remove redundant type conversions:

func SetPrintDebugPart(val string) error {
	mu.Lock()
	defer mu.Unlock()
	if val == "" {
		return fmt.Errorf("log level not set")
	}
-	if val == string(LogLevelDebug) || val == string(LogLevelTrace) {
+	if val == LogLevelDebug || val == LogLevelTrace {
		PrintDebugPart = true
	} else {
		PrintDebugPart = false
	}
	return nil
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// set PrintDebugPart to true if log level is Debug or Trace
func SetPrintDebugPart(val string) error {
mu.Lock()
defer mu.Unlock()
if val == "" {
return fmt.Errorf("log level not set")
}
if val == string(LogLevelDebug) || val == string(LogLevelTrace) {
PrintDebugPart = true
} else {
PrintDebugPart = false
}
return nil
}
// set PrintDebugPart to true if log level is Debug or Trace
func SetPrintDebugPart(val string) error {
mu.Lock()
defer mu.Unlock()
if val == "" {
return fmt.Errorf("log level not set")
}
if val == LogLevelDebug || val == LogLevelTrace {
PrintDebugPart = true
} else {
PrintDebugPart = false
}
return nil
}


// ExtendedError is an error type that includes a message and debug info.
type ExtendedError struct {
Message string // Error message to be printed with log level Error , Warning or Info
aknysh marked this conversation as resolved.
Show resolved Hide resolved
DebugInfo string // Debug info to be printed with log level Debug or Trace
}

// Error returns the error message . If PrintDebugPart is true, it returns the error message and debug info
aknysh marked this conversation as resolved.
Show resolved Hide resolved
func (e *ExtendedError) Error() string {
// Print debug info if PrintDebugPart is true
if PrintDebugPart {
return fmt.Sprintf("%s\n%s", e.Message, e.DebugInfo)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use fmt.Sprintf, Atmos has many helper functions to print info, warning and error in diff colors.

These two parts e.Message and e.DebugInfo should be printed in diff colors (this is the main point of this PR)

}
return e.Message
}
aknysh marked this conversation as resolved.
Show resolved Hide resolved

aknysh marked this conversation as resolved.
Show resolved Hide resolved
// PrintMessage prints the message to the console
func PrintMessage(message string) {
fmt.Println(message)
Expand Down Expand Up @@ -48,6 +84,12 @@ func LogErrorAndExit(cliConfig schema.CliConfiguration, err error) {
// LogError logs errors to std.Error
func LogError(cliConfig schema.CliConfiguration, err error) {
if err != nil {
// set PrintDebugPart to true if log level is Debug or Trace
if cliConfig.Logs.Level == LogLevelDebug || cliConfig.Logs.Level == LogLevelTrace {
if setErr := SetPrintDebugPart(cliConfig.Logs.Level); setErr != nil {
color.Red("%s\n", setErr)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Initialize PrintDebugPart during startup instead of within LogError

Setting PrintDebugPart inside the LogError function causes it to be updated every time an error is logged, which can be inefficient and may introduce race conditions. It's advisable to initialize PrintDebugPart once during application startup or configuration loading to improve performance and ensure thread safety.

c := color.New(color.FgRed)
_, printErr := c.Fprintln(color.Error, err.Error()+"\n")
if printErr != nil {
Expand Down
Loading