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 5 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
4 changes: 3 additions & 1 deletion cmd/root.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
e "github.com/cloudposse/atmos/internal/exec"
tuiUtils "github.com/cloudposse/atmos/internal/tui/utils"
cfg "github.com/cloudposse/atmos/pkg/config"
"github.com/cloudposse/atmos/pkg/logger"
"github.com/cloudposse/atmos/pkg/schema"
u "github.com/cloudposse/atmos/pkg/utils"
)
Expand Down Expand Up @@ -71,7 +72,8 @@ 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 IError interface
logger.SetPrintDebugPart(cliConfig.Logs.Level)
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

LGTM: Setting log level for IError interface.

The new line correctly sets the log level for the IError interface using the CLI configuration. This aligns with the PR objective of enhancing error handling and adjusting log levels.

However, consider adding error handling for the SetPrintDebugPart function call to ensure robustness.

Consider updating the code as follows:

 // Set the log level for IError interface
-logger.SetPrintDebugPart(cliConfig.Logs.Level)
+if err := logger.SetPrintDebugPart(cliConfig.Logs.Level); err != nil {
+    u.LogErrorAndExit(err)
+}
📝 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 the log level for IError interface
logger.SetPrintDebugPart(cliConfig.Logs.Level)
// Set the log level for IError interface
if err := logger.SetPrintDebugPart(cliConfig.Logs.Level); err != nil {
u.LogErrorAndExit(err)
}

// If CLI configuration was found, process its custom commands and command aliases
if err == nil {
err = processCustomCommands(cliConfig, cliConfig.Commands, RootCmd, true)
Expand Down
12 changes: 10 additions & 2 deletions pkg/config/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"strconv"
"strings"

"github.com/cloudposse/atmos/pkg/logger"
"github.com/cloudposse/atmos/pkg/schema"
u "github.com/cloudposse/atmos/pkg/utils"
)
Expand Down Expand Up @@ -41,7 +42,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, &logger.IError{
Message: err.Error(),
DebugInfo: fmt.Sprintf("CLI config:\n\n%v", y),
}
haitham911 marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down Expand Up @@ -119,7 +123,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, &logger.IError{
Message: err.Error(),
DebugInfo: fmt.Sprintf("CLI config:\n\n%v", y),
}
haitham911 marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down
22 changes: 22 additions & 0 deletions pkg/logger/logger.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,28 @@ import (
"github.com/cloudposse/atmos/pkg/schema"
)

var PrintDebugPart = false
haitham911 marked this conversation as resolved.
Show resolved Hide resolved

// set PrintDebugPart to true if log level is Debug or Trace
func SetPrintDebugPart(val string) {
if val == string(LogLevelDebug) || val == string(LogLevelTrace) {
PrintDebugPart = true
}
}
haitham911 marked this conversation as resolved.
Show resolved Hide resolved

type IError struct {
Message string
DebugInfo string
}

func (e *IError) Error() string {
// Print debug info if PrintDebugPart is true
if PrintDebugPart {
return fmt.Sprintf("%s\n%s", e.Message, e.DebugInfo)
}
return e.Message
}
haitham911 marked this conversation as resolved.
Show resolved Hide resolved

type LogLevel string

const (
Expand Down
Loading