Validate Atmos Log Levels (#930)

* log level invalid cover * fixes log level wip * added validation for parser * logger level fixes * update logger * checkpoint * checkpoint wip * final logger improvements --------- Co-authored-by: Andriy Knysh <[email protected]>
cloudposse · Jan 16, 2025 · 0a75010 · 0a75010
1 parent 6bd89f1
commit 0a75010
Show file tree

Hide file tree

Showing 6 changed files with 143 additions and 27 deletions.
diff --git a/pkg/config/utils.go b/pkg/config/utils.go
@@ -9,6 +9,7 @@ import (
 	"strings"
 
 	"github.com/charmbracelet/log"
+	"github.com/cloudposse/atmos/pkg/logger"
 	"github.com/cloudposse/atmos/pkg/schema"
 	"github.com/cloudposse/atmos/pkg/store"
 	u "github.com/cloudposse/atmos/pkg/utils"
@@ -358,6 +359,11 @@ func processEnvVars(atmosConfig *schema.AtmosConfiguration) error {
 	logsLevel := os.Getenv("ATMOS_LOGS_LEVEL")
 	if len(logsLevel) > 0 {
 		u.LogTrace(*atmosConfig, fmt.Sprintf("Found ENV var ATMOS_LOGS_LEVEL=%s", logsLevel))
+		// Validate the log level before setting it
+		if _, err := logger.ParseLogLevel(logsLevel); err != nil {
+			return err
+		}
+		// Only set the log level if validation passes
 		atmosConfig.Logs.Level = logsLevel
 	}
 
@@ -396,6 +402,12 @@ func checkConfig(atmosConfig schema.AtmosConfiguration) error {
 		return errors.New("at least one path must be provided in 'stacks.included_paths' config or ATMOS_STACKS_INCLUDED_PATHS' ENV variable")
 	}
 
+	if len(atmosConfig.Logs.Level) > 0 {
+		if _, err := logger.ParseLogLevel(atmosConfig.Logs.Level); err != nil {
+			return err
+		}
+	}
+
 	return nil
 }
 
@@ -473,6 +485,10 @@ func processCommandLineArgs(atmosConfig *schema.AtmosConfiguration, configAndSta
 		u.LogTrace(*atmosConfig, fmt.Sprintf("Using command line argument '%s' as path to Atmos JSON Schema", configAndStacksInfo.AtmosManifestJsonSchema))
 	}
 	if len(configAndStacksInfo.LogsLevel) > 0 {
+		if _, err := logger.ParseLogLevel(configAndStacksInfo.LogsLevel); err != nil {
+			return err
+		}
+		// Only set the log level if validation passes
 		atmosConfig.Logs.Level = configAndStacksInfo.LogsLevel
 		u.LogTrace(*atmosConfig, fmt.Sprintf("Using command line argument '%s=%s'", LogsLevelFlag, configAndStacksInfo.LogsLevel))
 	}

diff --git a/pkg/logger/logger.go b/pkg/logger/logger.go
@@ -20,6 +20,15 @@ const (
 	LogLevelWarning LogLevel = "Warning"
 )
 
+// logLevelOrder defines the order of log levels from most verbose to least verbose
+var logLevelOrder = map[LogLevel]int{
+	LogLevelTrace:   0,
+	LogLevelDebug:   1,
+	LogLevelInfo:    2,
+	LogLevelWarning: 3,
+	LogLevelOff:     4,
+}
+
 type Logger struct {
 	LogLevel LogLevel
 	File     string
@@ -45,18 +54,14 @@ func ParseLogLevel(logLevel string) (LogLevel, error) {
 		return LogLevelInfo, nil
 	}
 
-	switch LogLevel(logLevel) { // Convert logLevel to type LogLevel
-	case LogLevelTrace:
-		return LogLevelTrace, nil
-	case LogLevelDebug:
-		return LogLevelDebug, nil
-	case LogLevelInfo:
-		return LogLevelInfo, nil
-	case LogLevelWarning:
-		return LogLevelWarning, nil
-	default:
-		return LogLevelInfo, fmt.Errorf("invalid log level '%s'. Supported log levels are Trace, Debug, Info, Warning, Off", logLevel)
+	validLevels := []LogLevel{LogLevelTrace, LogLevelDebug, LogLevelInfo, LogLevelWarning, LogLevelOff}
+	for _, level := range validLevels {
+		if LogLevel(logLevel) == level {
+			return level, nil
+		}
 	}
+
+	return "", fmt.Errorf("Error: Invalid log level '%s'. Valid options are: %v", logLevel, validLevels)
 }
 
 func (l *Logger) log(logColor *color.Color, message string) {
@@ -104,7 +109,7 @@ func (l *Logger) SetLogLevel(logLevel LogLevel) error {
 }
 
 func (l *Logger) Error(err error) {
-	if err != nil {
+	if err != nil && l.LogLevel != LogLevelOff {
 		_, err2 := theme.Colors.Error.Fprintln(color.Error, err.Error()+"\n")
 		if err2 != nil {
 			color.Red("Error logging the error:")
@@ -115,35 +120,34 @@ func (l *Logger) Error(err error) {
 	}
 }
 
+// isLevelEnabled checks if a given log level should be enabled based on the logger's current level
+func (l *Logger) isLevelEnabled(level LogLevel) bool {
+	if l.LogLevel == LogLevelOff {
+		return false
+	}
+	return logLevelOrder[level] >= logLevelOrder[l.LogLevel]
+}
+
 func (l *Logger) Trace(message string) {
-	if l.LogLevel == LogLevelTrace {
+	if l.isLevelEnabled(LogLevelTrace) {
 		l.log(theme.Colors.Info, message)
 	}
 }
 
 func (l *Logger) Debug(message string) {
-	if l.LogLevel == LogLevelTrace ||
-		l.LogLevel == LogLevelDebug {
-
+	if l.isLevelEnabled(LogLevelDebug) {
 		l.log(theme.Colors.Info, message)
 	}
 }
 
 func (l *Logger) Info(message string) {
-	if l.LogLevel == LogLevelTrace ||
-		l.LogLevel == LogLevelDebug ||
-		l.LogLevel == LogLevelInfo {
-
-		l.log(theme.Colors.Default, message)
+	if l.isLevelEnabled(LogLevelInfo) {
+		l.log(theme.Colors.Info, message)
 	}
 }
 
 func (l *Logger) Warning(message string) {
-	if l.LogLevel == LogLevelTrace ||
-		l.LogLevel == LogLevelDebug ||
-		l.LogLevel == LogLevelInfo ||
-		l.LogLevel == LogLevelWarning {
-
+	if l.isLevelEnabled(LogLevelWarning) {
 		l.log(theme.Colors.Warning, message)
 	}
 }
diff --git a/pkg/utils/log_utils.go b/pkg/utils/log_utils.go
@@ -48,7 +48,7 @@ func LogErrorAndExit(atmosConfig schema.AtmosConfiguration, err error) {
 // LogError logs errors to std.Error
 func LogError(atmosConfig schema.AtmosConfiguration, err error) {
 	if err != nil {
-		_, printErr := theme.Colors.Error.Fprintln(color.Error, err.Error()+"\n")
+		_, printErr := theme.Colors.Error.Fprintln(color.Error, err.Error())
 		if printErr != nil {
 			theme.Colors.Error.Println("Error logging the error:")
 			theme.Colors.Error.Printf("%s\n", printErr)

diff --git a/tests/fixtures/invalid-log-level/atmos.yaml b/tests/fixtures/invalid-log-level/atmos.yaml
@@ -0,0 +1,8 @@
+logs:
+  level: XTrace
+  file: /dev/stdout
+
+stacks:
+  base_path: stacks
+  included_paths:
+    - "**/*" 
diff --git a/tests/fixtures/valid-log-level/atmos.yaml b/tests/fixtures/valid-log-level/atmos.yaml
@@ -0,0 +1,8 @@
+logs:
+  level: Trace
+  file: /dev/stdout
+
+stacks:
+  base_path: stacks
+  included_paths:
+    - "**/*" 
diff --git a/tests/test-cases/log-level-validation.yaml b/tests/test-cases/log-level-validation.yaml
@@ -0,0 +1,80 @@
+tests:
+  - name: "Invalid Log Level in Config File"
+    enabled: true
+    description: "Test validation of invalid log level in atmos.yaml config file"
+    workdir: "fixtures/invalid-log-level"
+    command: "atmos"
+    args:
+      - terraform
+      - plan
+      - test
+      - -s
+      - test
+    expect:
+      exit_code: 1
+      stderr:
+        - "Error: Invalid log level 'XTrace'. Valid options are: \\[Trace Debug Info Warning Off\\]"
+
+  - name: "Invalid Log Level in Environment Variable"
+    enabled: true
+    description: "Test validation of invalid log level in ATMOS_LOGS_LEVEL env var"
+    workdir: "../"
+    command: "atmos"
+    args:
+      - terraform
+      - plan
+      - test
+      - -s
+      - test
+    env:
+      ATMOS_LOGS_LEVEL: XTrace
+    expect:
+      exit_code: 1
+      stderr:
+        - "Error: Invalid log level 'XTrace'. Valid options are: \\[Trace Debug Info Warning Off\\]"
+
+  - name: "Valid Log Level in Config File"
+    enabled: true
+    description: "Test validation of valid log level in atmos.yaml config file"
+    workdir: "fixtures/valid-log-level"
+    command: "atmos"
+    args:
+      - terraform
+      - plan
+      - test
+      - -s
+      - test
+    expect:
+      exit_code: 0
+
+  - name: "Valid Log Level in Environment Variable"
+    enabled: true
+    description: "Test validation of valid log level in ATMOS_LOGS_LEVEL env var"
+    workdir: "../"
+    command: "atmos"
+    args:
+      - terraform
+      - plan
+      - test
+      - -s
+      - test
+    env:
+      ATMOS_LOGS_LEVEL: Debug
+    expect:
+      exit_code: 0
+
+  - name: "Valid Log Level in Command Line Flag"
+    enabled: true
+    description: "Test validation of valid log level in --logs-level flag"
+    workdir: "../"
+    command: "atmos"
+    args:
+      - --logs-level
+      - Info
+      - terraform
+      - plan
+      - test
+      - -s
+      - test
+    expect:
+      exit_code: 0