Migrate to Charmbracelet Logger #980
Conversation
|
💥 This pull request now has conflicts. Could you fix it @mcalhoun? 🙏 |
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive refactoring of error logging and configuration handling across the Atmos codebase. The primary change involves removing the Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
The changes represent a methodical approach to cleaning up logging across the project, reducing unnecessary complexity while maintaining the core functionality of error reporting and logging. By removing the 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Learnings (1)tests/snapshots/TestCLICommands_check_atmos_--help_in_empty-dir.stdout.golden (1)⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (5)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (20)
pkg/utils/log_utils.go (1)
57-58: Differentiate trace from debug.
Consider adding a distinct trace level if deeper verbosity is desired.pkg/validate/validate_component_test.go (1)
Line range hint
13-89: Consider improving test organization.The test functions share similar setup code. Consider using test table patterns or test helpers to reduce duplication and improve maintainability.
Example refactor:
+func setupTest(t *testing.T) (schema.AtmosConfiguration, schema.ConfigAndStacksInfo) { + info := schema.ConfigAndStacksInfo{} + atmosConfig, err := cfg.InitCliConfig(info, true) + assert.Nil(t, err) + return atmosConfig, info +} + +type validateComponentTestCase struct { + name string + component string + stack string + policyFile string + policyType string + imports []string + expectedError string +} + +func TestValidateComponent(t *testing.T) { + atmosConfig, info := setupTest(t) + + tests := []validateComponentTestCase{ + { + name: "vpc component validation", + component: "infra/vpc", + stack: "tenant1-ue2-dev", + policyFile: "vpc/validate-infra-vpc-component.rego", + policyType: "opa", + imports: []string{"catalog"}, + }, + // Add other test cases... + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + _, err := e.ExecuteValidateComponent( + atmosConfig, + info, + tc.component, + tc.stack, + tc.policyFile, + tc.policyType, + tc.imports, + 0) + u.LogError(err) + assert.Error(t, err) + if tc.expectedError != "" { + assert.Equal(t, tc.expectedError, err.Error()) + } + }) + } +}internal/exec/terraform_generate_varfile.go (1)
62-62: Consider standardizing debug message format.While the debug messages are informative, we could enhance consistency by standardizing the format:
-u.LogDebug(fmt.Sprintf("\nVariables for the component '%s' in the stack '%s':", info.ComponentFromArg, info.Stack)) -u.LogDebug("Writing the variables to file:") -u.LogDebug(varFilePath) +u.LogDebug(fmt.Sprintf("Variables for component '%s' in stack '%s'", info.ComponentFromArg, info.Stack)) +u.LogDebug(fmt.Sprintf("Writing variables to file: %s", varFilePath))Also applies to: 72-73
cmd/version.go (1)
37-45: Simplify error handling flow.The nested error checks could be flattened for better readability:
- if err == nil && latestReleaseTag != "" { - if err != nil { - u.LogWarning(fmt.Sprintf("Failed to check for updates: %v", err)) - return - } - if latestReleaseTag == "" { - u.LogWarning("No release information available") - return - } + if err != nil { + u.LogWarning(fmt.Sprintf("Failed to check for updates: %v", err)) + return + } + if latestReleaseTag == "" { + u.LogWarning("No release information available") + return + }internal/exec/template_funcs_component.go (1)
18-18: LGTM! Trace and error logging properly migrated.The function's logging has been successfully migrated while maintaining:
- Execution tracing
- Cache hit logging
- YAML conversion error handling
- Output section tracing
However, consider adding debug level logging for cache misses to improve observability.
existingSections, found := componentFuncSyncMap.Load(stackSlug) if found && existingSections != nil { if atmosConfig.Logs.Level == u.LogLevelTrace { + } else { + log.Debug("Cache miss", "component", component, "stack", stack) }Also applies to: 26-26, 29-29, 32-34, 76-76, 79-81
internal/exec/oci_utils.go (1)
63-65: Consider enhancing with structured loggingThe removal of
atmosConfigsimplifies the error logging. Since we're migrating to charmbracelet logger, we could further improve this by using structured logging:- u.LogError(err) + log.Error("failed to open tarball file", "error", err)cmd/terraform_generate_varfiles.go (2)
22-22: Consider enhancing error loggingWhile removing
atmosConfigis good, we could further improve by using structured logging here as well:- u.LogErrorAndExit(err) + log.Error("command execution failed", "error", err) + os.Exit(1)This would provide more context about where and why the error occurred.
Also applies to: 60-60
Line range hint
1-1: Consider standardizing structured logging patternsThe migration to charmbracelet logger is well-executed. To maximize its benefits, consider:
- Standardizing key names across all logging calls
- Creating helper functions for common logging patterns
- Documenting the structured logging conventions in a central location
This will ensure consistency as the codebase grows.
pkg/utils/json_utils.go (1)
44-44: Consider adding structured logging fields for better context.While simplifying the logging is good, we could enhance it further with charmbracelet's structured logging capabilities.
-LogInfo(j) +LogInfo("json_output", log.WithValue("json", j))pkg/config/cache.go (1)
107-107: Enhance warning log with structured fields.Let's leverage charmbracelet's structured logging for better log parsing and filtering.
-u.LogWarning(fmt.Sprintf("Unsupported frequency '%s' encountered. Defaulting to daily.", frequency)) +u.LogWarning("Unsupported frequency encountered", + log.WithString("frequency", frequency), + log.WithString("default", "daily"))cmd/describe_affected.go (1)
Line range hint
1-1: Consider creating a centralized logging package.To fully leverage charmbracelet logger's capabilities and ensure consistent logging patterns across the codebase, consider:
- Creating a dedicated logging package with pre-configured log levels and formats
- Implementing helper functions for common logging patterns
- Establishing guidelines for structured logging field names
This will make the logging migration more maintainable and standardized.
internal/exec/terraform_generate_backends.go (1)
260-260: Consider adding more context to the error message.The error handling is correct, but adding more context about the component section conversion would make debugging easier.
-u.LogErrorAndExit(err) +u.LogErrorAndExit(fmt.Errorf("failed to convert component section: %w", err))pkg/config/config.go (1)
393-393: Improve string formatting for better readability and safety.The warning message uses string concatenation which is less readable and could be improved using proper formatting.
-u.LogWarning(fmt.Sprintf("error closing file '" + configPath + "'. " + err.Error())) +u.LogWarning(fmt.Sprintf("error closing file '%s': %v", configPath, err))🧰 Tools
🪛 golangci-lint (1.62.2)
393-393: printf: non-constant format string in call to fmt.Sprintf
(govet)
pkg/spacelift/spacelift_stack_processor.go (1)
190-190: Consider consolidating error handling.The error handling pattern is repeated multiple times in
TransformStackConfigToSpaceliftStacks. Consider creating a helper function to handle these errors consistently.+func logAndReturnError(err error) (map[string]any, error) { + u.LogError(err) + return nil, err +}Then use it consistently throughout the function:
-u.LogError(err) -return nil, err +return logAndReturnError(err)Also applies to: 260-260, 303-303, 363-363, 381-381, 400-400
internal/exec/terraform_clean.go (2)
290-292: Consider using LogInfo instead of LogWarning for user-initiated actions.The "Mission aborted" message is a result of a user choice rather than a warning condition. Using
u.LogInfowould better reflect the nature of this message.-u.LogWarning("Mission aborted.") +u.LogInfo("Mission aborted.")
338-346: Consider consolidating error handling in handleTFDataDir.The function could be simplified by combining the validation and existence checks into a single error handling block.
-if err := IsValidDataDir(tfDataDir); err != nil { - u.LogWarning(err.Error()) - return -} -if _, err := os.Stat(filepath.Join(componentPath, tfDataDir)); os.IsNotExist(err) { - u.LogWarning(fmt.Sprintf("TF_DATA_DIR '%s' does not exist", tfDataDir)) - return -} -if err := DeletePathTerraform(filepath.Join(componentPath, tfDataDir), filepath.Join(relativePath, tfDataDir)); err != nil { - u.LogWarning(err.Error()) -} +if err := IsValidDataDir(tfDataDir); err != nil { + u.LogWarning(err.Error()) + return +} + +tfDataDirPath := filepath.Join(componentPath, tfDataDir) +if _, err := os.Stat(tfDataDirPath); os.IsNotExist(err) { + u.LogWarning(fmt.Sprintf("TF_DATA_DIR '%s' does not exist", tfDataDir)) + return +} + +if err := DeletePathTerraform(tfDataDirPath, filepath.Join(relativePath, tfDataDir)); err != nil { + u.LogWarning(err.Error()) +}internal/exec/describe_affected_utils.go (1)
451-458: Optimize string handling in logging callsThe fmt.Sprintf calls are unnecessary when no string formatting is needed. Let's simplify:
-u.LogTrace(fmt.Sprintf("\nGetting current working repo commit object...")) +u.LogTrace("\nGetting current working repo commit object...") -u.LogTrace(fmt.Sprintf("Got current working repo commit object")) +u.LogTrace("Got current working repo commit object")🧰 Tools
🪛 golangci-lint (1.62.2)
451-451: S1039: unnecessary use of fmt.Sprintf
(gosimple)
458-458: S1039: unnecessary use of fmt.Sprintf
(gosimple)
cmd/validate_stacks.go (1)
24-27: Consider enhancing success message with lipgloss.Since we're migrating to charmbracelet ecosystem, we could improve the visual presentation of the success message using lipgloss styles.
- u.PrintMessageInColor("all stacks validated successfully\n", theme.Colors.Success) + style := lipgloss.NewStyle().Foreground(lipgloss.Color("10")).Bold() + fmt.Println(style.Render("✓ All stacks validated successfully"))cmd/terraform_generate_backend.go (1)
23-23: Standardize error presentation across both error paths.Both error paths should present errors consistently using charmbracelet logger's formatting capabilities.
Consider wrapping both errors with context:
- u.LogErrorAndExit(err) + u.LogErrorAndExit(fmt.Errorf("failed to generate backend: %w", err))- u.LogErrorAndExit(err) + u.LogErrorAndExit(fmt.Errorf("failed to mark stack flag as required: %w", err))Also applies to: 34-34
pkg/utils/hcl_utils.go (1)
51-51: Simplified warning logs in defer blocks!The warning logs in file handling are properly maintained while adapting to the new logging system. Consider adding file paths to the warning messages for better debugging context.
- LogWarning(err.Error()) + LogWarning(fmt.Sprintf("Error closing file %s: %s", filePath, err.Error()))Also applies to: 132-132
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (73)
cmd/atlantis_generate_repo_config.go(1 hunks)cmd/aws_eks_update_kubeconfig.go(1 hunks)cmd/cmd_utils.go(12 hunks)cmd/completion.go(1 hunks)cmd/describe_affected.go(1 hunks)cmd/describe_component.go(2 hunks)cmd/describe_config.go(1 hunks)cmd/describe_dependents.go(2 hunks)cmd/describe_stacks.go(1 hunks)cmd/describe_workflows.go(1 hunks)cmd/docs.go(4 hunks)cmd/helmfile.go(2 hunks)cmd/helmfile_generate_varfile.go(2 hunks)cmd/pro_lock.go(1 hunks)cmd/pro_unlock.go(1 hunks)cmd/root.go(5 hunks)cmd/terraform.go(2 hunks)cmd/terraform_generate_backend.go(2 hunks)cmd/terraform_generate_backends.go(1 hunks)cmd/terraform_generate_varfile.go(2 hunks)cmd/terraform_generate_varfiles.go(2 hunks)cmd/validate_component.go(2 hunks)cmd/validate_stacks.go(1 hunks)cmd/vendor_diff.go(1 hunks)cmd/vendor_pull.go(1 hunks)cmd/version.go(2 hunks)cmd/workflow.go(5 hunks)internal/exec/atlantis_generate_repo_config.go(1 hunks)internal/exec/aws_eks_update_kubeconfig.go(1 hunks)internal/exec/describe_affected_utils.go(12 hunks)internal/exec/describe_stacks.go(2 hunks)internal/exec/file_utils.go(1 hunks)internal/exec/helmfile.go(7 hunks)internal/exec/helmfile_generate_varfile.go(2 hunks)internal/exec/oci_utils.go(1 hunks)internal/exec/shell_utils.go(10 hunks)internal/exec/tar_utils.go(2 hunks)internal/exec/template_funcs_component.go(2 hunks)internal/exec/template_funcs_gomplate_datasource.go(3 hunks)internal/exec/template_utils.go(3 hunks)internal/exec/terraform.go(10 hunks)internal/exec/terraform_clean.go(6 hunks)internal/exec/terraform_generate_backend.go(2 hunks)internal/exec/terraform_generate_backends.go(2 hunks)internal/exec/terraform_generate_varfile.go(2 hunks)internal/exec/terraform_generate_varfiles.go(2 hunks)internal/exec/terraform_outputs.go(10 hunks)internal/exec/terraform_utils.go(1 hunks)internal/exec/utils.go(5 hunks)internal/exec/validate_component.go(2 hunks)internal/exec/validate_stacks.go(2 hunks)internal/exec/vendor_component_utils.go(4 hunks)internal/exec/vendor_model.go(3 hunks)internal/exec/vendor_utils.go(8 hunks)internal/exec/workflow_utils.go(4 hunks)internal/exec/yaml_func_exec.go(1 hunks)internal/exec/yaml_func_store.go(1 hunks)internal/exec/yaml_func_template.go(1 hunks)internal/exec/yaml_func_terraform_output.go(2 hunks)main.go(1 hunks)pkg/aws/aws_eks_update_kubeconfig.go(1 hunks)pkg/component/component_processor.go(3 hunks)pkg/config/cache.go(1 hunks)pkg/config/config.go(3 hunks)pkg/config/utils.go(8 hunks)pkg/hooks/cmd.go(3 hunks)pkg/spacelift/spacelift_stack_processor.go(9 hunks)pkg/utils/hcl_utils.go(2 hunks)pkg/utils/json_utils.go(1 hunks)pkg/utils/log_utils.go(2 hunks)pkg/utils/yaml_utils.go(1 hunks)pkg/validate/validate_component_test.go(4 hunks)pkg/validate/validate_stacks_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- internal/exec/atlantis_generate_repo_config.go
- internal/exec/validate_component.go
- internal/exec/template_utils.go
- internal/exec/aws_eks_update_kubeconfig.go
🧰 Additional context used
📓 Learnings (5)
internal/exec/terraform_clean.go (1)
Learnt from: osterman
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:329-332
Timestamp: 2024-11-12T05:52:05.088Z
Learning: In the Atmos Go code, when deleting directories or handling file paths (e.g., in `terraform_clean.go`), always resolve the absolute path using `filepath.Abs` and use the logger `u.LogWarning` for logging messages instead of using `fmt.Printf`.
internal/exec/yaml_func_terraform_output.go (1)
Learnt from: aknysh
PR: cloudposse/atmos#863
File: internal/exec/yaml_func_terraform_output.go:34-38
Timestamp: 2024-12-17T07:08:41.288Z
Learning: In the `processTagTerraformOutput` function within `internal/exec/yaml_func_terraform_output.go`, parameters are separated by spaces and do not contain spaces. Therefore, using `strings.Fields()` for parsing is acceptable, and there's no need to handle parameters with spaces.
internal/exec/terraform_outputs.go (1)
Learnt from: aknysh
PR: cloudposse/atmos#810
File: internal/exec/terraform_utils.go:40-213
Timestamp: 2024-12-03T05:18:49.169Z
Learning: In the context of the Atmos project, it's acceptable for functions like `execTerraformOutput` to remain as single functions if they perform a single purpose, such as retrieving Terraform outputs for a component in a stack, even if the function is lengthy.
cmd/cmd_utils.go (1)
Learnt from: osterman
PR: cloudposse/atmos#740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-11-12T05:52:05.088Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
internal/exec/helmfile_generate_varfile.go (1)
Learnt from: Listener430
PR: cloudposse/atmos#825
File: internal/exec/helmfile_generate_varfile.go:28-31
Timestamp: 2024-12-07T16:16:13.038Z
Learning: In `internal/exec/helmfile_generate_varfile.go`, the `--help` command (`./atmos helmfile generate varfile --help`) works correctly without requiring stack configurations, and the only change needed was to make `ProcessCommandLineArgs` exportable by capitalizing its name.
🪛 golangci-lint (1.62.2)
pkg/config/config.go
393-393: printf: non-constant format string in call to fmt.Sprintf
(govet)
internal/exec/describe_affected_utils.go
451-451: S1039: unnecessary use of fmt.Sprintf
(gosimple)
458-458: S1039: unnecessary use of fmt.Sprintf
(gosimple)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (92)
pkg/utils/log_utils.go (6)
10-10: Smooth logger import.
The aliaslis concise and keeps things simple.
32-42: Well-structured exit logic.
The removal of the configuration parameter simplifies this function. Good work.
46-52: Clean error handling.
Logging the error and optionally showing a stack trace is straightforward.
62-63: Effective debug logging.
Nicely migrated to the new logger.
67-68: Straightforward info logging.
Implementation is consistent with the rest.
72-73: Uniform warning handling.
This mirrors the pattern from other log methods.main.go (1)
12-12: Streamlined error exit.
Removing the configuration argument makes sense.pkg/aws/aws_eks_update_kubeconfig.go (1)
15-15: Consistent error reporting.
Swapping toLogErroronly is in line with the new approach.pkg/validate/validate_stacks_test.go (1)
15-15: Tests should verify charmbracelet logger integration.While removing
atmosConfigparameter, we should also update tests to verify the integration with charmbracelet logger. Consider adding test cases that validate:
- Correct log format using charmbracelet logger
- Log level configuration
- Log output styling capabilities
Also applies to: 21-21, 27-27
cmd/completion.go (1)
33-33: LGTM! Clean error handling.The simplified error logging aligns well with the migration to charmbracelet logger. The error handling remains functionally equivalent while removing unnecessary configuration context.
internal/exec/file_utils.go (2)
15-15: Clean warning log in removeTempDir.Good simplification of warning log while maintaining the essential error message.
22-22: Well-formatted error message in closeFile.The error message retains all necessary context (filename and error) while simplifying the logging call.
internal/exec/terraform_utils.go (1)
29-29: Clear debug logging for Terraform workspace cleanup.The debug message maintains clarity about the operation being performed while aligning with the new logging approach.
cmd/describe_workflows.go (1)
26-26: LGTM! Consistent error handling.The simplified error logging maintains consistency with other command files while supporting the migration to charmbracelet logger.
cmd/vendor_diff.go (1)
22-22: LGTM! Clean logging migration.The removal of the
atmosConfigparameter fromLogErrorAndExitaligns with the migration to charmbracelet logger.internal/exec/yaml_func_terraform_output.go (1)
17-17: LGTM! Consistent logging migration.All logging calls have been updated to remove the
atmosConfigparameter while maintaining clear and informative log messages.Also applies to: 21-21, 41-41, 45-45
cmd/describe_dependents.go (1)
23-23: LGTM! Error handling remains solid.The removal of
atmosConfigfrom bothLogErrorAndExitcalls maintains proper error handling while aligning with the new logging approach.Also applies to: 37-37
pkg/validate/validate_component_test.go (1)
29-29: LGTM! Test logging updated consistently.The removal of
atmosConfigfromLogErrorcalls maintains test clarity while aligning with the new logging approach.Also applies to: 48-48, 67-67, 86-86
cmd/describe_component.go (1)
22-22: Clean and consistent error handling changes!The removal of
schema.AtmosConfiguration{}fromLogErrorAndExitcalls aligns with the migration to charmbracelet logger. The error handling remains functionally equivalent while being more concise.Also applies to: 36-36
cmd/vendor_pull.go (1)
23-23: Solid error handling simplification!The streamlined
LogErrorAndExit(err)call maintains error handling while removing unnecessary configuration context.cmd/version.go (1)
27-27: Clean logging migration!The removal of
schema.AtmosConfiguration{}from logging calls is consistent with the charmbracelet logger migration.Also applies to: 38-38, 42-42
internal/exec/tar_utils.go (2)
45-48: LGTM! Security logging is preserved.The security warning for potential directory traversal attacks is properly maintained while migrating to the new logging format.
87-90: LGTM! Warning logs maintained.The warning for unsupported header types is correctly preserved in the new logging format.
internal/exec/yaml_func_store.go (1)
49-49: LGTM! Error handling properly migrated.The error handling has been successfully migrated to the new logging format while maintaining all critical error messages and exit conditions.
Also applies to: 54-54, 60-61, 66-67
cmd/validate_component.go (1)
28-28: LGTM! Command error handling preserved.The command validation error handling has been successfully migrated while maintaining proper exit conditions for both execution and flag validation errors.
Also applies to: 47-47
pkg/utils/yaml_utils.go (1)
52-52: Clean and focused logging - well done!The removal of the
atmosConfigparameter fromLogInfosimplifies the logging while maintaining the essential information. This aligns perfectly with the migration to charmbracelet logger.pkg/hooks/cmd.go (3)
7-7: Excellent choice of logging library!The charmbracelet logger is a solid choice for structured logging.
40-40: Well-implemented structured logging - great job!The conversion to structured logging with key-value pairs is excellent. It provides better context and makes logs more machine-parseable while remaining human-readable.
Also applies to: 47-47, 51-51
66-66: Clean error handling approachThe simplified error logging maintains clarity while removing unnecessary context. This is consistent with the project's new logging standards.
Also applies to: 71-71, 78-78
cmd/terraform_generate_backends.go (1)
22-22: LGTM! Clean error handling.The simplified error logging maintains all necessary information while reducing complexity.
cmd/describe_affected.go (1)
22-22: LGTM! Consistent error handling.The simplified error logging aligns with the project-wide pattern and maintains clarity.
cmd/root.go (2)
53-53: LGTM! Error handling simplification looks good.The removal of
schema.AtmosConfiguration{}fromLogErrorAndExitcalls makes the code cleaner and more maintainable.Also applies to: 58-58, 113-113, 125-125, 130-130, 150-150, 169-169, 172-172, 177-177, 181-181
117-118: Consider thread safety in logger initialization.The logger setup should be synchronized to prevent potential race conditions if multiple goroutines attempt to initialize the logger simultaneously.
Consider wrapping the setup in a sync.Once:
+var setupLoggerOnce sync.Once func Execute() error { // ... - setupLogger(&atmosConfig) + setupLoggerOnce.Do(func() { + setupLogger(&atmosConfig) + }) // ... }cmd/docs.go (1)
39-39: LGTM! Error handling changes are consistent.The simplification of error handling aligns well with the overall PR objective of streamlining logging.
Also applies to: 69-69, 72-72, 78-78, 80-80, 86-86, 96-96, 101-101, 111-111
cmd/terraform.go (1)
44-44: LGTM! Error handling simplification is consistent.The changes maintain consistency with the project-wide error handling improvements.
Also applies to: 138-138
cmd/atlantis_generate_repo_config.go (1)
22-22: LGTM! Error handling is consistent with other changes.The simplification aligns with the project-wide improvements in error handling.
cmd/workflow.go (1)
90-90: LGTM! Logging standardization looks good.The removal of
atmosConfigparameter fromLogErrorAndExitcalls aligns with the migration to charmbracelet logger. The error handling remains robust while achieving a more standardized format.Let's verify the logging standardization across the codebase:
Also applies to: 97-97, 110-110, 125-125, 133-133, 171-171, 181-181, 185-185
✅ Verification successful
VERIFIED: Logging standardization is complete and consistent! 🛡️
All
LogErrorAndExitcalls across the codebase follow the standardized pattern without theatmosConfigparameter, maintaining robust error handling withfmt.Errorf.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining instances of LogErrorAndExit with atmosConfig rg "LogErrorAndExit\([^,)]+," --type goLength of output: 1879
internal/exec/workflow_utils.go (1)
43-43: Solid logging improvements!The logging changes maintain all necessary debug information while achieving a cleaner, standardized format. The workflow execution logs will be more consistent and readable.
Also applies to: 67-67, 100-100, 114-114, 115-115
internal/exec/vendor_model.go (1)
141-141: Clean logging standardization!The vendor model logging has been successfully standardized while preserving all essential error and progress information. The changes will provide better consistency in log output.
Also applies to: 154-161, 170-170
internal/exec/terraform_outputs.go (4)
80-81: Well-structured logging for backend and provider operations!The logging changes for backend and provider operations maintain clear visibility while achieving a more standardized format.
Also applies to: 103-104, 113-114, 122-123
139-139: Terraform operation logging looks great!The standardization of terraform operation logs (init, workspace, output) improves readability while maintaining all essential information. The debug level is appropriate for these operations.
Also applies to: 151-151, 154-164, 168-168, 173-173, 178-180
186-186: Clean variable conversion logging!The logging for variable conversion operations has been standardized while maintaining detailed debug information for troubleshooting.
Also applies to: 191-191, 194-194, 204-204
224-224: Error handling and output logging is solid!The error handling and output logging have been successfully standardized while maintaining robust error reporting and clear output information.
Also applies to: 231-231, 238-238, 249-249, 273-273, 299-299
internal/exec/helmfile.go (5)
71-71: LGTM! Clear and informative component status logging.The log message effectively communicates that a component is disabled and being skipped.
Line range hint
108-131: LGTM! Comprehensive debug logging for component variables.The debug logs provide clear visibility into:
- Component variables in the stack context
- Variable file path information
152-157: LGTM! Well-structured AWS configuration logging.The debug logs effectively capture:
- AWS profile being used
- Kubeconfig download details and path
187-213: LGTM! Thorough command execution logging.The debug logs provide comprehensive command execution details:
- Helmfile binary and command
- Global options and arguments
- Component and stack information
- Working directory
279-279: LGTM! Appropriate warning level for non-critical error.The warning log is correctly used for a non-critical file removal error.
internal/exec/shell_utils.go (4)
83-90: LGTM! Proper error handling with warning logs.The warning logs appropriately handle file operation errors:
- File opening errors
- File closing errors in defer function
97-98: LGTM! Consistent command execution logging.The debug logs maintain consistency across different command execution functions:
- ExecuteShellCommand
- ExecuteShell
- ExecuteShellAndReturnOutput
Also applies to: 122-123, 149-150
317-317: LGTM! Clear process lifecycle logging.The debug logs effectively track shell process lifecycle:
- Process start
- Process exit with state
Also applies to: 332-332
349-349: LGTM! Appropriate logging for environment setup.The logs handle:
- Warning for TTY support status
- Debug logging for environment variables
Also applies to: 378-378
internal/exec/terraform_generate_varfiles.go (2)
269-269: LGTM! Appropriate error handling for template processing.The error logging correctly handles template processing failures with program exit.
326-330: LGTM! Comprehensive varfile generation logging.The debug logs provide clear visibility into varfile generation:
- Varfile path
- Terraform component details
- Atmos component and stack information
- Stack config file path
internal/exec/vendor_component_utils.go (2)
127-127: LGTM! Detailed trace logging for file operations.The trace logs provide granular visibility into file operations:
- File exclusions based on patterns
- File inclusions based on patterns
- Default inclusion behavior
Also applies to: 145-145, 157-157, 163-163
351-351: LGTM! Clear warning for TTY support status.The warning log appropriately notifies about TTY support and non-interactive mode.
internal/exec/terraform_generate_backends.go (1)
330-330: LGTM!The debug message is clear and informative, providing good context about the backend config file being written.
pkg/config/config.go (1)
Line range hint
102-102: LGTM!The trace message clearly indicates that the default embedded schema is being used.
pkg/spacelift/spacelift_stack_processor.go (1)
33-33: LGTM!The error logging in
CreateSpaceliftStacksis consistent and appropriate for the context.Also applies to: 49-49, 73-73
internal/exec/validate_stacks.go (2)
102-102: LGTM!The trace message clearly indicates that the default embedded schema is being used.
142-142: LGTM!The debug message provides clear context about the validation process and exclusions.
internal/exec/terraform_clean.go (5)
54-56: LGTM! Appropriate error handling for subdirectory reading.The error is properly logged as a warning and the function continues execution, which is the correct behavior when encountering non-critical errors during directory traversal.
315-315: LGTM! Robust error handling for folder deletion.The implementation properly collects and logs all deletion errors while continuing to process remaining folders, providing good resilience against partial failures.
323-323: LGTM! Appropriate error handling for directory cleanup.The warning log is suitable for this non-critical cleanup operation, allowing the process to continue if an empty directory can't be removed.
414-423: LGTM! Appropriate use of LogTrace for detailed error information.The implementation correctly uses trace-level logging for detailed error information while still returning the error for higher-level handling.
433-437: LGTM! Consistent error handling pattern.The implementation maintains consistency with the previous error handling pattern, using trace-level logging for detailed error information.
internal/exec/vendor_utils.go (3)
175-177: Clean logging refactor!The simplified LogWarning call maintains the same functionality while reducing parameter coupling.
418-419: Great user experience message!Clear communication about TTY detection helps users understand why they might see different output formats. This is essential for CLI tools.
Line range hint
606-641: Well-structured debug logging!The detailed trace logging for file path matching provides excellent debugging context, making it easier to troubleshoot file inclusion/exclusion issues.
pkg/config/utils.go (1)
Line range hint
191-387: Excellent environment variable logging!The consistent pattern of logging environment variable detection provides clear visibility into configuration sourcing. This will be invaluable for debugging configuration issues.
cmd/pro_unlock.go (1)
22-22: Verify logging implementation in pkg/utils.The removal of
schema.AtmosConfiguration{}aligns with the migration to charmbracelet logger. However, we should verify thatLogErrorAndExitin pkg/utils has been updated to use the new logger.✅ Verification successful
LogErrorAndExit correctly implements charmbracelet logger
The function has been properly updated to use the new charmbracelet logger, imported as
l "github.com/charmbracelet/log". The implementation maintains all necessary functionality including error logging and exit code handling.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if LogErrorAndExit has been updated to use charmbracelet logger rg -A 5 "func LogErrorAndExit" pkg/utils/Length of output: 351
Script:
#!/bin/bash # Check logger initialization and imports rg -B 10 "func LogErrorAndExit" pkg/utils/log_utils.goLength of output: 427
Script:
#!/bin/bash # View complete file content cat pkg/utils/log_utils.goLength of output: 1471
cmd/describe_config.go (1)
19-19: LGTM! Clean and consistent change.The removal of
schema.AtmosConfiguration{}and cleanup of unused imports aligns with the logging migration.cmd/pro_lock.go (1)
22-22: LGTM! Well-structured command.The logging change is consistent with the migration pattern, and the command maintains good documentation and flag descriptions.
cmd/helmfile_generate_varfile.go (1)
22-22: LGTM! Clean error handling migration.The removal of the
schema.AtmosConfiguration{}parameter fromLogErrorAndExitcalls maintains the error handling functionality while aligning with the new logging approach.Also applies to: 34-34
cmd/terraform_generate_varfile.go (1)
22-22: LGTM! Consistent error handling migration.The changes align with the logging migration pattern seen across the codebase, maintaining error handling functionality.
Also applies to: 34-34
cmd/helmfile.go (1)
31-31: LGTM! Error handling migration complete.The removal of
schema.AtmosConfiguration{}fromLogErrorAndExitcalls maintains consistent error handling while simplifying the logging interface.Also applies to: 37-37, 46-46
internal/exec/helmfile_generate_varfile.go (1)
62-62: LGTM! Debug logging migration complete.The debug logging changes maintain clear output while aligning with the new logging approach. The use of
fmt.Sprintffor the first message ensures proper string formatting.Also applies to: 72-73
internal/exec/terraform_generate_backend.go (1)
61-61: LGTM! Debug logging migration complete.The debug logging changes maintain clear output while simplifying the interface. The backend configuration and file path logging remain informative.
Also applies to: 86-87
pkg/component/component_processor.go (1)
28-28: Consistent error logging refactor looks good!The simplification of error logging calls aligns well with the migration to charmbracelet logger. The error handling logic remains robust while reducing unnecessary parameter passing.
Also applies to: 38-38, 64-64, 70-70, 76-76
cmd/describe_stacks.go (1)
22-22: Clean logging refactor in command handler!The simplified error logging maintains the exit behavior while adapting to the new logging system.
cmd/aws_eks_update_kubeconfig.go (1)
38-38: Standardized error logging in EKS command!The simplified error logging maintains consistency with other command handlers while adapting to charmbracelet logger.
pkg/utils/hcl_utils.go (1)
Line range hint
1-1: Verify complete migration to new logging patternLet's verify that all logging calls have been migrated to the new pattern.
✅ Verification successful
Migration to new logging pattern is complete 💪
All logging calls have been successfully migrated to the new pattern. No instances of passing AtmosConfiguration to logging functions were found in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining old-style logging calls that might have been missed echo "Checking for potential missed logging migrations..." # Look for LogError calls with atmosConfig rg "LogError\(.*AtmosConfiguration" -l # Look for LogWarning calls with atmosConfig rg "LogWarning\(.*AtmosConfiguration" -l # Look for LogErrorAndExit calls with atmosConfig rg "LogErrorAndExit\(.*AtmosConfiguration" -lLength of output: 244
Script:
#!/bin/bash # Broader search for logging patterns echo "=== Checking all logging patterns ===" # Look for any logging function calls echo "Current logging patterns:" rg "Log(Error|Warning|ErrorAndExit)\(" -A 2 # Look for any remaining atmosConfig references near logging echo -e "\nChecking for atmosConfig near logging statements:" rg -B 2 -A 2 "(?i)log.*atmos"Length of output: 69137
cmd/cmd_utils.go (4)
142-148: Clean error handling refactor!The simplified error handling maintains the same functionality while aligning with the new logging approach.
184-188: Good error handling for invalid command scenarios!The logging changes maintain clear error reporting while following the new logging pattern.
Line range hint
220-235: Solid argument validation with clear error messages!The error handling properly validates required arguments and provides helpful error messages.
Line range hint
299-537: Consistent logging refactor across the file!The logging changes maintain proper error handling, debug information, and user feedback while transitioning to the new logging system. The different log levels (error, debug, warning) are used appropriately throughout the code.
internal/exec/terraform.go (4)
Line range hint
60-85: Clean debug logging in terraform configuration generation!The debug logging for backend configuration and provider overrides is clear and helpful for troubleshooting.
Line range hint
150-189: Well-structured logging for component processing!The logging changes maintain clear visibility into component processing, configuration validation, and cleanup operations.
Line range hint
201-231: Appropriate debug logging for variable processing!The debug logging provides good visibility into variable processing and file operations.
Line range hint
274-379: Comprehensive logging for terraform operations!The logging changes effectively cover environment variables, initialization, and command execution while maintaining proper debug levels.
internal/exec/describe_stacks.go (1)
385-385: Consistent error handling in template processing!The error handling for template processing maintains proper error reporting while aligning with the new logging approach.
Also applies to: 576-576
internal/exec/utils.go (1)
318-318: Robust error handling in stack processing!The logging changes maintain proper error reporting and tracing while processing stacks, with clear error messages for missing components and duplicate configurations.
Also applies to: 427-441, 523-535
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/utils/log_utils.go (3)
10-10: Consider using a more descriptive import alias.The alias
lfor the charmbracelet logger is quite terse. Consider using a more descriptive alias likelogorclogto improve code readability.-l "github.com/charmbracelet/log" +log "github.com/charmbracelet/log"
51-58: Clean implementation with good debug support!The function effectively handles error logging with debug-level stack traces. However, there's an unnecessary empty line that could be removed.
func LogError(err error) { - l.Error(err)
62-63: Document the LogTrace to LogDebug redirection.Consider adding a comment explaining why LogTrace redirects to LogDebug. This helps future maintainers understand if this is temporary or intentional behavior.
// LogTrace logs the provided trace message +// Note: Currently redirects to LogDebug as charmbracelet logger doesn't support TRACE level func LogTrace(message string) { LogDebug(message) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
cmd/atlantis_generate_repo_config.go(1 hunks)cmd/cmd_utils.go(12 hunks)cmd/completion.go(1 hunks)cmd/describe_affected.go(1 hunks)cmd/describe_config.go(1 hunks)cmd/describe_stacks.go(1 hunks)cmd/describe_workflows.go(1 hunks)cmd/docs.go(4 hunks)cmd/helmfile.go(1 hunks)cmd/helmfile_generate_varfile.go(2 hunks)cmd/pro_lock.go(1 hunks)cmd/pro_unlock.go(1 hunks)cmd/root.go(5 hunks)cmd/terraform.go(1 hunks)cmd/terraform_generate_backend.go(2 hunks)cmd/terraform_generate_backends.go(1 hunks)cmd/terraform_generate_varfile.go(2 hunks)cmd/terraform_generate_varfiles.go(2 hunks)cmd/validate_component.go(2 hunks)cmd/validate_stacks.go(1 hunks)cmd/vendor_diff.go(1 hunks)cmd/vendor_pull.go(1 hunks)cmd/version.go(2 hunks)cmd/workflow.go(5 hunks)internal/exec/describe_affected_utils.go(12 hunks)internal/exec/describe_stacks.go(2 hunks)internal/exec/file_utils.go(1 hunks)internal/exec/helmfile.go(7 hunks)internal/exec/terraform.go(11 hunks)internal/exec/terraform_outputs.go(12 hunks)internal/exec/utils.go(5 hunks)internal/exec/validate_stacks.go(2 hunks)internal/exec/vendor_component_utils.go(4 hunks)internal/exec/vendor_model.go(3 hunks)internal/exec/vendor_utils.go(6 hunks)internal/exec/yaml_func_store.go(1 hunks)pkg/config/config.go(3 hunks)pkg/utils/hcl_utils.go(2 hunks)pkg/utils/json_utils.go(1 hunks)pkg/utils/log_utils.go(2 hunks)pkg/utils/yaml_utils.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (40)
- cmd/helmfile_generate_varfile.go
- cmd/atlantis_generate_repo_config.go
- cmd/pro_unlock.go
- cmd/terraform_generate_backend.go
- cmd/helmfile.go
- cmd/pro_lock.go
- cmd/describe_stacks.go
- pkg/utils/yaml_utils.go
- pkg/utils/hcl_utils.go
- internal/exec/validate_stacks.go
- cmd/describe_affected.go
- internal/exec/yaml_func_store.go
- cmd/terraform.go
- cmd/vendor_diff.go
- cmd/docs.go
- internal/exec/file_utils.go
- cmd/terraform_generate_varfile.go
- cmd/root.go
- internal/exec/vendor_model.go
- internal/exec/helmfile.go
- cmd/completion.go
- cmd/validate_stacks.go
- internal/exec/vendor_utils.go
- cmd/validate_component.go
- cmd/describe_workflows.go
- cmd/terraform_generate_varfiles.go
- pkg/utils/json_utils.go
- cmd/vendor_pull.go
- internal/exec/terraform_outputs.go
- cmd/version.go
- cmd/workflow.go
- cmd/terraform_generate_backends.go
- pkg/config/config.go
- cmd/describe_config.go
- internal/exec/utils.go
- internal/exec/terraform.go
- internal/exec/vendor_component_utils.go
- internal/exec/describe_stacks.go
- internal/exec/describe_affected_utils.go
- cmd/cmd_utils.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (2)
pkg/utils/log_utils.go (2)
38-47: Clean and robust error handling implementation!The function effectively:
- Logs errors using the new logger
- Properly extracts exit codes from exec.ExitError
- Falls back to exit code 1 for other errors
67-78: Well-structured logging functions!The implementations are clean, consistent, and follow a clear pattern using the charmbracelet logger.
mcalhoun
force-pushed
the
dev-2668-migrate-to-charmbracelet-logger
branch
from
962bb78 to
181d188
Compare
mcalhoun
force-pushed
the
dev-2668-migrate-to-charmbracelet-logger
branch
from
181d188 to
3178e58
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
internal/exec/yaml_func_include.go (1)
Line range hint
31-31: Fix missed logging call migration.This logging call still uses the old signature with
atmosConfigparameter while all others have been migrated.Apply this diff to fix the inconsistency:
- u.LogErrorAndExit(atmosConfig, e) + u.LogErrorAndExit(e)
🧹 Nitpick comments (7)
cmd/root.go (1)
95-122: Solid logger setup implementation!The function effectively:
- Maps log levels to charmbracelet logger levels
- Handles file output configuration
- Maintains file handle open for program duration (as intended)
One suggestion to consider: add error logging for the "Off" case to inform users when logging is completely disabled.
case "Off": + log.Info("logging disabled, redirecting output to io.Discard") log.SetOutput(io.Discard)cmd/cmd_utils.go (6)
143-143: Consider consolidating error handling in processCommandAliases.The error handling pattern could be simplified by combining the flag parsing and command execution into a single error check.
-err := cmd.ParseFlags(args) -if err != nil { - u.LogErrorAndExit(err) -} - -commandToRun := fmt.Sprintf("%s %s %s", os.Args[0], aliasCmd, strings.Join(args, " ")) -err = e.ExecuteShell(atmosConfig, commandToRun, commandToRun, ".", nil, false) -if err != nil { - u.LogErrorAndExit(err) -} +if err := cmd.ParseFlags(args); err != nil { + u.LogErrorAndExit(err) +} + +commandToRun := fmt.Sprintf("%s %s %s", os.Args[0], aliasCmd, strings.Join(args, " ")) +if err := e.ExecuteShell(atmosConfig, commandToRun, commandToRun, ".", nil, false); err != nil { + u.LogErrorAndExit(err) +}Also applies to: 149-149
185-192: Consider using structured logging for command availability.When showing available commands, structured logging would make the output more readable and consistent with the charmbracelet logger migration.
-u.LogInfo(sb.String()) +u.LogInfo("Available Commands", "commands", sb.String()) -u.LogError(errors.New( - "invalid command: no args, no steps, no sub-commands", -)) +u.LogError(errors.New("invalid command configuration"), + "reason", "no args, no steps, no sub-commands")
324-344: Enhance error messages for component configuration.The error messages for component configuration could be more descriptive and structured.
-u.LogErrorAndExit(err) +u.LogErrorAndExit(fmt.Errorf("failed to process component config template: %w", err)) -u.LogErrorAndExit(fmt.Errorf("the command defines an invalid 'component_config.component: %s' in '%s'", - commandConfig.ComponentConfig.Component, cfg.CliConfigFileName+u.DefaultStackConfigFileExtension)) +u.LogErrorAndExit(fmt.Errorf("invalid component configuration"), + "component", commandConfig.ComponentConfig.Component, + "config_file", cfg.CliConfigFileName+u.DefaultStackConfigFileExtension)
388-391: Improve debug logging for environment variables.The debug logging for environment variables could be more structured and informative.
-u.LogDebug("\nUsing ENV vars:") -for _, v := range envVarsList { - u.LogDebug(v) -} +u.LogDebug("Environment Variables", + "count", len(envVarsList), + "variables", envVarsList)
504-506: Enhance error handling in version check.The version check error handling could provide more context about the failure.
-u.LogWarning(fmt.Sprintf("Could not load cache: %s", err)) +u.LogWarning("Version check cache operation failed", + "operation", "load", + "error", err) -u.LogWarning(fmt.Sprintf("Failed to retrieve latest Atmos release info: %s", err)) +u.LogWarning("Version check GitHub operation failed", + "operation", "fetch_release", + "error", err) -u.LogWarning("No release information available") +u.LogWarning("Version check failed", + "reason", "no_release_info", + "repository", "cloudposse/atmos")Also applies to: 517-523
537-540: Fix formatting in cache error handling.There's an extra newline in the error message that affects readability.
-u.LogWarning(fmt.Sprintf("Unable to save cache: %s", saveErr)) - +u.LogWarning(fmt.Sprintf("Unable to save cache: %s", saveErr))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (76)
cmd/atlantis_generate_repo_config.go(1 hunks)cmd/aws_eks_update_kubeconfig.go(1 hunks)cmd/cmd_utils.go(12 hunks)cmd/completion.go(1 hunks)cmd/describe_affected.go(1 hunks)cmd/describe_component.go(2 hunks)cmd/describe_config.go(1 hunks)cmd/describe_dependents.go(2 hunks)cmd/describe_stacks.go(1 hunks)cmd/describe_workflows.go(1 hunks)cmd/docs.go(4 hunks)cmd/helmfile.go(1 hunks)cmd/helmfile_generate_varfile.go(2 hunks)cmd/pro_lock.go(1 hunks)cmd/pro_unlock.go(1 hunks)cmd/root.go(5 hunks)cmd/terraform.go(1 hunks)cmd/terraform_generate_backend.go(2 hunks)cmd/terraform_generate_backends.go(1 hunks)cmd/terraform_generate_varfile.go(2 hunks)cmd/terraform_generate_varfiles.go(2 hunks)cmd/validate_component.go(2 hunks)cmd/validate_stacks.go(1 hunks)cmd/vendor_diff.go(1 hunks)cmd/vendor_pull.go(1 hunks)cmd/version.go(2 hunks)cmd/workflow.go(5 hunks)internal/exec/atlantis_generate_repo_config.go(1 hunks)internal/exec/aws_eks_update_kubeconfig.go(1 hunks)internal/exec/describe_affected_utils.go(13 hunks)internal/exec/describe_stacks.go(2 hunks)internal/exec/file_utils.go(1 hunks)internal/exec/go_getter_utils.go(2 hunks)internal/exec/helmfile.go(7 hunks)internal/exec/helmfile_generate_varfile.go(2 hunks)internal/exec/oci_utils.go(1 hunks)internal/exec/shell_utils.go(10 hunks)internal/exec/tar_utils.go(2 hunks)internal/exec/template_funcs_component.go(2 hunks)internal/exec/template_funcs_gomplate_datasource.go(3 hunks)internal/exec/template_utils.go(3 hunks)internal/exec/terraform.go(11 hunks)internal/exec/terraform_clean.go(6 hunks)internal/exec/terraform_generate_backend.go(2 hunks)internal/exec/terraform_generate_backends.go(2 hunks)internal/exec/terraform_generate_varfile.go(2 hunks)internal/exec/terraform_generate_varfiles.go(2 hunks)internal/exec/terraform_outputs.go(14 hunks)internal/exec/terraform_utils.go(1 hunks)internal/exec/utils.go(5 hunks)internal/exec/validate_component.go(2 hunks)internal/exec/validate_stacks.go(2 hunks)internal/exec/vendor_component_utils.go(4 hunks)internal/exec/vendor_model.go(3 hunks)internal/exec/vendor_utils.go(6 hunks)internal/exec/workflow_utils.go(4 hunks)internal/exec/yaml_func_env.go(3 hunks)internal/exec/yaml_func_exec.go(1 hunks)internal/exec/yaml_func_include.go(3 hunks)internal/exec/yaml_func_store.go(1 hunks)internal/exec/yaml_func_template.go(1 hunks)internal/exec/yaml_func_terraform_output.go(2 hunks)main.go(1 hunks)pkg/aws/aws_eks_update_kubeconfig.go(1 hunks)pkg/component/component_processor.go(3 hunks)pkg/config/cache.go(1 hunks)pkg/config/config.go(3 hunks)pkg/config/utils.go(8 hunks)pkg/hooks/cmd.go(3 hunks)pkg/spacelift/spacelift_stack_processor.go(9 hunks)pkg/utils/hcl_utils.go(2 hunks)pkg/utils/json_utils.go(1 hunks)pkg/utils/log_utils.go(2 hunks)pkg/utils/yaml_utils.go(1 hunks)pkg/validate/validate_component_test.go(4 hunks)pkg/validate/validate_stacks_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal/exec/go_getter_utils.go
🚧 Files skipped from review as they are similar to previous changes (69)
- cmd/helmfile.go
- cmd/aws_eks_update_kubeconfig.go
- cmd/workflow.go
- pkg/utils/yaml_utils.go
- cmd/terraform_generate_varfile.go
- internal/exec/aws_eks_update_kubeconfig.go
- cmd/vendor_diff.go
- internal/exec/template_funcs_component.go
- internal/exec/terraform_generate_backend.go
- cmd/describe_stacks.go
- cmd/pro_unlock.go
- internal/exec/yaml_func_terraform_output.go
- main.go
- cmd/atlantis_generate_repo_config.go
- internal/exec/describe_stacks.go
- internal/exec/yaml_func_store.go
- internal/exec/terraform_generate_varfile.go
- pkg/config/cache.go
- pkg/validate/validate_component_test.go
- internal/exec/terraform_generate_backends.go
- cmd/validate_component.go
- internal/exec/terraform_utils.go
- pkg/component/component_processor.go
- cmd/pro_lock.go
- pkg/utils/hcl_utils.go
- pkg/utils/json_utils.go
- cmd/helmfile_generate_varfile.go
- cmd/docs.go
- cmd/terraform_generate_backends.go
- internal/exec/terraform_outputs.go
- cmd/describe_config.go
- cmd/terraform_generate_varfiles.go
- internal/exec/vendor_component_utils.go
- cmd/vendor_pull.go
- internal/exec/terraform_generate_varfiles.go
- internal/exec/vendor_model.go
- cmd/describe_affected.go
- internal/exec/oci_utils.go
- pkg/config/utils.go
- internal/exec/utils.go
- internal/exec/helmfile_generate_varfile.go
- internal/exec/template_funcs_gomplate_datasource.go
- internal/exec/validate_stacks.go
- pkg/hooks/cmd.go
- internal/exec/describe_affected_utils.go
- internal/exec/vendor_utils.go
- internal/exec/validate_component.go
- internal/exec/terraform.go
- internal/exec/yaml_func_template.go
- internal/exec/shell_utils.go
- cmd/terraform.go
- cmd/describe_dependents.go
- internal/exec/yaml_func_exec.go
- cmd/completion.go
- pkg/config/config.go
- internal/exec/terraform_clean.go
- cmd/terraform_generate_backend.go
- internal/exec/file_utils.go
- pkg/validate/validate_stacks_test.go
- cmd/validate_stacks.go
- pkg/aws/aws_eks_update_kubeconfig.go
- pkg/spacelift/spacelift_stack_processor.go
- internal/exec/tar_utils.go
- cmd/version.go
- cmd/describe_workflows.go
- internal/exec/atlantis_generate_repo_config.go
- internal/exec/template_utils.go
- internal/exec/helmfile.go
- internal/exec/workflow_utils.go
🧰 Additional context used
📓 Learnings (1)
cmd/cmd_utils.go (1)
Learnt from: osterman
PR: cloudposse/atmos#740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-11-12T05:52:05.088Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (7)
cmd/root.go (3)
6-6: Clean import additions!The new imports for
ioandcharmbracelet/logare well-organized and necessary for the logging migration.Also applies to: 11-11
85-85: Consistent error logging refactor!The removal of
schema.AtmosConfiguration{}parameter fromLogErrorAndExitcalls streamlines the error handling across the codebase. This change aligns well with the migration to charmbracelet logger.Also applies to: 90-90, 140-140, 152-152, 157-157, 201-201, 233-233, 236-236, 241-241, 245-245
144-145: Well-placed logger initialization!The logger setup is correctly positioned after config initialization, ensuring proper configuration of log levels and output destinations.
internal/exec/yaml_func_env.go (1)
18-18: LGTM! Clean migration of logging calls.The removal of
atmosConfigparameter from logging calls is consistent with the migration to charmbracelet logger. The error messages remain descriptive and helpful.Also applies to: 22-22, 32-32, 44-44
internal/exec/yaml_func_include.go (1)
19-19: LGTM! Clean migration of other logging calls.The remaining logging calls have been correctly migrated to the new signature.
Also applies to: 22-22, 42-42, 55-55, 61-61
pkg/utils/log_utils.go (1)
38-78: LGTM! Clean implementation of logging functions.The migration to charmbracelet logger is well-implemented:
- Good error handling with exit codes
- Nice addition of stack trace for debug level
- Clean function signatures
cmd/describe_component.go (1)
22-22: LGTM! Clean migration and import cleanup.The logging calls have been correctly migrated and the unused schema import has been removed.
Also applies to: 36-36
mcalhoun
force-pushed
the
dev-2668-migrate-to-charmbracelet-logger
branch
from
3178e58 to
adf0cb0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/utils/log_utils.go (3)
38-47: Consider adding context to error messages.While the error handling is solid, consider wrapping errors with additional context to make debugging easier.
func LogErrorAndExit(err error) { - l.Error(err) + l.Error("execution failed", "error", err) // Find the executed command's exit code from the error
62-63: Consider implementing distinct trace level logging.Currently, trace logs are redirected to debug level. For more granular control, consider implementing true trace-level logging.
func LogTrace(message string) { - LogDebug(message) + l.SetLevel(l.DebugLevel) + l.Debug(message, "level", "trace") }
67-78: Consider enhancing logs with structured fields.The logging functions are clean and consistent. Consider adding support for structured logging to enhance log analysis capabilities.
Example enhancement:
func LogInfo(message string, fields ...interface{}) { l.Info(message, fields...) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (76)
cmd/atlantis_generate_repo_config.go(1 hunks)cmd/aws_eks_update_kubeconfig.go(1 hunks)cmd/cmd_utils.go(12 hunks)cmd/completion.go(1 hunks)cmd/describe_affected.go(1 hunks)cmd/describe_component.go(2 hunks)cmd/describe_config.go(1 hunks)cmd/describe_dependents.go(2 hunks)cmd/describe_stacks.go(1 hunks)cmd/describe_workflows.go(1 hunks)cmd/docs.go(4 hunks)cmd/helmfile.go(1 hunks)cmd/helmfile_generate_varfile.go(2 hunks)cmd/pro_lock.go(1 hunks)cmd/pro_unlock.go(1 hunks)cmd/root.go(5 hunks)cmd/terraform.go(1 hunks)cmd/terraform_generate_backend.go(2 hunks)cmd/terraform_generate_backends.go(1 hunks)cmd/terraform_generate_varfile.go(2 hunks)cmd/terraform_generate_varfiles.go(2 hunks)cmd/validate_component.go(2 hunks)cmd/validate_stacks.go(1 hunks)cmd/vendor_diff.go(1 hunks)cmd/vendor_pull.go(1 hunks)cmd/version.go(2 hunks)cmd/workflow.go(5 hunks)internal/exec/atlantis_generate_repo_config.go(1 hunks)internal/exec/aws_eks_update_kubeconfig.go(1 hunks)internal/exec/describe_affected_utils.go(13 hunks)internal/exec/describe_stacks.go(2 hunks)internal/exec/file_utils.go(1 hunks)internal/exec/go_getter_utils.go(2 hunks)internal/exec/helmfile.go(7 hunks)internal/exec/helmfile_generate_varfile.go(2 hunks)internal/exec/oci_utils.go(1 hunks)internal/exec/shell_utils.go(10 hunks)internal/exec/tar_utils.go(2 hunks)internal/exec/template_funcs_component.go(2 hunks)internal/exec/template_funcs_gomplate_datasource.go(3 hunks)internal/exec/template_utils.go(3 hunks)internal/exec/terraform.go(11 hunks)internal/exec/terraform_clean.go(6 hunks)internal/exec/terraform_generate_backend.go(2 hunks)internal/exec/terraform_generate_backends.go(2 hunks)internal/exec/terraform_generate_varfile.go(2 hunks)internal/exec/terraform_generate_varfiles.go(2 hunks)internal/exec/terraform_outputs.go(14 hunks)internal/exec/terraform_utils.go(1 hunks)internal/exec/utils.go(5 hunks)internal/exec/validate_component.go(2 hunks)internal/exec/validate_stacks.go(2 hunks)internal/exec/vendor_component_utils.go(4 hunks)internal/exec/vendor_model.go(3 hunks)internal/exec/vendor_utils.go(6 hunks)internal/exec/workflow_utils.go(4 hunks)internal/exec/yaml_func_env.go(3 hunks)internal/exec/yaml_func_exec.go(1 hunks)internal/exec/yaml_func_include.go(3 hunks)internal/exec/yaml_func_store.go(1 hunks)internal/exec/yaml_func_template.go(1 hunks)internal/exec/yaml_func_terraform_output.go(2 hunks)main.go(1 hunks)pkg/aws/aws_eks_update_kubeconfig.go(1 hunks)pkg/component/component_processor.go(3 hunks)pkg/config/cache.go(1 hunks)pkg/config/config.go(3 hunks)pkg/config/utils.go(8 hunks)pkg/hooks/cmd.go(3 hunks)pkg/spacelift/spacelift_stack_processor.go(9 hunks)pkg/utils/hcl_utils.go(2 hunks)pkg/utils/json_utils.go(1 hunks)pkg/utils/log_utils.go(2 hunks)pkg/utils/yaml_utils.go(1 hunks)pkg/validate/validate_component_test.go(4 hunks)pkg/validate/validate_stacks_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (74)
- internal/exec/terraform_generate_varfile.go
- cmd/describe_stacks.go
- internal/exec/yaml_func_template.go
- pkg/component/component_processor.go
- internal/exec/yaml_func_exec.go
- internal/exec/yaml_func_store.go
- internal/exec/yaml_func_include.go
- internal/exec/atlantis_generate_repo_config.go
- internal/exec/file_utils.go
- cmd/atlantis_generate_repo_config.go
- internal/exec/template_funcs_gomplate_datasource.go
- cmd/helmfile_generate_varfile.go
- cmd/workflow.go
- cmd/pro_lock.go
- cmd/vendor_diff.go
- cmd/terraform_generate_varfile.go
- cmd/describe_config.go
- pkg/validate/validate_component_test.go
- main.go
- cmd/pro_unlock.go
- cmd/describe_workflows.go
- cmd/validate_component.go
- cmd/helmfile.go
- cmd/describe_dependents.go
- pkg/validate/validate_stacks_test.go
- internal/exec/template_utils.go
- pkg/utils/json_utils.go
- internal/exec/oci_utils.go
- cmd/describe_component.go
- cmd/completion.go
- cmd/aws_eks_update_kubeconfig.go
- internal/exec/yaml_func_terraform_output.go
- pkg/utils/yaml_utils.go
- internal/exec/yaml_func_env.go
- internal/exec/terraform_outputs.go
- pkg/utils/hcl_utils.go
- internal/exec/workflow_utils.go
- internal/exec/aws_eks_update_kubeconfig.go
- cmd/validate_stacks.go
- internal/exec/terraform_generate_backends.go
- internal/exec/helmfile.go
- internal/exec/template_funcs_component.go
- internal/exec/helmfile_generate_varfile.go
- internal/exec/describe_stacks.go
- cmd/terraform_generate_varfiles.go
- internal/exec/vendor_component_utils.go
- cmd/version.go
- cmd/docs.go
- internal/exec/validate_stacks.go
- pkg/spacelift/spacelift_stack_processor.go
- internal/exec/terraform_generate_backend.go
- internal/exec/utils.go
- pkg/config/cache.go
- internal/exec/tar_utils.go
- internal/exec/terraform_generate_varfiles.go
- pkg/aws/aws_eks_update_kubeconfig.go
- cmd/terraform.go
- pkg/config/config.go
- cmd/cmd_utils.go
- pkg/config/utils.go
- internal/exec/shell_utils.go
- cmd/describe_affected.go
- internal/exec/terraform.go
- internal/exec/go_getter_utils.go
- internal/exec/terraform_utils.go
- internal/exec/terraform_clean.go
- internal/exec/vendor_model.go
- internal/exec/describe_affected_utils.go
- internal/exec/validate_component.go
- pkg/hooks/cmd.go
- cmd/terraform_generate_backends.go
- internal/exec/vendor_utils.go
- cmd/vendor_pull.go
- cmd/terraform_generate_backend.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (7)
pkg/utils/log_utils.go (2)
10-10: Initialize charmbracelet logger.The logger is imported but not initialized with default settings.
Consider adding initialization code to set up the logger with appropriate defaults:
func init() { l.SetLevel(l.InfoLevel) l.SetReportCaller(true) l.SetFormatter(l.TextFormatter) }
51-58: Well structured error logging with debug support!The debug stack trace integration is a great addition. The code effectively uses the logger's level checking.
cmd/root.go (5)
6-6: Clean import organization!The new imports for
ioandcharmbracelet/logare well-organized and properly support the logging migration.Also applies to: 11-11
85-85: Streamlined error handling!The simplified
LogErrorAndExitcalls maintain proper error handling while aligning with the new logging approach.Also applies to: 90-90
95-122: Robust logger setup implementation!The logger configuration handles all log levels appropriately and maintains the intentional file handle management pattern as previously discussed. The implementation properly maps Atmos log levels to charmbracelet logger levels.
Note: As discussed in previous reviews, keeping the log file open for the entire program execution is intentional for efficient logging.
138-138: Well-placed logger initialization!The logger setup is strategically positioned after config initialization but before command processing, ensuring proper logging configuration for all subsequent operations.
Also applies to: 140-140, 144-146
152-152: Consistent error handling throughout!The error handling modifications maintain a uniform pattern across all functions, providing a cohesive logging implementation.
Also applies to: 157-157, 201-201, 233-233, 236-236, 241-241, 245-245
mcalhoun
force-pushed
the
dev-2668-migrate-to-charmbracelet-logger
branch
from
adf0cb0 to
6135ede
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (11)
cmd/validate_editorconfig.go (1)
96-97: Enhance format validation error messages.The error messages for invalid format could be more descriptive. Consider including the current format value in the error message.
- u.LogErrorAndExit(fmt.Errorf("%v is not a valid format choose from the following: %v", atmosConfig.Validate.EditorConfig.Format, outputformat.GetArgumentChoiceText())) + u.LogErrorAndExit(fmt.Errorf("invalid format '%s'. Valid formats are: %s", atmosConfig.Validate.EditorConfig.Format, outputformat.GetArgumentChoiceText()))Also applies to: 102-103
pkg/utils/log_utils.go (2)
38-47: Enhance error handling in LogErrorAndExit.Consider adding more specific error type checks before falling back to exit code 1.
func LogErrorAndExit(err error) { l.Error(err) // Find the executed command's exit code from the error var exitError *exec.ExitError if errors.As(err, &exitError) { os.Exit(exitError.ExitCode()) } + + // Check for other common error types + var pathError *os.PathError + if errors.As(err, &pathError) { + os.Exit(2) + } os.Exit(1) }
51-58: Consider adding structured logging fields.The charmbracelet logger supports structured logging. Consider adding relevant fields for better log analysis.
func LogError(err error) { - l.Error(err) + l.Error("error occurred", "error", err) // Print stack trace if l.GetLevel() == l.DebugLevel { debug.PrintStack() } }cmd/root.go (2)
89-102: Make log level mapping more explicit.Consider using a map for log level mapping to make the code more maintainable.
+var logLevelMap = map[string]log.Level{ + "Trace": log.DebugLevel, + "Debug": log.DebugLevel, + "Info": log.InfoLevel, + "Warning": log.WarnLevel, +} + func setupLogger(atmosConfig *schema.AtmosConfiguration) { - switch atmosConfig.Logs.Level { - case "Trace": - log.SetLevel(log.DebugLevel) - case "Debug": - log.SetLevel(log.DebugLevel) - case "Info": - log.SetLevel(log.InfoLevel) - case "Warning": - log.SetLevel(log.WarnLevel) - case "Off": + if atmosConfig.Logs.Level == "Off" { log.SetOutput(io.Discard) - default: - log.SetLevel(log.InfoLevel) + return + } + + if level, ok := logLevelMap[atmosConfig.Logs.Level]; ok { + log.SetLevel(level) + } else { + log.SetLevel(log.InfoLevel) }
108-114: Add error handling for log file permissions.Consider checking file permissions before opening the log file.
log.Info("setting log file", "file", atmosConfig.Logs.File) + // Check if we have write permissions + if err := checkFilePermissions(atmosConfig.Logs.File); err != nil { + log.Fatal("Failed to access log file:", err) + } logFile, err := os.OpenFile(atmosConfig.Logs.File, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0666) if err != nil { log.Fatal("Failed to open log file:", err) } defer logFile.Close() log.SetOutput(logFile)pkg/config/config.go (1)
394-394: Fix string formatting in warning message.The current string concatenation could be replaced with fmt.Sprintf for better readability and safety.
- u.LogWarning(fmt.Sprintf("error closing file '" + configPath + "'. " + err.Error())) + u.LogWarning(fmt.Sprintf("error closing file '%s': %v", configPath, err))🧰 Tools
🪛 golangci-lint (1.62.2)
394-394: printf: non-constant format string in call to fmt.Sprintf
(govet)
cmd/cmd_utils.go (3)
185-192: Consider using a custom error type for command validation.The error handling here mixes direct logging with os.Exit. Consider creating a custom error type for command validation to make it more testable and maintainable.
+type CommandValidationError struct { + message string + code int +} + +func (e *CommandValidationError) Error() string { + return e.message +} if len(commandConfig.Commands) > 0 { sb.WriteString("Available command(s):\n") for i, c := range commandConfig.Commands { sb.WriteString( fmt.Sprintf("%d. %s %s %s\n", i+1, parentCommand.Use, commandConfig.Name, c.Name), ) } - u.LogInfo(sb.String()) - os.Exit(1) + return &CommandValidationError{message: sb.String(), code: 1} } else { - u.LogError(errors.New( + return &CommandValidationError{message: "invalid command: no args, no steps, no sub-commands", - )) - os.Exit(1) + code: 1} }
388-391: Consider using structured logging for debug information.The debug logging could benefit from structured fields to make it easier to parse and filter logs.
-u.LogDebug("\nUsing ENV vars:") -for _, v := range envVarsList { - u.LogDebug(v) -} +u.LogDebugWithFields(map[string]interface{}{ + "message": "Using ENV vars", + "vars": envVarsList, +})
Line range hint
504-538: Enhance error handling in version check logic.The version check logic has multiple early returns with warnings. Consider consolidating the error handling to make the flow more predictable.
func CheckForAtmosUpdateAndPrintMessage(atmosConfig schema.AtmosConfiguration) { + if !shouldPerformVersionCheck(atmosConfig) { + return + } + + result, err := performVersionCheck(atmosConfig) + if err != nil { + u.LogWarning(fmt.Sprintf("Version check failed: %s", err)) + return + } + + if result.shouldUpdate { + u.PrintMessageToUpgradeToAtmosLatestRelease(result.latestVersion) + } } +type versionCheckResult struct { + shouldUpdate bool + latestVersion string +} + +func shouldPerformVersionCheck(atmosConfig schema.AtmosConfiguration) bool { if !atmosConfig.Version.Check.Enabled { return false } + return true +}internal/exec/describe_affected_utils.go (2)
451-451: Optimize string formatting in trace logging.The static analysis tool correctly identified unnecessary use of
fmt.Sprintf. Since these are simple string values, direct string concatenation would be more efficient.-u.LogTrace(fmt.Sprintf("\nGetting current working repo commit object...")) +u.LogTrace("\nGetting current working repo commit object...") -u.LogTrace(fmt.Sprintf("Skipping disabled component %s", componentName)) +u.LogTrace("Skipping disabled component " + componentName)Also applies to: 458-458, 1528-1528
🧰 Tools
🪛 golangci-lint (1.62.2)
451-451: S1039: unnecessary use of fmt.Sprintf
(gosimple)
494-503: Consider adding file change statistics in trace logs.The trace logging for changed files could be enhanced with additional statistics to help with debugging.
if len(patch.Stats()) > 0 { - u.LogTrace(fmt.Sprintf("Found difference between the current working branch and remote target branch")) - u.LogTrace("\nChanged files:\n") + u.LogTrace(fmt.Sprintf("Found %d changed files between the current working branch and remote target branch", len(patch.Stats()))) + u.LogTrace("\nChanged files:") for _, fileStat := range patch.Stats() { - u.LogTrace(fileStat.Name) + u.LogTrace(fmt.Sprintf("- %s (additions: %d, deletions: %d)", fileStat.Name, fileStat.Addition, fileStat.Deletion)) changedFiles = append(changedFiles, fileStat.Name) } u.LogTrace("") }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (77)
cmd/atlantis_generate_repo_config.go(1 hunks)cmd/aws_eks_update_kubeconfig.go(1 hunks)cmd/cmd_utils.go(13 hunks)cmd/completion.go(1 hunks)cmd/describe_affected.go(1 hunks)cmd/describe_component.go(2 hunks)cmd/describe_config.go(1 hunks)cmd/describe_dependents.go(2 hunks)cmd/describe_stacks.go(1 hunks)cmd/describe_workflows.go(1 hunks)cmd/docs.go(5 hunks)cmd/helmfile.go(1 hunks)cmd/helmfile_generate_varfile.go(2 hunks)cmd/pro_lock.go(1 hunks)cmd/pro_unlock.go(1 hunks)cmd/root.go(6 hunks)cmd/terraform.go(1 hunks)cmd/terraform_generate_backend.go(2 hunks)cmd/terraform_generate_backends.go(1 hunks)cmd/terraform_generate_varfile.go(2 hunks)cmd/terraform_generate_varfiles.go(2 hunks)cmd/validate_component.go(2 hunks)cmd/validate_editorconfig.go(3 hunks)cmd/validate_stacks.go(1 hunks)cmd/vendor_diff.go(1 hunks)cmd/vendor_pull.go(1 hunks)cmd/version.go(2 hunks)cmd/workflow.go(5 hunks)internal/exec/atlantis_generate_repo_config.go(1 hunks)internal/exec/aws_eks_update_kubeconfig.go(1 hunks)internal/exec/describe_affected_utils.go(13 hunks)internal/exec/describe_stacks.go(2 hunks)internal/exec/file_utils.go(1 hunks)internal/exec/go_getter_utils.go(2 hunks)internal/exec/helmfile.go(7 hunks)internal/exec/helmfile_generate_varfile.go(2 hunks)internal/exec/oci_utils.go(1 hunks)internal/exec/shell_utils.go(10 hunks)internal/exec/tar_utils.go(2 hunks)internal/exec/template_funcs_component.go(2 hunks)internal/exec/template_funcs_gomplate_datasource.go(3 hunks)internal/exec/template_utils.go(3 hunks)internal/exec/terraform.go(11 hunks)internal/exec/terraform_clean.go(6 hunks)internal/exec/terraform_generate_backend.go(2 hunks)internal/exec/terraform_generate_backends.go(2 hunks)internal/exec/terraform_generate_varfile.go(2 hunks)internal/exec/terraform_generate_varfiles.go(2 hunks)internal/exec/terraform_outputs.go(14 hunks)internal/exec/terraform_utils.go(1 hunks)internal/exec/utils.go(5 hunks)internal/exec/validate_component.go(2 hunks)internal/exec/validate_stacks.go(2 hunks)internal/exec/vendor_component_utils.go(4 hunks)internal/exec/vendor_model.go(3 hunks)internal/exec/vendor_utils.go(6 hunks)internal/exec/workflow_utils.go(4 hunks)internal/exec/yaml_func_env.go(3 hunks)internal/exec/yaml_func_exec.go(1 hunks)internal/exec/yaml_func_include.go(3 hunks)internal/exec/yaml_func_store.go(1 hunks)internal/exec/yaml_func_template.go(1 hunks)internal/exec/yaml_func_terraform_output.go(2 hunks)main.go(1 hunks)pkg/aws/aws_eks_update_kubeconfig.go(1 hunks)pkg/component/component_processor.go(3 hunks)pkg/config/cache.go(1 hunks)pkg/config/config.go(3 hunks)pkg/config/utils.go(8 hunks)pkg/hooks/cmd.go(3 hunks)pkg/spacelift/spacelift_stack_processor.go(9 hunks)pkg/utils/hcl_utils.go(2 hunks)pkg/utils/json_utils.go(1 hunks)pkg/utils/log_utils.go(2 hunks)pkg/utils/yaml_utils.go(1 hunks)pkg/validate/validate_component_test.go(4 hunks)pkg/validate/validate_stacks_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (71)
- internal/exec/yaml_func_include.go
- internal/exec/yaml_func_store.go
- pkg/aws/aws_eks_update_kubeconfig.go
- internal/exec/yaml_func_exec.go
- cmd/pro_lock.go
- pkg/utils/json_utils.go
- internal/exec/yaml_func_env.go
- internal/exec/yaml_func_template.go
- pkg/config/cache.go
- cmd/aws_eks_update_kubeconfig.go
- pkg/utils/yaml_utils.go
- pkg/validate/validate_component_test.go
- pkg/component/component_processor.go
- internal/exec/terraform_generate_backend.go
- cmd/terraform_generate_varfiles.go
- internal/exec/file_utils.go
- internal/exec/template_utils.go
- cmd/workflow.go
- cmd/vendor_diff.go
- internal/exec/terraform_outputs.go
- cmd/helmfile.go
- cmd/pro_unlock.go
- internal/exec/terraform_generate_backends.go
- internal/exec/terraform_utils.go
- internal/exec/yaml_func_terraform_output.go
- cmd/describe_workflows.go
- cmd/validate_component.go
- cmd/terraform_generate_varfile.go
- cmd/validate_stacks.go
- cmd/describe_affected.go
- internal/exec/describe_stacks.go
- cmd/terraform_generate_backend.go
- cmd/describe_component.go
- cmd/terraform.go
- internal/exec/oci_utils.go
- cmd/vendor_pull.go
- cmd/helmfile_generate_varfile.go
- cmd/completion.go
- cmd/atlantis_generate_repo_config.go
- internal/exec/vendor_model.go
- internal/exec/atlantis_generate_repo_config.go
- internal/exec/terraform_generate_varfiles.go
- internal/exec/go_getter_utils.go
- internal/exec/aws_eks_update_kubeconfig.go
- pkg/spacelift/spacelift_stack_processor.go
- internal/exec/helmfile_generate_varfile.go
- cmd/docs.go
- internal/exec/tar_utils.go
- internal/exec/utils.go
- internal/exec/terraform_generate_varfile.go
- internal/exec/vendor_component_utils.go
- internal/exec/validate_component.go
- cmd/describe_dependents.go
- internal/exec/helmfile.go
- internal/exec/validate_stacks.go
- pkg/config/utils.go
- internal/exec/shell_utils.go
- pkg/hooks/cmd.go
- cmd/version.go
- internal/exec/terraform_clean.go
- internal/exec/template_funcs_gomplate_datasource.go
- pkg/utils/hcl_utils.go
- main.go
- pkg/validate/validate_stacks_test.go
- cmd/terraform_generate_backends.go
- cmd/describe_stacks.go
- cmd/describe_config.go
- internal/exec/terraform.go
- internal/exec/vendor_utils.go
- internal/exec/template_funcs_component.go
- internal/exec/workflow_utils.go
🧰 Additional context used
📓 Learnings (1)
cmd/cmd_utils.go (1)
Learnt from: osterman
PR: cloudposse/atmos#740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-11-12T05:52:05.088Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.
🪛 golangci-lint (1.62.2)
internal/exec/describe_affected_utils.go
451-451: S1039: unnecessary use of fmt.Sprintf
(gosimple)
458-458: S1039: unnecessary use of fmt.Sprintf
(gosimple)
pkg/config/config.go
394-394: printf: non-constant format string in call to fmt.Sprintf
(govet)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (3)
cmd/cmd_utils.go (2)
143-143: LGTM! Error handling looks good.The error handling in the command alias processing is straightforward and follows the pattern of immediately exiting on errors.
Also applies to: 149-149
324-344: LGTM! Robust component config validation.The validation chain for component configuration is thorough and handles all edge cases appropriately.
internal/exec/describe_affected_utils.go (1)
407-408: LGTM! Clear and informative trace logging.The trace logging for HEAD and BASE references is clear and helpful for debugging.
mcalhoun
force-pushed
the
dev-2668-migrate-to-charmbracelet-logger
branch
from
6135ede to
1b67e30
Compare
mcalhoun
added
enhancement
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
pkg/config/config.go (1)
394-394: Use fmt.Sprintf properly to improve error message formatting.Instead of string concatenation, use fmt.Sprintf's formatting capabilities for better maintainability and performance.
- u.LogWarning(fmt.Sprintf("error closing file '" + configPath + "'. " + err.Error())) + u.LogWarning(fmt.Sprintf("error closing file '%s'. %s", configPath, err.Error()))🧰 Tools
🪛 golangci-lint (1.62.2)
394-394: printf: non-constant format string in call to fmt.Sprintf
(govet)
internal/exec/describe_affected_utils.go (1)
451-451: Simplify string loggingLet's optimize these lines by removing unnecessary
fmt.Sprintfcalls:-u.LogTrace(fmt.Sprintf("Getting current working repo commit object...")) +u.LogTrace("Getting current working repo commit object...") -u.LogTrace(fmt.Sprintf("Got current working repo commit object")) +u.LogTrace("Got current working repo commit object")Also applies to: 458-458
🧰 Tools
🪛 golangci-lint (1.62.2)
451-451: S1039: unnecessary use of fmt.Sprintf
(gosimple)
pkg/utils/log_utils.go (2)
51-58: Consider enhancing error logging with structured fields.The error logging could be more informative by utilizing charmbracelet's structured logging capabilities.
func LogError(err error) { - l.Error(err) + l.Error("error occurred", "error", err) // Print stack trace if l.GetLevel() == l.DebugLevel { debug.PrintStack() } }
62-78: Standardize logging with structured fields across all levels.Consider adopting a consistent structured logging approach across all logging functions for better observability.
func LogTrace(message string) { LogDebug(message) } func LogDebug(message string) { - l.Debug(message) + l.Debug("debug message", "message", message) } func LogInfo(message string) { - l.Info(message) + l.Info("info message", "message", message) } func LogWarning(message string) { - l.Warn(message) + l.Warn("warning message", "message", message) }cmd/root.go (1)
88-112: Consider using charmbracelet's built-in file writer.The current implementation could be improved by using charmbracelet's built-in file writer capabilities.
func setupLogger(atmosConfig *schema.AtmosConfiguration) { switch atmosConfig.Logs.Level { case "Trace": log.SetLevel(log.DebugLevel) case "Debug": log.SetLevel(log.DebugLevel) case "Info": log.SetLevel(log.InfoLevel) case "Warning": log.SetLevel(log.WarnLevel) case "Off": log.SetOutput(io.Discard) default: log.SetLevel(log.InfoLevel) } if atmosConfig.Logs.File != "/dev/stderr" { - logFile, err := os.OpenFile(atmosConfig.Logs.File, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0666) - if err != nil { - log.Fatal("Failed to open log file:", err) - } - defer logFile.Close() - log.SetOutput(logFile) + fileWriter := log.NewFileWriter(atmosConfig.Logs.File, &log.FileWriterOptions{ + Flag: os.O_CREATE | os.O_WRONLY | os.O_APPEND, + Permission: 0666, + }) + log.SetOutput(fileWriter) } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (77)
cmd/atlantis_generate_repo_config.go(1 hunks)cmd/aws_eks_update_kubeconfig.go(1 hunks)cmd/cmd_utils.go(13 hunks)cmd/completion.go(1 hunks)cmd/describe_affected.go(1 hunks)cmd/describe_component.go(2 hunks)cmd/describe_config.go(1 hunks)cmd/describe_dependents.go(2 hunks)cmd/describe_stacks.go(1 hunks)cmd/describe_workflows.go(1 hunks)cmd/docs.go(5 hunks)cmd/helmfile.go(1 hunks)cmd/helmfile_generate_varfile.go(2 hunks)cmd/pro_lock.go(1 hunks)cmd/pro_unlock.go(1 hunks)cmd/root.go(6 hunks)cmd/terraform.go(1 hunks)cmd/terraform_generate_backend.go(2 hunks)cmd/terraform_generate_backends.go(1 hunks)cmd/terraform_generate_varfile.go(2 hunks)cmd/terraform_generate_varfiles.go(2 hunks)cmd/validate_component.go(2 hunks)cmd/validate_editorconfig.go(3 hunks)cmd/validate_stacks.go(1 hunks)cmd/vendor_diff.go(1 hunks)cmd/vendor_pull.go(1 hunks)cmd/version.go(2 hunks)cmd/workflow.go(5 hunks)internal/exec/atlantis_generate_repo_config.go(1 hunks)internal/exec/aws_eks_update_kubeconfig.go(1 hunks)internal/exec/describe_affected_utils.go(13 hunks)internal/exec/describe_stacks.go(2 hunks)internal/exec/file_utils.go(1 hunks)internal/exec/go_getter_utils.go(2 hunks)internal/exec/helmfile.go(7 hunks)internal/exec/helmfile_generate_varfile.go(2 hunks)internal/exec/oci_utils.go(1 hunks)internal/exec/shell_utils.go(10 hunks)internal/exec/tar_utils.go(2 hunks)internal/exec/template_funcs_component.go(2 hunks)internal/exec/template_funcs_gomplate_datasource.go(3 hunks)internal/exec/template_utils.go(3 hunks)internal/exec/terraform.go(11 hunks)internal/exec/terraform_clean.go(6 hunks)internal/exec/terraform_generate_backend.go(2 hunks)internal/exec/terraform_generate_backends.go(2 hunks)internal/exec/terraform_generate_varfile.go(2 hunks)internal/exec/terraform_generate_varfiles.go(2 hunks)internal/exec/terraform_outputs.go(14 hunks)internal/exec/terraform_utils.go(1 hunks)internal/exec/utils.go(5 hunks)internal/exec/validate_component.go(2 hunks)internal/exec/validate_stacks.go(2 hunks)internal/exec/vendor_component_utils.go(4 hunks)internal/exec/vendor_model.go(3 hunks)internal/exec/vendor_utils.go(6 hunks)internal/exec/workflow_utils.go(4 hunks)internal/exec/yaml_func_env.go(3 hunks)internal/exec/yaml_func_exec.go(1 hunks)internal/exec/yaml_func_include.go(3 hunks)internal/exec/yaml_func_store.go(1 hunks)internal/exec/yaml_func_template.go(1 hunks)internal/exec/yaml_func_terraform_output.go(2 hunks)main.go(1 hunks)pkg/aws/aws_eks_update_kubeconfig.go(1 hunks)pkg/component/component_processor.go(3 hunks)pkg/config/cache.go(1 hunks)pkg/config/config.go(3 hunks)pkg/config/utils.go(8 hunks)pkg/hooks/cmd.go(3 hunks)pkg/spacelift/spacelift_stack_processor.go(9 hunks)pkg/utils/hcl_utils.go(2 hunks)pkg/utils/json_utils.go(1 hunks)pkg/utils/log_utils.go(2 hunks)pkg/utils/yaml_utils.go(1 hunks)pkg/validate/validate_component_test.go(4 hunks)pkg/validate/validate_stacks_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (73)
- internal/exec/template_utils.go
- cmd/pro_unlock.go
- internal/exec/yaml_func_template.go
- internal/exec/file_utils.go
- pkg/aws/aws_eks_update_kubeconfig.go
- cmd/pro_lock.go
- internal/exec/terraform_utils.go
- pkg/config/cache.go
- pkg/utils/hcl_utils.go
- cmd/describe_workflows.go
- internal/exec/template_funcs_gomplate_datasource.go
- internal/exec/yaml_func_env.go
- cmd/describe_stacks.go
- cmd/validate_stacks.go
- internal/exec/terraform_generate_backend.go
- cmd/atlantis_generate_repo_config.go
- pkg/utils/json_utils.go
- internal/exec/yaml_func_include.go
- cmd/helmfile.go
- pkg/validate/validate_component_test.go
- cmd/vendor_diff.go
- pkg/validate/validate_stacks_test.go
- internal/exec/atlantis_generate_repo_config.go
- cmd/describe_config.go
- internal/exec/terraform_generate_backends.go
- cmd/completion.go
- internal/exec/template_funcs_component.go
- internal/exec/aws_eks_update_kubeconfig.go
- cmd/validate_component.go
- cmd/terraform_generate_varfile.go
- cmd/terraform_generate_varfiles.go
- internal/exec/terraform_generate_varfile.go
- internal/exec/utils.go
- internal/exec/vendor_model.go
- cmd/terraform_generate_backends.go
- internal/exec/yaml_func_exec.go
- cmd/aws_eks_update_kubeconfig.go
- pkg/component/component_processor.go
- internal/exec/helmfile_generate_varfile.go
- internal/exec/vendor_utils.go
- main.go
- cmd/workflow.go
- internal/exec/helmfile.go
- cmd/cmd_utils.go
- internal/exec/validate_stacks.go
- cmd/terraform_generate_backend.go
- cmd/version.go
- cmd/helmfile_generate_varfile.go
- cmd/docs.go
- internal/exec/yaml_func_store.go
- cmd/vendor_pull.go
- cmd/describe_component.go
- cmd/describe_affected.go
- pkg/config/utils.go
- cmd/validate_editorconfig.go
- internal/exec/terraform_outputs.go
- internal/exec/go_getter_utils.go
- internal/exec/terraform.go
- internal/exec/validate_component.go
- internal/exec/vendor_component_utils.go
- internal/exec/shell_utils.go
- internal/exec/tar_utils.go
- internal/exec/oci_utils.go
- pkg/hooks/cmd.go
- internal/exec/describe_stacks.go
- cmd/terraform.go
- internal/exec/yaml_func_terraform_output.go
- pkg/utils/yaml_utils.go
- cmd/describe_dependents.go
- internal/exec/workflow_utils.go
- pkg/spacelift/spacelift_stack_processor.go
- internal/exec/terraform_clean.go
- internal/exec/terraform_generate_varfiles.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
internal/exec/describe_affected_utils.go
451-451: S1039: unnecessary use of fmt.Sprintf
(gosimple)
458-458: S1039: unnecessary use of fmt.Sprintf
(gosimple)
pkg/config/config.go
394-394: printf: non-constant format string in call to fmt.Sprintf
(govet)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (11)
pkg/config/config.go (3)
195-195: LGTM! Clean logging migration.The removal of
atmosConfigparameter fromLogTracealigns with the migration to charmbracelet logger.
Line range hint
358-363: LGTM! Consistent logging update.The logging change maintains consistency with the new logging pattern while preserving the meaningful debug information.
Line range hint
195-394: Overall logging migration looks solid!The changes consistently remove the
atmosConfigparameter from logging calls while maintaining the same level of logging information. This aligns well with the migration to charmbracelet logger.internal/exec/describe_affected_utils.go (4)
Line range hint
76-89: Clean logging changes!The logging modifications in this section maintain the same informative messages while adapting to the new logging pattern.
Line range hint
129-155: Well-structured logging for Git operations!The logging changes maintain clear visibility of Git checkout operations while adapting to the new logging pattern.
Line range hint
211-307: Clear operational logging maintained!The logging changes effectively track repository operations while adapting to the new logging pattern.
Line range hint
407-503: Effective diff operation logging!The logging changes maintain clear visibility of diff operations while adapting to the new logging pattern.
🧰 Tools
🪛 golangci-lint (1.62.2)
451-451: S1039: unnecessary use of fmt.Sprintf
(gosimple)
458-458: S1039: unnecessary use of fmt.Sprintf
(gosimple)
pkg/utils/log_utils.go (2)
10-10: Initialize charmbracelet logger with default settings.The logger is imported but not initialized with default settings. Consider adding initialization code to set up the logger with appropriate defaults.
38-47: Well-structured error handling implementation!The function effectively handles both generic errors and command-specific exit codes. The use of
errors.Asfor type assertion is a robust approach.cmd/root.go (2)
134-135: Well-placed logger initialization!The logger setup is correctly positioned after the atmosConfig is loaded, ensuring proper configuration of the logging system.
63-63: Consistent error handling throughout the file!The error handling is uniform and follows best practices with appropriate use of LogErrorAndExit for fatal errors.
Also applies to: 66-66, 78-78, 83-83, 130-130, 142-142, 147-147, 191-191, 223-223, 226-226, 231-231, 235-235
osterman
changed the title
mcalhoun
force-pushed
the
dev-2668-migrate-to-charmbracelet-logger
branch
from
1b67e30 to
cf81753
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
internal/exec/describe_affected_utils.go (1)
451-451: Optimize string handling.Remove unnecessary
fmt.Sprintfcalls when no string formatting is needed.Apply this diff to optimize the code:
- u.LogTrace(fmt.Sprintf("\nGetting current working repo commit object...")) + u.LogTrace("\nGetting current working repo commit object...") - u.LogTrace(fmt.Sprintf("Got current working repo commit object")) + u.LogTrace("Got current working repo commit object")Also applies to: 458-458
🧰 Tools
🪛 golangci-lint (1.62.2)
451-451: S1039: unnecessary use of fmt.Sprintf
(gosimple)
pkg/utils/log_utils.go (2)
51-57: Simplify debug level stack trace handling.Consider using Charmbracelet logger's built-in caller reporting instead of manual stack trace printing.
func LogError(err error) { - l.Error(err) - - // Print stack trace - if l.GetLevel() == l.DebugLevel { - debug.PrintStack() - } + l.Error(err, "caller", l.Caller(2)) }
62-78: Consider utilizing structured logging capabilities.Charmbracelet logger supports structured logging which can enhance log parsing and filtering.
Example enhancement:
-func LogInfo(message string) { +func LogInfo(message string, fields ...interface{}) { - l.Info(message) + l.Info(message, fields...) }This allows for richer context:
LogInfo("Processing complete", "duration", elapsed, "items", count)pkg/config/config.go (1)
Line range hint
358-363: Consider improving string formatting readability.While the logging change is good, we can make the multi-line string formatting more readable.
- u.LogTrace(fmt.Sprintf("\nThe stack '%s' matches the stack manifest %s\n", - configAndStacksInfo.Stack, - stackConfigFilesRelativePaths[0]), - ) + u.LogTrace(fmt.Sprintf( + "\nThe stack '%s' matches the stack manifest %s\n", + configAndStacksInfo.Stack, + stackConfigFilesRelativePaths[0], + ))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (82)
cmd/atlantis_generate_repo_config.go(1 hunks)cmd/aws_eks_update_kubeconfig.go(1 hunks)cmd/cmd_utils.go(13 hunks)cmd/completion.go(1 hunks)cmd/describe_affected.go(1 hunks)cmd/describe_component.go(2 hunks)cmd/describe_config.go(1 hunks)cmd/describe_dependents.go(2 hunks)cmd/describe_stacks.go(1 hunks)cmd/describe_workflows.go(1 hunks)cmd/docs.go(5 hunks)cmd/helmfile.go(1 hunks)cmd/helmfile_generate_varfile.go(2 hunks)cmd/pro_lock.go(1 hunks)cmd/pro_unlock.go(1 hunks)cmd/root.go(6 hunks)cmd/terraform.go(1 hunks)cmd/terraform_generate_backend.go(2 hunks)cmd/terraform_generate_backends.go(1 hunks)cmd/terraform_generate_varfile.go(2 hunks)cmd/terraform_generate_varfiles.go(2 hunks)cmd/validate_component.go(2 hunks)cmd/validate_editorconfig.go(3 hunks)cmd/validate_stacks.go(1 hunks)cmd/vendor_diff.go(1 hunks)cmd/vendor_pull.go(1 hunks)cmd/version.go(2 hunks)cmd/workflow.go(5 hunks)internal/exec/atlantis_generate_repo_config.go(1 hunks)internal/exec/aws_eks_update_kubeconfig.go(1 hunks)internal/exec/describe_affected_utils.go(13 hunks)internal/exec/describe_stacks.go(2 hunks)internal/exec/file_utils.go(1 hunks)internal/exec/go_getter_utils.go(2 hunks)internal/exec/helmfile.go(7 hunks)internal/exec/helmfile_generate_varfile.go(2 hunks)internal/exec/oci_utils.go(1 hunks)internal/exec/shell_utils.go(10 hunks)internal/exec/tar_utils.go(2 hunks)internal/exec/template_funcs_component.go(2 hunks)internal/exec/template_funcs_gomplate_datasource.go(3 hunks)internal/exec/template_utils.go(3 hunks)internal/exec/terraform.go(11 hunks)internal/exec/terraform_clean.go(6 hunks)internal/exec/terraform_generate_backend.go(2 hunks)internal/exec/terraform_generate_backends.go(2 hunks)internal/exec/terraform_generate_varfile.go(2 hunks)internal/exec/terraform_generate_varfiles.go(2 hunks)internal/exec/terraform_outputs.go(14 hunks)internal/exec/terraform_utils.go(1 hunks)internal/exec/utils.go(5 hunks)internal/exec/validate_component.go(2 hunks)internal/exec/validate_stacks.go(2 hunks)internal/exec/vendor_component_utils.go(4 hunks)internal/exec/vendor_model.go(3 hunks)internal/exec/vendor_utils.go(6 hunks)internal/exec/workflow_utils.go(4 hunks)internal/exec/yaml_func_env.go(3 hunks)internal/exec/yaml_func_exec.go(1 hunks)internal/exec/yaml_func_include.go(3 hunks)internal/exec/yaml_func_store.go(1 hunks)internal/exec/yaml_func_template.go(1 hunks)internal/exec/yaml_func_terraform_output.go(2 hunks)main.go(1 hunks)pkg/aws/aws_eks_update_kubeconfig.go(1 hunks)pkg/component/component_processor.go(3 hunks)pkg/config/cache.go(1 hunks)pkg/config/config.go(3 hunks)pkg/config/utils.go(8 hunks)pkg/hooks/cmd.go(3 hunks)pkg/spacelift/spacelift_stack_processor.go(9 hunks)pkg/utils/hcl_utils.go(2 hunks)pkg/utils/json_utils.go(1 hunks)pkg/utils/log_utils.go(2 hunks)pkg/utils/yaml_utils.go(1 hunks)pkg/validate/validate_component_test.go(4 hunks)pkg/validate/validate_stacks_test.go(1 hunks)tests/README.md(1 hunks)tests/snapshots/TestCLICommands_Invalid_Log_Level_in_Config_File.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_Invalid_Log_Level_in_Environment_Variable.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_Valid_Log_Level_in_Environment_Variable.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_--help.stdout.golden(1 hunks)
⛔ Files not processed due to max files limit (6)
- tests/snapshots/TestCLICommands_atmos_circuit-breaker.stderr.golden
- tests/snapshots/TestCLICommands_atmos_describe_config.stdout.golden
- tests/snapshots/TestCLICommands_atmos_describe_config_-f_yaml.stdout.golden
- tests/snapshots/TestCLICommands_atmos_terraform_deploy_locked_component.stderr.golden
- tests/snapshots/TestCLICommands_atmos_vendor_pull_(no_tty).stderr.golden
- tests/snapshots/TestCLICommands_check_atmos_--help_in_empty-dir.stdout.golden
✅ Files skipped from review due to trivial changes (3)
- tests/README.md
- tests/snapshots/TestCLICommands_Valid_Log_Level_in_Environment_Variable.stderr.golden
- tests/snapshots/TestCLICommands_atmos_--help.stdout.golden
🚧 Files skipped from review as they are similar to previous changes (73)
- internal/exec/yaml_func_template.go
- cmd/pro_lock.go
- pkg/utils/yaml_utils.go
- main.go
- pkg/validate/validate_component_test.go
- cmd/describe_workflows.go
- internal/exec/go_getter_utils.go
- cmd/aws_eks_update_kubeconfig.go
- cmd/completion.go
- cmd/describe_stacks.go
- pkg/config/cache.go
- cmd/describe_affected.go
- pkg/aws/aws_eks_update_kubeconfig.go
- pkg/component/component_processor.go
- cmd/terraform_generate_varfile.go
- internal/exec/helmfile_generate_varfile.go
- cmd/vendor_diff.go
- cmd/validate_component.go
- internal/exec/describe_stacks.go
- cmd/workflow.go
- cmd/terraform_generate_backends.go
- pkg/utils/hcl_utils.go
- internal/exec/template_utils.go
- internal/exec/file_utils.go
- internal/exec/yaml_func_include.go
- cmd/pro_unlock.go
- internal/exec/terraform_generate_backends.go
- cmd/helmfile_generate_varfile.go
- internal/exec/yaml_func_terraform_output.go
- internal/exec/terraform_utils.go
- cmd/validate_stacks.go
- cmd/describe_component.go
- internal/exec/terraform_generate_varfiles.go
- internal/exec/yaml_func_store.go
- internal/exec/vendor_model.go
- internal/exec/tar_utils.go
- internal/exec/validate_stacks.go
- cmd/cmd_utils.go
- cmd/docs.go
- internal/exec/oci_utils.go
- cmd/helmfile.go
- cmd/terraform.go
- internal/exec/helmfile.go
- cmd/terraform_generate_backend.go
- cmd/describe_dependents.go
- internal/exec/aws_eks_update_kubeconfig.go
- internal/exec/atlantis_generate_repo_config.go
- pkg/validate/validate_stacks_test.go
- internal/exec/utils.go
- internal/exec/shell_utils.go
- cmd/version.go
- pkg/hooks/cmd.go
- internal/exec/terraform_outputs.go
- cmd/vendor_pull.go
- internal/exec/terraform_generate_backend.go
- internal/exec/workflow_utils.go
- cmd/terraform_generate_varfiles.go
- internal/exec/validate_component.go
- internal/exec/yaml_func_env.go
- internal/exec/yaml_func_exec.go
- internal/exec/terraform_clean.go
- cmd/atlantis_generate_repo_config.go
- internal/exec/terraform_generate_varfile.go
- cmd/describe_config.go
- pkg/utils/json_utils.go
- internal/exec/vendor_utils.go
- internal/exec/vendor_component_utils.go
- cmd/validate_editorconfig.go
- pkg/spacelift/spacelift_stack_processor.go
- internal/exec/template_funcs_gomplate_datasource.go
- internal/exec/template_funcs_component.go
- pkg/config/utils.go
- internal/exec/terraform.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
internal/exec/describe_affected_utils.go
451-451: S1039: unnecessary use of fmt.Sprintf
(gosimple)
458-458: S1039: unnecessary use of fmt.Sprintf
(gosimple)
pkg/config/config.go
394-394: printf: non-constant format string in call to fmt.Sprintf
(govet)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (14)
internal/exec/describe_affected_utils.go (4)
Line range hint
76-89: Clean logging refactor!The removal of
atmosConfigparameter from logging calls aligns well with the migration to charmbracelet logger while maintaining the same logging information.
Line range hint
129-155: Solid logging simplification!The logging changes maintain functionality while streamlining the API by removing the unnecessary
atmosConfigparameter.
Line range hint
211-307: Excellent logging refactor!The logging changes in this section are consistent with the overall migration strategy and maintain clear, informative messages.
Line range hint
407-503: Well-executed logging changes!The logging modifications in this section maintain clarity while aligning with the new logging architecture.
🧰 Tools
🪛 golangci-lint (1.62.2)
451-451: S1039: unnecessary use of fmt.Sprintf
(gosimple)
458-458: S1039: unnecessary use of fmt.Sprintf
(gosimple)
tests/snapshots/TestCLICommands_Invalid_Log_Level_in_Environment_Variable.stderr.golden (1)
1-1: Well structured error output, warrior! 🛡️The error message format aligns perfectly with charmbracelet logger's standard output format, including:
- Timestamp for temporal context
- Color-coded error level for quick visual identification
- Clear error description with valid options
Let's verify the logger configuration to ensure consistency across the codebase.
✅ Verification successful
Logger configuration verified and battle-ready! 🛡️
The error message in the snapshot test perfectly aligns with the charmbracelet logger implementation. The supported log levels [Trace Debug Info Warning Off] and error formatting are consistently configured across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify logger configuration and usage across the codebase # Check for consistent logger initialization echo "Checking logger initialization patterns..." rg -A 3 "charmbracelet/log" # Verify log level configuration echo "Checking log level configurations..." rg -A 2 "SetLevel|SetReportCaller|SetFormatter" # Look for any remaining old logging patterns echo "Checking for old logging patterns..." rg "log\.(Print|Fatal|Error|Info|Debug|Warn)[f|ln]?\("Length of output: 3615
tests/snapshots/TestCLICommands_Invalid_Log_Level_in_Config_File.stderr.golden (1)
1-1: Well structured error output format!The new format provides excellent clarity with:
- Timestamp for context
- Color-coded error level
- Clear error message with valid options
pkg/utils/log_utils.go (2)
10-10: Initialize charmbracelet logger.The logger needs initialization with appropriate defaults.
38-78: Strong foundation for the logger migration!The removal of
atmosConfigand migration to Charmbracelet logger has significantly simplified the codebase. The implementation is clean and maintainable.pkg/config/config.go (1)
195-195: Clean logging refactor!The simplified logging call aligns well with the migration to charmbracelet logger.
cmd/root.go (5)
11-11: Clean migration to charmbracelet logger! 🛡️The simplified logging calls and clean import of charmbracelet/log align perfectly with the standardization goals.
Also applies to: 63-66
88-112: Solid logger setup implementation! 🎯The implementation provides comprehensive log level mapping and file output configuration. The file handle management approach (keeping it open for the program's lifecycle) aligns with the previously confirmed design intention.
128-131: Smart error handling for version command! 💪Good choice using debug level for configuration errors during version command execution. This prevents unnecessary error messages while maintaining debug information.
134-136: Perfect placement of logger setup! 🎯Strategic positioning of setupLogger ensures proper logging configuration before any command processing begins.
88-112: Verify logger configuration meets styling requirementsThe implementation successfully migrates to charmbracelet logger. Let's verify the logger configuration supports future lipgloss styling capabilities.
Also applies to: 134-136
✅ Verification successful
Logger implementation is battle-ready! 💪
The logger setup is properly configured and ready for styling capabilities. The implementation:
- Correctly integrates with the project's theming system
- Supports all required log levels
- Maintains compatibility with charmbracelet's styling features
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the charmbracelet/log version supports styling features go list -m -versions github.com/charmbracelet/log | grep -q "v" && echo "Logger version supports styling" # Verify lipgloss compatibility in imports rg -l 'github.com/charmbracelet/(log|lipgloss)' --type goLength of output: 801
Script:
#!/bin/bash # Check log utils implementation for styling setup cat pkg/utils/log_utils.go # Verify if there are any style configurations in the logger setup rg -A 5 "SetStyle|NewStyle|Style" pkg/utils/log_utils.go cmd/root.goLength of output: 2670
|
💥 This pull request now has conflicts. Could you fix it @mcalhoun? 🙏 |
mcalhoun
force-pushed
the
dev-2668-migrate-to-charmbracelet-logger
branch
from
cf81753 to
b5e8a44
Compare
mcalhoun
force-pushed
the
dev-2668-migrate-to-charmbracelet-logger
branch
from
aa9809b to
283cb4b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
pkg/utils/log_utils.go (1)
Line range hint
15-19: Align log level constants with Charmbracelet logger.The current constants should be updated to match Charmbracelet's logging levels for consistency.
const ( - LogLevelTrace = "Trace" - LogLevelDebug = "Debug" - LogLevelInfo = "Info" - LogLevelWarning = "Warning" + LogLevelTrace = l.TraceLevel + LogLevelDebug = l.DebugLevel + LogLevelInfo = l.InfoLevel + LogLevelWarning = l.WarnLevel )
♻️ Duplicate comments (1)
pkg/utils/log_utils.go (1)
10-10:⚠️ Potential issueInitialize the Charmbracelet logger with appropriate defaults.
The logger is imported but not initialized. This could lead to unexpected logging behavior.
Add initialization code at the package level:
func init() { l.SetLevel(l.InfoLevel) l.SetReportCaller(true) l.SetFormatter(l.TextFormatter) }
🧹 Nitpick comments (8)
cmd/root.go (2)
88-102: Consider adding error handling for log level setting.While the log level mapping is correct, consider handling potential errors from
SetLevelcalls to ensure robust logging initialization.func setupLogger(atmosConfig *schema.AtmosConfiguration) { + var err error switch atmosConfig.Logs.Level { case "Trace": - log.SetLevel(log.DebugLevel) + err = log.SetLevel(log.DebugLevel) case "Debug": - log.SetLevel(log.DebugLevel) + err = log.SetLevel(log.DebugLevel) case "Info": - log.SetLevel(log.InfoLevel) + err = log.SetLevel(log.InfoLevel) case "Warning": - log.SetLevel(log.WarnLevel) + err = log.SetLevel(log.WarnLevel) case "Off": log.SetOutput(io.Discard) default: - log.SetLevel(log.InfoLevel) + err = log.SetLevel(log.InfoLevel) + } + if err != nil { + log.Fatal("Failed to set log level:", err) }
104-105: Good temporary solution for snapshot compatibility.The timestamp disable is well-documented as a temporary measure. Consider tracking this in an issue for future styling updates.
Would you like me to create an issue to track the future styling updates for timestamps?
internal/exec/describe_affected_utils.go (4)
76-76: Simplify string formatting in log messages.The logging changes look good, but some messages use unnecessary
fmt.Sprintffor simple strings. Let's optimize these calls.Apply this diff to simplify the string formatting:
-u.LogTrace(fmt.Sprintf("\nCloning repo '%s' into the temp dir '%s'", localRepoInfo.RepoUrl, tempDir)) +u.LogTrace(fmt.Sprintf("\nCloning repo %q into the temp dir %q", localRepoInfo.RepoUrl, tempDir)) -u.LogTrace(fmt.Sprintf("\nCloning Git ref '%s' ...\n", ref)) +u.LogTrace(fmt.Sprintf("\nCloning Git ref %q...\n", ref)) -u.LogTrace(fmt.Sprintf("\nCloned Git ref '%s'\n", ref)) +u.LogTrace(fmt.Sprintf("\nCloned Git ref %q\n", ref)) -u.LogTrace(fmt.Sprintf("\nCloned Git ref '%s'\n", remoteRepoHead.Name())) +u.LogTrace(fmt.Sprintf("\nCloned Git ref %q\n", remoteRepoHead.Name())) -u.LogTrace(fmt.Sprintf("\nChecking out commit SHA '%s' ...\n", sha)) +u.LogTrace(fmt.Sprintf("\nChecking out commit SHA %q...\n", sha)) -u.LogTrace(fmt.Sprintf("\nChecked out commit SHA '%s'\n", sha)) +u.LogTrace(fmt.Sprintf("\nChecked out commit SHA %q\n", sha))Also applies to: 87-87, 89-89, 129-129, 131-131, 136-136, 155-155
211-211: Optimize string formatting in checkout log messages.The logging changes are good, but let's optimize the string formatting for better readability and performance.
Apply this diff to improve the string formatting:
-u.LogTrace(fmt.Sprintf("\nCopying the local repo into the temp directory '%s' ...", tempDir)) +u.LogTrace(fmt.Sprintf("\nCopying the local repo into the temp directory %q...", tempDir)) -u.LogTrace(fmt.Sprintf("Copied the local repo into the temp directory '%s'\n", tempDir)) +u.LogTrace(fmt.Sprintf("Copied the local repo into the temp directory %q\n", tempDir)) -u.LogTrace(fmt.Sprintf("\nChecking out commit SHA '%s' ...\n", sha)) +u.LogTrace(fmt.Sprintf("\nChecking out commit SHA %q...\n", sha)) -u.LogTrace(fmt.Sprintf("Checked out commit SHA '%s'\n", sha)) +u.LogTrace(fmt.Sprintf("Checked out commit SHA %q\n", sha)) -u.LogTrace(fmt.Sprintf("\nChecking out Git ref '%s' ...", ref)) +u.LogTrace(fmt.Sprintf("\nChecking out Git ref %q...", ref)) -u.LogTrace(fmt.Sprintf("Checked out Git ref '%s'\n", ref)) +u.LogTrace(fmt.Sprintf("Checked out Git ref %q\n", ref))Also applies to: 239-239, 256-256, 275-275, 282-282, 307-307
407-408: Simplify Git operation log messages.The logging changes provide good tracing of Git operations. However, some messages can be simplified based on static analysis hints.
Apply this diff to optimize the logging:
-u.LogTrace(fmt.Sprintf("Current HEAD: %s", localRepoHead)) +u.LogTrace("Current HEAD: " + localRepoHead.String()) -u.LogTrace(fmt.Sprintf("BASE: %s", remoteRepoHead)) +u.LogTrace("BASE: " + remoteRepoHead.String()) -u.LogTrace(fmt.Sprintf("\nGetting current working repo commit object...")) +u.LogTrace("\nGetting current working repo commit object...") -u.LogTrace(fmt.Sprintf("Got current working repo commit object")) +u.LogTrace("Got current working repo commit object") -u.LogTrace(fmt.Sprintf("Getting current working repo commit tree...")) +u.LogTrace("Getting current working repo commit tree...") -u.LogTrace(fmt.Sprintf("Got current working repo commit tree")) +u.LogTrace("Got current working repo commit tree") -u.LogTrace(fmt.Sprintf("Getting remote repo commit object...")) +u.LogTrace("Getting remote repo commit object...") -u.LogTrace(fmt.Sprintf("Got remote repo commit object")) +u.LogTrace("Got remote repo commit object") -u.LogTrace(fmt.Sprintf("Getting remote repo commit tree...")) +u.LogTrace("Getting remote repo commit tree...") -u.LogTrace(fmt.Sprintf("Got remote repo commit tree")) +u.LogTrace("Got remote repo commit tree") -u.LogTrace(fmt.Sprintf("Finding difference between the current working branch and remote target branch ...")) +u.LogTrace("Finding difference between the current working branch and remote target branch...") -u.LogTrace(fmt.Sprintf("Found difference between the current working branch and remote target branch")) +u.LogTrace("Found difference between the current working branch and remote target branch") -u.LogTrace(fmt.Sprintf("The current working branch and remote target branch are the same")) +u.LogTrace("The current working branch and remote target branch are the same")Also applies to: 451-451, 458-459, 466-467, 474-475, 482-483, 494-495, 498-498, 501-501, 503-503
1528-1528: Optimize component status log message.The logging change is good, but let's optimize the string formatting.
Apply this diff to simplify the logging:
-u.LogTrace(fmt.Sprintf("Skipping disabled component %s", componentName)) +u.LogTrace("Skipping disabled component " + componentName)pkg/utils/log_utils.go (2)
38-47: Consider adding context to errors before logging.While the error handling is functional, adding context would improve debugging capabilities.
func LogErrorAndExit(err error) { + if err != nil { + err = fmt.Errorf("execution failed: %w", err) + } l.Error(err)
51-58: Standardize debug level checking across error handling.The debug level check is good, but consider extracting it into a helper function for consistency.
+func shouldPrintStack() bool { + return l.GetLevel() == l.DebugLevel +} + func LogError(err error) { l.Error(err) - if l.GetLevel() == l.DebugLevel { + if shouldPrintStack() { debug.PrintStack() } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (82)
cmd/atlantis_generate_repo_config.go(1 hunks)cmd/aws_eks_update_kubeconfig.go(1 hunks)cmd/cmd_utils.go(13 hunks)cmd/completion.go(1 hunks)cmd/describe_affected.go(1 hunks)cmd/describe_component.go(2 hunks)cmd/describe_config.go(1 hunks)cmd/describe_dependents.go(2 hunks)cmd/describe_stacks.go(1 hunks)cmd/describe_workflows.go(1 hunks)cmd/docs.go(5 hunks)cmd/helmfile.go(1 hunks)cmd/helmfile_generate_varfile.go(2 hunks)cmd/pro_lock.go(1 hunks)cmd/pro_unlock.go(1 hunks)cmd/root.go(6 hunks)cmd/terraform.go(1 hunks)cmd/terraform_generate_backend.go(2 hunks)cmd/terraform_generate_backends.go(1 hunks)cmd/terraform_generate_varfile.go(2 hunks)cmd/terraform_generate_varfiles.go(2 hunks)cmd/validate_component.go(2 hunks)cmd/validate_editorconfig.go(3 hunks)cmd/validate_stacks.go(1 hunks)cmd/vendor_diff.go(1 hunks)cmd/vendor_pull.go(1 hunks)cmd/version.go(2 hunks)cmd/workflow.go(5 hunks)internal/exec/atlantis_generate_repo_config.go(1 hunks)internal/exec/aws_eks_update_kubeconfig.go(1 hunks)internal/exec/describe_affected_utils.go(13 hunks)internal/exec/describe_stacks.go(2 hunks)internal/exec/file_utils.go(1 hunks)internal/exec/go_getter_utils.go(2 hunks)internal/exec/helmfile.go(7 hunks)internal/exec/helmfile_generate_varfile.go(2 hunks)internal/exec/oci_utils.go(1 hunks)internal/exec/shell_utils.go(10 hunks)internal/exec/tar_utils.go(2 hunks)internal/exec/template_funcs_component.go(2 hunks)internal/exec/template_funcs_gomplate_datasource.go(3 hunks)internal/exec/template_utils.go(3 hunks)internal/exec/terraform.go(11 hunks)internal/exec/terraform_clean.go(6 hunks)internal/exec/terraform_generate_backend.go(2 hunks)internal/exec/terraform_generate_backends.go(2 hunks)internal/exec/terraform_generate_varfile.go(2 hunks)internal/exec/terraform_generate_varfiles.go(2 hunks)internal/exec/terraform_outputs.go(14 hunks)internal/exec/terraform_utils.go(1 hunks)internal/exec/utils.go(5 hunks)internal/exec/validate_component.go(2 hunks)internal/exec/validate_stacks.go(2 hunks)internal/exec/vendor_component_utils.go(4 hunks)internal/exec/vendor_model.go(3 hunks)internal/exec/vendor_utils.go(6 hunks)internal/exec/workflow_utils.go(4 hunks)internal/exec/yaml_func_env.go(3 hunks)internal/exec/yaml_func_exec.go(1 hunks)internal/exec/yaml_func_include.go(3 hunks)internal/exec/yaml_func_store.go(1 hunks)internal/exec/yaml_func_template.go(1 hunks)internal/exec/yaml_func_terraform_output.go(2 hunks)main.go(1 hunks)pkg/aws/aws_eks_update_kubeconfig.go(1 hunks)pkg/component/component_processor.go(3 hunks)pkg/config/cache.go(1 hunks)pkg/config/config.go(3 hunks)pkg/config/utils.go(8 hunks)pkg/hooks/cmd.go(3 hunks)pkg/spacelift/spacelift_stack_processor.go(9 hunks)pkg/utils/hcl_utils.go(2 hunks)pkg/utils/json_utils.go(1 hunks)pkg/utils/log_utils.go(2 hunks)pkg/utils/yaml_utils.go(1 hunks)pkg/validate/validate_component_test.go(4 hunks)pkg/validate/validate_stacks_test.go(1 hunks)tests/README.md(1 hunks)tests/snapshots/TestCLICommands_Invalid_Log_Level_in_Config_File.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_Invalid_Log_Level_in_Environment_Variable.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_Valid_Log_Level_in_Environment_Variable.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_--help.stdout.golden(1 hunks)
⛔ Files not processed due to max files limit (6)
- tests/snapshots/TestCLICommands_atmos_circuit-breaker.stderr.golden
- tests/snapshots/TestCLICommands_atmos_describe_config.stdout.golden
- tests/snapshots/TestCLICommands_atmos_describe_config_-f_yaml.stdout.golden
- tests/snapshots/TestCLICommands_atmos_terraform_deploy_locked_component.stderr.golden
- tests/snapshots/TestCLICommands_atmos_vendor_pull_(no_tty).stderr.golden
- tests/snapshots/TestCLICommands_check_atmos_--help_in_empty-dir.stdout.golden
✅ Files skipped from review due to trivial changes (1)
- tests/README.md
🚧 Files skipped from review as they are similar to previous changes (78)
- tests/snapshots/TestCLICommands_Valid_Log_Level_in_Environment_Variable.stderr.golden
- cmd/validate_stacks.go
- cmd/aws_eks_update_kubeconfig.go
- internal/exec/template_funcs_gomplate_datasource.go
- internal/exec/helmfile.go
- cmd/vendor_diff.go
- pkg/component/component_processor.go
- cmd/pro_lock.go
- internal/exec/yaml_func_terraform_output.go
- cmd/workflow.go
- cmd/pro_unlock.go
- cmd/describe_stacks.go
- pkg/aws/aws_eks_update_kubeconfig.go
- pkg/utils/hcl_utils.go
- internal/exec/terraform_utils.go
- cmd/terraform_generate_varfile.go
- internal/exec/go_getter_utils.go
- internal/exec/aws_eks_update_kubeconfig.go
- cmd/completion.go
- internal/exec/yaml_func_exec.go
- internal/exec/terraform_generate_backends.go
- pkg/validate/validate_stacks_test.go
- internal/exec/yaml_func_store.go
- internal/exec/yaml_func_env.go
- cmd/validate_component.go
- internal/exec/yaml_func_template.go
- internal/exec/template_utils.go
- internal/exec/template_funcs_component.go
- cmd/helmfile_generate_varfile.go
- cmd/describe_affected.go
- pkg/utils/json_utils.go
- cmd/terraform_generate_backends.go
- internal/exec/helmfile_generate_varfile.go
- tests/snapshots/TestCLICommands_Invalid_Log_Level_in_Config_File.stderr.golden
- cmd/describe_dependents.go
- tests/snapshots/TestCLICommands_Invalid_Log_Level_in_Environment_Variable.stderr.golden
- internal/exec/file_utils.go
- cmd/atlantis_generate_repo_config.go
- internal/exec/yaml_func_include.go
- cmd/terraform_generate_backend.go
- pkg/config/cache.go
- pkg/utils/yaml_utils.go
- cmd/version.go
- cmd/docs.go
- pkg/config/utils.go
- cmd/describe_component.go
- cmd/helmfile.go
- tests/snapshots/TestCLICommands_atmos_--help.stdout.golden
- cmd/terraform_generate_varfiles.go
- internal/exec/tar_utils.go
- internal/exec/vendor_utils.go
- internal/exec/describe_stacks.go
- cmd/vendor_pull.go
- cmd/describe_workflows.go
- internal/exec/shell_utils.go
- pkg/config/config.go
- internal/exec/vendor_component_utils.go
- cmd/validate_editorconfig.go
- internal/exec/terraform_generate_varfiles.go
- internal/exec/terraform.go
- internal/exec/validate_component.go
- internal/exec/terraform_generate_varfile.go
- pkg/hooks/cmd.go
- internal/exec/terraform_outputs.go
- internal/exec/vendor_model.go
- cmd/terraform.go
- internal/exec/workflow_utils.go
- internal/exec/atlantis_generate_repo_config.go
- internal/exec/terraform_generate_backend.go
- pkg/validate/validate_component_test.go
- internal/exec/oci_utils.go
- cmd/describe_config.go
- internal/exec/validate_stacks.go
- pkg/spacelift/spacelift_stack_processor.go
- internal/exec/utils.go
- cmd/cmd_utils.go
- internal/exec/terraform_clean.go
- main.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
internal/exec/describe_affected_utils.go
451-451: S1039: unnecessary use of fmt.Sprintf
(gosimple)
458-458: S1039: unnecessary use of fmt.Sprintf
(gosimple)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: [mock-windows] tests/fixtures/scenarios/complete
- GitHub Check: [localstack] demo-localstack
- GitHub Check: Acceptance Tests (macos-latest, macos)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Acceptance Tests (ubuntu-latest, linux)
- GitHub Check: Summary
🔇 Additional comments (5)
cmd/root.go (4)
6-6: Clean import additions!The new imports for charmbracelet logger and io package are well-organized.
Also applies to: 11-11
63-63: Excellent error handling standardization!The error handling has been consistently simplified across all locations, making the code more maintainable and readable.
Also applies to: 66-66, 78-78, 83-83, 133-133, 145-145, 150-150, 194-194, 226-226, 229-229, 234-234, 238-238
107-114: Solid file handle management!The file handle management aligns with the project's intention to maintain an open log file throughout the program's lifecycle, as confirmed in previous discussions.
131-131: Perfect logger initialization timing!The logger setup is well-placed after config initialization but before command processing, with appropriate debug logging for version command scenarios.
Also applies to: 137-138
pkg/utils/log_utils.go (1)
38-78: Verify all callers have been updated for the new function signatures.The removal of the
atmosConfigparameter affects all logging function signatures. Let's verify all callers have been updated.✅ Verification successful
All logging function calls are correctly updated 💪
The search results were false positives where
atmosConfigappears in the message content rather than as a function parameter. The logging functions are being called with the correct new signatures across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining calls with the old signature pattern echo "Checking for potentially missed updates to logging calls..." # Look for calls that might still be passing atmosConfig rg -l 'Log(Error(AndExit)?|Trace|Debug|Info|Warning)\s*\([^)]*atmosConfig' \ --type go \ --glob '!pkg/utils/log_utils.go'Length of output: 330
Script:
#!/bin/bash # Show the actual logging calls in context echo "=== internal/exec/terraform.go ===" rg 'Log(Error(AndExit)?|Trace|Debug|Info|Warning)\s*\([^)]*atmosConfig' internal/exec/terraform.go -B 1 -A 1 echo -e "\n=== internal/exec/helmfile.go ===" rg 'Log(Error(AndExit)?|Trace|Debug|Info|Warning)\s*\([^)]*atmosConfig' internal/exec/helmfile.go -B 1 -A 1 echo -e "\n=== cmd/validate_editorconfig.go ===" rg 'Log(Error(AndExit)?|Trace|Debug|Info|Warning)\s*\([^)]*atmosConfig' cmd/validate_editorconfig.go -B 1 -A 1Length of output: 1317
mcalhoun
force-pushed
the
dev-2668-migrate-to-charmbracelet-logger
branch
2 times, most recently
from
f603758 to
5154db6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
internal/exec/describe_affected_utils.go (2)
76-76: Optimize string formatting in logging statements.The logging statements are informative, but some use unnecessary
fmt.Sprintf. For better performance, you can directly pass the string when no formatting is needed.Apply these changes:
-u.LogTrace(fmt.Sprintf("\nCloning repo '%s' into the temp dir '%s'", localRepoInfo.RepoUrl, tempDir)) +u.LogTrace(fmt.Sprintf("\nCloning repo '%s' into the temp dir '%s'", localRepoInfo.RepoUrl, tempDir)) // Keep as is - needs formatting -u.LogTrace(fmt.Sprintf("\nCloning Git ref '%s' ...\n", ref)) +u.LogTrace(fmt.Sprintf("\nCloning Git ref '%s' ...\n", ref)) // Keep as is - needs formatting -u.LogTrace("\nCloned the HEAD of the default branch ...\n") +u.LogTrace("\nCloned the HEAD of the default branch ...\n") // Keep as is - no formatting needed -u.LogTrace(fmt.Sprintf("\nCloned Git ref '%s'\n", ref)) +u.LogTrace(fmt.Sprintf("\nCloned Git ref '%s'\n", ref)) // Keep as is - needs formatting -u.LogTrace(fmt.Sprintf("\nCloned Git ref '%s'\n", remoteRepoHead.Name())) +u.LogTrace(fmt.Sprintf("\nCloned Git ref '%s'\n", remoteRepoHead.Name())) // Keep as is - needs formatting -u.LogTrace(fmt.Sprintf("\nChecking out commit SHA '%s' ...\n", sha)) +u.LogTrace(fmt.Sprintf("\nChecking out commit SHA '%s' ...\n", sha)) // Keep as is - needs formatting -u.LogTrace(fmt.Sprintf("\nChecked out commit SHA '%s'\n", sha)) +u.LogTrace(fmt.Sprintf("\nChecked out commit SHA '%s'\n", sha)) // Keep as is - needs formattingAlso applies to: 87-87, 89-89, 129-129, 131-131, 136-136, 155-155
407-408: Optimize string formatting in Git operation logs.The logging provides excellent tracing of Git operations. However, some statements use unnecessary
fmt.Sprintf.Apply these changes:
-u.LogTrace(fmt.Sprintf("Current HEAD: %s", localRepoHead)) +u.LogTrace(fmt.Sprintf("Current HEAD: %s", localRepoHead)) // Keep as is - needs formatting -u.LogTrace(fmt.Sprintf("BASE: %s", remoteRepoHead)) +u.LogTrace(fmt.Sprintf("BASE: %s", remoteRepoHead)) // Keep as is - needs formatting -u.LogTrace(fmt.Sprintf("\nGetting current working repo commit object...")) +u.LogTrace("\nGetting current working repo commit object...") // Remove unnecessary fmt.Sprintf -u.LogTrace(fmt.Sprintf("Got current working repo commit object")) +u.LogTrace("Got current working repo commit object") // Remove unnecessary fmt.Sprintf -u.LogTrace(fmt.Sprintf("Getting current working repo commit tree...")) +u.LogTrace("Getting current working repo commit tree...") // Remove unnecessary fmt.SprintfAlso applies to: 451-451, 458-459, 466-467, 474-475, 482-483, 494-495, 498-498, 501-501, 503-503
main.go (1)
11-12: Consider documenting timestamp configuration in code.The comment about disabling timestamps for snapshots is helpful, but consider adding a link to a tracking issue or TODO for the future PR that will address styles.
pkg/utils/log_utils.go (1)
67-78: Clean and consistent logging implementation.The simplified logging functions with direct mapping to charmbracelet logger levels are clean and maintainable. Consider adding debug statements to track the transition period:
func LogDebug(message string) { + // TODO: Remove after migration period (2024-Q2) + l.Debug("legacy_debug_call", "message", message) l.Debug(message) }cmd/root.go (2)
128-128: Use structured logging fields instead of string concatenation.The charmbracelet logger supports structured logging. Let's leverage this feature for better log parsing.
-log.Debug("warning: CLI configuration 'atmos.yaml' file not found", "error", initErr) +log.Debug("CLI configuration file not found", "file", "atmos.yaml", "error", initErr)
90-93: Consolidate duplicate log level cases.Both "Trace" and "Debug" set the same log level. Consider consolidating them or implementing proper trace level support.
switch atmosConfig.Logs.Level { -case "Trace": - log.SetLevel(log.DebugLevel) case "Debug": log.SetLevel(log.DebugLevel)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (82)
cmd/atlantis_generate_repo_config.go(1 hunks)cmd/aws_eks_update_kubeconfig.go(1 hunks)cmd/cmd_utils.go(13 hunks)cmd/completion.go(1 hunks)cmd/describe_affected.go(1 hunks)cmd/describe_component.go(2 hunks)cmd/describe_config.go(1 hunks)cmd/describe_dependents.go(2 hunks)cmd/describe_stacks.go(1 hunks)cmd/describe_workflows.go(1 hunks)cmd/docs.go(5 hunks)cmd/helmfile.go(1 hunks)cmd/helmfile_generate_varfile.go(2 hunks)cmd/pro_lock.go(1 hunks)cmd/pro_unlock.go(1 hunks)cmd/root.go(6 hunks)cmd/terraform.go(1 hunks)cmd/terraform_generate_backend.go(2 hunks)cmd/terraform_generate_backends.go(1 hunks)cmd/terraform_generate_varfile.go(2 hunks)cmd/terraform_generate_varfiles.go(2 hunks)cmd/validate_component.go(2 hunks)cmd/validate_editorconfig.go(3 hunks)cmd/validate_stacks.go(1 hunks)cmd/vendor_diff.go(1 hunks)cmd/vendor_pull.go(1 hunks)cmd/version.go(2 hunks)cmd/workflow.go(5 hunks)internal/exec/atlantis_generate_repo_config.go(1 hunks)internal/exec/aws_eks_update_kubeconfig.go(1 hunks)internal/exec/describe_affected_utils.go(13 hunks)internal/exec/describe_stacks.go(2 hunks)internal/exec/file_utils.go(1 hunks)internal/exec/go_getter_utils.go(2 hunks)internal/exec/helmfile.go(7 hunks)internal/exec/helmfile_generate_varfile.go(2 hunks)internal/exec/oci_utils.go(1 hunks)internal/exec/shell_utils.go(10 hunks)internal/exec/tar_utils.go(2 hunks)internal/exec/template_funcs_component.go(2 hunks)internal/exec/template_funcs_gomplate_datasource.go(3 hunks)internal/exec/template_utils.go(3 hunks)internal/exec/terraform.go(11 hunks)internal/exec/terraform_clean.go(6 hunks)internal/exec/terraform_generate_backend.go(2 hunks)internal/exec/terraform_generate_backends.go(2 hunks)internal/exec/terraform_generate_varfile.go(2 hunks)internal/exec/terraform_generate_varfiles.go(2 hunks)internal/exec/terraform_outputs.go(14 hunks)internal/exec/terraform_utils.go(1 hunks)internal/exec/utils.go(5 hunks)internal/exec/validate_component.go(2 hunks)internal/exec/validate_stacks.go(2 hunks)internal/exec/vendor_component_utils.go(4 hunks)internal/exec/vendor_model.go(3 hunks)internal/exec/vendor_utils.go(6 hunks)internal/exec/workflow_utils.go(4 hunks)internal/exec/yaml_func_env.go(3 hunks)internal/exec/yaml_func_exec.go(1 hunks)internal/exec/yaml_func_include.go(3 hunks)internal/exec/yaml_func_store.go(1 hunks)internal/exec/yaml_func_template.go(1 hunks)internal/exec/yaml_func_terraform_output.go(2 hunks)main.go(1 hunks)pkg/aws/aws_eks_update_kubeconfig.go(1 hunks)pkg/component/component_processor.go(3 hunks)pkg/config/cache.go(1 hunks)pkg/config/config.go(3 hunks)pkg/config/utils.go(8 hunks)pkg/hooks/cmd.go(3 hunks)pkg/spacelift/spacelift_stack_processor.go(9 hunks)pkg/utils/hcl_utils.go(2 hunks)pkg/utils/json_utils.go(1 hunks)pkg/utils/log_utils.go(2 hunks)pkg/utils/yaml_utils.go(1 hunks)pkg/validate/validate_component_test.go(4 hunks)pkg/validate/validate_stacks_test.go(1 hunks)tests/README.md(1 hunks)tests/snapshots/TestCLICommands_Invalid_Log_Level_in_Config_File.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_Invalid_Log_Level_in_Environment_Variable.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_Valid_Log_Level_in_Environment_Variable.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_--help.stdout.golden(1 hunks)
⛔ Files not processed due to max files limit (6)
- tests/snapshots/TestCLICommands_atmos_circuit-breaker.stderr.golden
- tests/snapshots/TestCLICommands_atmos_describe_config.stdout.golden
- tests/snapshots/TestCLICommands_atmos_describe_config_-f_yaml.stdout.golden
- tests/snapshots/TestCLICommands_atmos_terraform_deploy_locked_component.stderr.golden
- tests/snapshots/TestCLICommands_atmos_vendor_pull_(no_tty).stderr.golden
- tests/snapshots/TestCLICommands_check_atmos_--help_in_empty-dir.stdout.golden
🚧 Files skipped from review as they are similar to previous changes (78)
- tests/snapshots/TestCLICommands_Invalid_Log_Level_in_Environment_Variable.stderr.golden
- cmd/atlantis_generate_repo_config.go
- pkg/config/cache.go
- internal/exec/template_funcs_gomplate_datasource.go
- internal/exec/terraform_generate_varfile.go
- tests/snapshots/TestCLICommands_Valid_Log_Level_in_Environment_Variable.stderr.golden
- cmd/aws_eks_update_kubeconfig.go
- cmd/pro_unlock.go
- tests/README.md
- internal/exec/yaml_func_exec.go
- pkg/component/component_processor.go
- internal/exec/go_getter_utils.go
- internal/exec/yaml_func_env.go
- cmd/vendor_diff.go
- internal/exec/yaml_func_store.go
- cmd/helmfile_generate_varfile.go
- cmd/describe_config.go
- internal/exec/helmfile.go
- cmd/describe_stacks.go
- pkg/validate/validate_component_test.go
- pkg/utils/yaml_utils.go
- cmd/describe_affected.go
- cmd/pro_lock.go
- internal/exec/file_utils.go
- internal/exec/aws_eks_update_kubeconfig.go
- internal/exec/terraform_generate_backend.go
- cmd/terraform_generate_varfiles.go
- cmd/helmfile.go
- internal/exec/atlantis_generate_repo_config.go
- internal/exec/oci_utils.go
- internal/exec/yaml_func_template.go
- cmd/terraform_generate_backends.go
- internal/exec/terraform_generate_backends.go
- cmd/terraform.go
- pkg/utils/hcl_utils.go
- cmd/describe_workflows.go
- internal/exec/helmfile_generate_varfile.go
- pkg/aws/aws_eks_update_kubeconfig.go
- cmd/completion.go
- pkg/utils/json_utils.go
- internal/exec/template_funcs_component.go
- cmd/docs.go
- cmd/terraform_generate_backend.go
- internal/exec/template_utils.go
- cmd/validate_stacks.go
- internal/exec/describe_stacks.go
- tests/snapshots/TestCLICommands_Invalid_Log_Level_in_Config_File.stderr.golden
- cmd/describe_dependents.go
- tests/snapshots/TestCLICommands_atmos_--help.stdout.golden
- internal/exec/terraform_outputs.go
- cmd/validate_component.go
- cmd/describe_component.go
- internal/exec/terraform_utils.go
- internal/exec/vendor_component_utils.go
- cmd/vendor_pull.go
- cmd/workflow.go
- internal/exec/vendor_model.go
- internal/exec/yaml_func_include.go
- cmd/version.go
- internal/exec/terraform_generate_varfiles.go
- cmd/validate_editorconfig.go
- internal/exec/vendor_utils.go
- internal/exec/terraform_clean.go
- internal/exec/shell_utils.go
- pkg/config/config.go
- internal/exec/utils.go
- cmd/cmd_utils.go
- internal/exec/terraform.go
- pkg/config/utils.go
- cmd/terraform_generate_varfile.go
- pkg/hooks/cmd.go
- internal/exec/workflow_utils.go
- internal/exec/validate_stacks.go
- internal/exec/yaml_func_terraform_output.go
- internal/exec/validate_component.go
- pkg/spacelift/spacelift_stack_processor.go
- pkg/validate/validate_stacks_test.go
- internal/exec/tar_utils.go
🧰 Additional context used
📓 Learnings (1)
pkg/utils/log_utils.go (1)
Learnt from: mcalhoun
PR: cloudposse/atmos#980
File: pkg/utils/log_utils.go:62-63
Timestamp: 2025-01-30T16:56:43.004Z
Learning: In pkg/utils/log_utils.go, LogTrace is intentionally redirected to LogDebug since charmbracelet logger doesn't support Trace level, maintaining backward compatibility with the original LogTrace functionality.
🪛 golangci-lint (1.62.2)
internal/exec/describe_affected_utils.go
451-451: S1039: unnecessary use of fmt.Sprintf
(gosimple)
458-458: S1039: unnecessary use of fmt.Sprintf
(gosimple)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (7)
internal/exec/describe_affected_utils.go (2)
211-211: LGTM! Good error handling and tracing.The logging statements provide clear tracing of the checkout process, and the error handling for Git operations is robust.
Also applies to: 239-239, 256-256, 275-275, 282-282, 307-307
1528-1528: LGTM! Clear component status logging.The logging statement clearly indicates when a component is skipped due to being disabled.
pkg/utils/log_utils.go (3)
38-47: Well-structured error handling with proper exit codes!The error handling is now more robust with proper exit code extraction from exec.ExitError. This is a significant improvement over the previous implementation.
51-58: Clean implementation of debug stack traces.Good job tying stack trace printing to the debug level. This provides valuable debugging information while keeping normal logs clean.
62-63: Maintaining backward compatibility with LogTrace.Based on the retrieved learning, redirecting LogTrace to LogDebug is intentional since charmbracelet logger doesn't support Trace level. This is a good approach to maintain backward compatibility.
cmd/root.go (2)
11-11: LGTM! Clean import of charmbracelet logger.The import is properly placed and follows Go conventions.
134-135: LGTM! Well-placed logger initialization.The logger setup is correctly positioned after config initialization but before command processing, ensuring proper log capture.
mcalhoun
force-pushed
the
dev-2668-migrate-to-charmbracelet-logger
branch
from
5154db6 to
f858200
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
cmd/root.go (1)
88-112: Consider adding log rotation support.The logger setup is well-structured, but for production environments, consider adding log rotation to prevent unbounded file growth.
Consider using a rolling file writer:
import "gopkg.in/natefinch/lumberjack.v2" // In setupLogger if atmosConfig.Logs.File != "/dev/stderr" { log.SetOutput(&lumberjack.Logger{ Filename: atmosConfig.Logs.File, MaxSize: 100, // megabytes MaxBackups: 3, MaxAge: 28, // days Compress: true, }) }internal/exec/describe_affected_utils.go (5)
Line range hint
76-89: Optimize string formatting in logging calls.The
fmt.Sprintfcalls are unnecessary when no string formatting is required. Let's simplify these logging statements.-u.LogTrace(fmt.Sprintf("\nCloning repo '%s' into the temp dir '%s'", localRepoInfo.RepoUrl, tempDir)) +u.LogTrace(fmt.Sprintf("\nCloning repo '%s' into the temp dir '%s'", localRepoInfo.RepoUrl, tempDir)) -u.LogTrace(fmt.Sprintf("\nCloning Git ref '%s' ...\n", ref)) +u.LogTrace("\nCloning Git ref '" + ref + "' ...\n") -u.LogTrace("\nCloned the HEAD of the default branch ...\n") +u.LogTrace("\nCloned the HEAD of the default branch ...\n")
Line range hint
129-155: Simplify string formatting in Git operation logs.Similar to the previous section, let's optimize the string formatting in these logging statements.
-u.LogTrace(fmt.Sprintf("\nCloned Git ref '%s'\n", ref)) +u.LogTrace("\nCloned Git ref '" + ref + "'\n") -u.LogTrace(fmt.Sprintf("\nCloned Git ref '%s'\n", remoteRepoHead.Name())) +u.LogTrace("\nCloned Git ref '" + remoteRepoHead.Name().String() + "'\n") -u.LogTrace(fmt.Sprintf("\nChecking out commit SHA '%s' ...\n", sha)) +u.LogTrace("\nChecking out commit SHA '" + sha + "' ...\n") -u.LogTrace(fmt.Sprintf("\nChecked out commit SHA '%s'\n", sha)) +u.LogTrace("\nChecked out commit SHA '" + sha + "'\n")
Line range hint
211-307: Optimize string formatting in filesystem operation logs.Let's streamline the string formatting in these logging statements while maintaining their clarity.
-u.LogTrace(fmt.Sprintf("\nCopying the local repo into the temp directory '%s' ...", tempDir)) +u.LogTrace("\nCopying the local repo into the temp directory '" + tempDir + "' ...") -u.LogTrace(fmt.Sprintf("Copied the local repo into the temp directory '%s'\n", tempDir)) +u.LogTrace("Copied the local repo into the temp directory '" + tempDir + "'\n") -u.LogTrace(fmt.Sprintf("\nChecking out commit SHA '%s' ...\n", sha)) +u.LogTrace("\nChecking out commit SHA '" + sha + "' ...\n") -u.LogTrace(fmt.Sprintf("Checked out commit SHA '%s'\n", sha)) +u.LogTrace("Checked out commit SHA '" + sha + "'\n") -u.LogTrace(fmt.Sprintf("\nChecking out Git ref '%s' ...", ref)) +u.LogTrace("\nChecking out Git ref '" + ref + "' ...") -u.LogTrace(fmt.Sprintf("Checked out Git ref '%s'\n", ref)) +u.LogTrace("Checked out Git ref '" + ref + "'\n")
Line range hint
407-503: Optimize simple string formatting while keeping complex ones.The logging statements provide excellent visibility into the Git operations. Let's optimize only the simple string formatting cases while keeping the necessary fmt.Sprintf calls.
-u.LogTrace(fmt.Sprintf("Current HEAD: %s", localRepoHead)) +u.LogTrace("Current HEAD: " + localRepoHead.String()) -u.LogTrace(fmt.Sprintf("BASE: %s", remoteRepoHead)) +u.LogTrace("BASE: " + remoteRepoHead.String()) -u.LogTrace(fmt.Sprintf("\nGetting current working repo commit object...")) +u.LogTrace("\nGetting current working repo commit object...") -u.LogTrace(fmt.Sprintf("Got current working repo commit object")) +u.LogTrace("Got current working repo commit object") -u.LogTrace(fmt.Sprintf("Getting current working repo commit tree...")) +u.LogTrace("Getting current working repo commit tree...") -u.LogTrace(fmt.Sprintf("Got current working repo commit tree")) +u.LogTrace("Got current working repo commit tree") -u.LogTrace(fmt.Sprintf("Getting remote repo commit object...")) +u.LogTrace("Getting remote repo commit object...") -u.LogTrace(fmt.Sprintf("Got remote repo commit object")) +u.LogTrace("Got remote repo commit object") -u.LogTrace(fmt.Sprintf("Getting remote repo commit tree...")) +u.LogTrace("Getting remote repo commit tree...") -u.LogTrace(fmt.Sprintf("Got remote repo commit tree")) +u.LogTrace("Got remote repo commit tree") -u.LogTrace(fmt.Sprintf("Finding difference between the current working branch and remote target branch ...")) +u.LogTrace("Finding difference between the current working branch and remote target branch ...") -u.LogTrace(fmt.Sprintf("Found difference between the current working branch and remote target branch")) +u.LogTrace("Found difference between the current working branch and remote target branch") -u.LogTrace(fmt.Sprintf("The current working branch and remote target branch are the same")) +u.LogTrace("The current working branch and remote target branch are the same")🧰 Tools
🪛 golangci-lint (1.62.2)
451-451: S1039: unnecessary use of fmt.Sprintf
(gosimple)
458-458: S1039: unnecessary use of fmt.Sprintf
(gosimple)
1528-1528: Simplify string formatting for disabled component log.Let's optimize this simple logging statement.
-u.LogTrace(fmt.Sprintf("Skipping disabled component %s", componentName)) +u.LogTrace("Skipping disabled component " + componentName)tests/snapshots/TestCLICommands_atmos_circuit-breaker.stderr.golden (1)
1-1: Consider enhancing the error message clarity.The error message format with the "ERRO" prefix aligns well with charmbracelet logger standards. However, we could make the message more user-friendly.
-ERRO ATMOS_SHLVL (11) exceeds maximum allowed depth (10). Infinite recursion? +ERRO Maximum recursion depth exceeded: ATMOS_SHLVL is 11 (limit: 10). Check for circular dependencies.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (82)
cmd/atlantis_generate_repo_config.go(1 hunks)cmd/aws_eks_update_kubeconfig.go(1 hunks)cmd/cmd_utils.go(13 hunks)cmd/completion.go(1 hunks)cmd/describe_affected.go(1 hunks)cmd/describe_component.go(2 hunks)cmd/describe_config.go(1 hunks)cmd/describe_dependents.go(2 hunks)cmd/describe_stacks.go(1 hunks)cmd/describe_workflows.go(1 hunks)cmd/docs.go(5 hunks)cmd/helmfile.go(1 hunks)cmd/helmfile_generate_varfile.go(2 hunks)cmd/pro_lock.go(1 hunks)cmd/pro_unlock.go(1 hunks)cmd/root.go(6 hunks)cmd/terraform.go(1 hunks)cmd/terraform_generate_backend.go(2 hunks)cmd/terraform_generate_backends.go(1 hunks)cmd/terraform_generate_varfile.go(2 hunks)cmd/terraform_generate_varfiles.go(2 hunks)cmd/validate_component.go(2 hunks)cmd/validate_editorconfig.go(3 hunks)cmd/validate_stacks.go(1 hunks)cmd/vendor_diff.go(1 hunks)cmd/vendor_pull.go(1 hunks)cmd/version.go(2 hunks)cmd/workflow.go(5 hunks)internal/exec/atlantis_generate_repo_config.go(1 hunks)internal/exec/aws_eks_update_kubeconfig.go(1 hunks)internal/exec/describe_affected_utils.go(13 hunks)internal/exec/describe_stacks.go(2 hunks)internal/exec/file_utils.go(1 hunks)internal/exec/go_getter_utils.go(2 hunks)internal/exec/helmfile.go(7 hunks)internal/exec/helmfile_generate_varfile.go(2 hunks)internal/exec/oci_utils.go(1 hunks)internal/exec/shell_utils.go(10 hunks)internal/exec/tar_utils.go(2 hunks)internal/exec/template_funcs_component.go(2 hunks)internal/exec/template_funcs_gomplate_datasource.go(3 hunks)internal/exec/template_utils.go(3 hunks)internal/exec/terraform.go(11 hunks)internal/exec/terraform_clean.go(6 hunks)internal/exec/terraform_generate_backend.go(2 hunks)internal/exec/terraform_generate_backends.go(2 hunks)internal/exec/terraform_generate_varfile.go(2 hunks)internal/exec/terraform_generate_varfiles.go(2 hunks)internal/exec/terraform_outputs.go(14 hunks)internal/exec/terraform_utils.go(1 hunks)internal/exec/utils.go(5 hunks)internal/exec/validate_component.go(2 hunks)internal/exec/validate_stacks.go(2 hunks)internal/exec/vendor_component_utils.go(4 hunks)internal/exec/vendor_model.go(3 hunks)internal/exec/vendor_utils.go(6 hunks)internal/exec/workflow_utils.go(4 hunks)internal/exec/yaml_func_env.go(3 hunks)internal/exec/yaml_func_exec.go(1 hunks)internal/exec/yaml_func_include.go(3 hunks)internal/exec/yaml_func_store.go(1 hunks)internal/exec/yaml_func_template.go(1 hunks)internal/exec/yaml_func_terraform_output.go(2 hunks)main.go(1 hunks)pkg/aws/aws_eks_update_kubeconfig.go(1 hunks)pkg/component/component_processor.go(3 hunks)pkg/config/cache.go(1 hunks)pkg/config/config.go(3 hunks)pkg/config/utils.go(8 hunks)pkg/hooks/cmd.go(3 hunks)pkg/spacelift/spacelift_stack_processor.go(9 hunks)pkg/utils/hcl_utils.go(2 hunks)pkg/utils/json_utils.go(1 hunks)pkg/utils/log_utils.go(2 hunks)pkg/utils/yaml_utils.go(1 hunks)pkg/validate/validate_component_test.go(4 hunks)pkg/validate/validate_stacks_test.go(1 hunks)tests/README.md(1 hunks)tests/snapshots/TestCLICommands_Invalid_Log_Level_in_Config_File.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_Invalid_Log_Level_in_Environment_Variable.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_Valid_Log_Level_in_Environment_Variable.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_circuit-breaker.stderr.golden(1 hunks)
⛔ Files not processed due to max files limit (5)
- tests/snapshots/TestCLICommands_atmos_describe_config.stdout.golden
- tests/snapshots/TestCLICommands_atmos_describe_config_-f_yaml.stdout.golden
- tests/snapshots/TestCLICommands_atmos_terraform_deploy_locked_component.stderr.golden
- tests/snapshots/TestCLICommands_atmos_vendor_pull_(no_tty).stderr.golden
- tests/snapshots/TestCLICommands_check_atmos_--help_in_empty-dir.stdout.golden
🚧 Files skipped from review as they are similar to previous changes (78)
- tests/snapshots/TestCLICommands_Invalid_Log_Level_in_Config_File.stderr.golden
- internal/exec/yaml_func_exec.go
- tests/README.md
- internal/exec/yaml_func_template.go
- cmd/aws_eks_update_kubeconfig.go
- pkg/component/component_processor.go
- cmd/pro_lock.go
- internal/exec/terraform_generate_backends.go
- tests/snapshots/TestCLICommands_Valid_Log_Level_in_Environment_Variable.stderr.golden
- internal/exec/yaml_func_env.go
- internal/exec/yaml_func_terraform_output.go
- cmd/vendor_diff.go
- internal/exec/aws_eks_update_kubeconfig.go
- internal/exec/template_funcs_gomplate_datasource.go
- internal/exec/yaml_func_store.go
- cmd/describe_stacks.go
- internal/exec/yaml_func_include.go
- cmd/describe_workflows.go
- cmd/terraform_generate_varfile.go
- cmd/pro_unlock.go
- tests/snapshots/TestCLICommands_Invalid_Log_Level_in_Environment_Variable.stderr.golden
- cmd/terraform_generate_backends.go
- internal/exec/go_getter_utils.go
- cmd/workflow.go
- internal/exec/terraform_generate_varfiles.go
- internal/exec/template_utils.go
- cmd/helmfile_generate_varfile.go
- cmd/describe_affected.go
- internal/exec/vendor_component_utils.go
- cmd/version.go
- internal/exec/oci_utils.go
- cmd/validate_stacks.go
- pkg/utils/hcl_utils.go
- internal/exec/terraform_outputs.go
- pkg/config/cache.go
- internal/exec/terraform_generate_backend.go
- internal/exec/terraform_generate_varfile.go
- pkg/validate/validate_stacks_test.go
- cmd/terraform_generate_backend.go
- cmd/describe_config.go
- cmd/describe_dependents.go
- internal/exec/file_utils.go
- pkg/validate/validate_component_test.go
- internal/exec/helmfile.go
- internal/exec/terraform_utils.go
- cmd/describe_component.go
- internal/exec/validate_stacks.go
- pkg/config/utils.go
- cmd/terraform.go
- main.go
- cmd/terraform_generate_varfiles.go
- internal/exec/vendor_utils.go
- internal/exec/tar_utils.go
- pkg/spacelift/spacelift_stack_processor.go
- cmd/validate_component.go
- internal/exec/vendor_model.go
- pkg/utils/json_utils.go
- cmd/docs.go
- internal/exec/utils.go
- internal/exec/describe_stacks.go
- internal/exec/template_funcs_component.go
- cmd/vendor_pull.go
- cmd/atlantis_generate_repo_config.go
- cmd/helmfile.go
- internal/exec/terraform_clean.go
- internal/exec/shell_utils.go
- internal/exec/terraform.go
- internal/exec/atlantis_generate_repo_config.go
- cmd/cmd_utils.go
- pkg/aws/aws_eks_update_kubeconfig.go
- internal/exec/validate_component.go
- cmd/validate_editorconfig.go
- cmd/completion.go
- pkg/config/config.go
- pkg/hooks/cmd.go
- pkg/utils/yaml_utils.go
- internal/exec/helmfile_generate_varfile.go
- internal/exec/workflow_utils.go
🧰 Additional context used
📓 Learnings (1)
pkg/utils/log_utils.go (1)
Learnt from: mcalhoun
PR: cloudposse/atmos#980
File: pkg/utils/log_utils.go:62-63
Timestamp: 2025-01-30T16:56:43.004Z
Learning: In pkg/utils/log_utils.go, LogTrace is intentionally redirected to LogDebug since charmbracelet logger doesn't support Trace level, maintaining backward compatibility with the original LogTrace functionality.
🪛 golangci-lint (1.62.2)
internal/exec/describe_affected_utils.go
451-451: S1039: unnecessary use of fmt.Sprintf
(gosimple)
458-458: S1039: unnecessary use of fmt.Sprintf
(gosimple)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (7)
pkg/utils/log_utils.go (4)
38-47: LGTM! Clean error handling implementation.The error handling logic is well-structured:
- Logs the error using charmbracelet logger
- Extracts exit code from exec.ExitError if available
- Falls back to exit code 1 for other errors
51-58: Nice touch with the stack trace for debug level!Good implementation of conditional stack trace printing based on debug level.
62-63: Maintaining backward compatibility with LogTrace.As per the retrieved learning, this redirection to LogDebug is intentional since charmbracelet logger doesn't support Trace level.
67-78: Clean and consistent logging wrappers.The logging functions provide a clean interface to charmbracelet logger while maintaining consistent naming with the rest of the codebase.
cmd/root.go (2)
134-135: LGTM! Strategic logger setup placement.Good placement of setupLogger call after config initialization but before command processing.
63-66: Clean error handling migration.The error handling has been consistently simplified across command processing, maintaining a clean and uniform approach.
Also applies to: 78-83, 142-147
tests/snapshots/TestCLICommands_atmos_circuit-breaker.stderr.golden (1)
2-11: Verify if multiple identical exit status messages are necessary.The repeated "exit status 1" messages might be overwhelming for users. Consider consolidating these into a single message with a count, or ensuring this repetition serves a specific purpose.
tests/snapshots/TestCLICommands_check_atmos_--help_in_empty-dir.stdout.golden
Outdated
Show resolved
Hide resolved
tests/snapshots/TestCLICommands_atmos_vendor_pull_(no_tty).stderr.golden
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
internal/exec/terraform_generate_varfiles.go (1)
326-330: LGTM! Debug logging statements updated.The debug logging statements have been simplified by removing the
atmosConfigparameter, aligning with the migration to Charmbracelet Logger. The log messages provide clear context about the varfile generation process.However, consider structuring the debug logs to make them more machine-parseable.
- u.LogDebug(fmt.Sprintf("varfile: %s", fileName)) - u.LogDebug(fmt.Sprintf("terraform component: %s", terraformComponent)) - u.LogDebug(fmt.Sprintf("atmos component: %s", componentName)) - u.LogDebug(fmt.Sprintf("atmos stack: %s", stackName)) - u.LogDebug(fmt.Sprintf("stack config file: %s", stackFileName)) + u.LogDebug("generating varfile", map[string]interface{}{ + "varfile": fileName, + "terraform_component": terraformComponent, + "atmos_component": componentName, + "atmos_stack": stackName, + "stack_config_file": stackFileName, + })internal/exec/describe_affected_utils.go (1)
403-404: Optimize string formatting in logging statements.The logging statements effectively track Git operations, but there are opportunities to optimize the code by removing unnecessary
fmt.Sprintfcalls.Apply this diff to optimize the code:
-u.LogTrace(fmt.Sprintf("Current HEAD: %s", localRepoHead)) +u.LogTrace("Current HEAD: " + localRepoHead.String()) -u.LogTrace(fmt.Sprintf("Getting current working repo commit object...")) +u.LogTrace("Getting current working repo commit object...")Also applies to: 447-447, 454-455, 462-463, 470-471, 478-479, 490-491, 494-494, 497-497, 499-499
cmd/root.go (1)
88-112: Enhance device handling in logger setup.Consider handling all /dev devices consistently, not just /dev/stderr.
Apply this diff:
-if atmosConfig.Logs.File != "/dev/stderr" { +if !strings.HasPrefix(atmosConfig.Logs.File, "/dev/") {Also, consider falling back to stderr when file opening fails instead of fatal:
if err != nil { - log.Fatal("Failed to open log file:", err) + log.Warn("Failed to open log file, falling back to stderr", "file", atmosConfig.Logs.File, "error", err) + return }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
cmd/cmd_utils.go(16 hunks)cmd/describe_config.go(1 hunks)cmd/describe_stacks.go(1 hunks)cmd/root.go(6 hunks)cmd/validate_editorconfig.go(4 hunks)internal/exec/atlantis_generate_repo_config.go(2 hunks)internal/exec/describe_affected_utils.go(13 hunks)internal/exec/describe_stacks.go(2 hunks)internal/exec/file_utils.go(3 hunks)internal/exec/helmfile.go(7 hunks)internal/exec/helmfile_generate_varfile.go(2 hunks)internal/exec/shell_utils.go(11 hunks)internal/exec/template_funcs_component.go(2 hunks)internal/exec/template_funcs_gomplate_datasource.go(2 hunks)internal/exec/terraform.go(11 hunks)internal/exec/terraform_clean.go(7 hunks)internal/exec/terraform_generate_backend.go(2 hunks)internal/exec/terraform_generate_backends.go(3 hunks)internal/exec/terraform_generate_varfile.go(2 hunks)internal/exec/terraform_generate_varfiles.go(2 hunks)internal/exec/terraform_outputs.go(15 hunks)internal/exec/utils.go(31 hunks)internal/exec/validate_stacks.go(3 hunks)internal/exec/vendor_model.go(4 hunks)internal/exec/vendor_utils.go(6 hunks)internal/exec/workflow_utils.go(7 hunks)internal/exec/yaml_func_exec.go(1 hunks)internal/exec/yaml_func_store.go(1 hunks)internal/exec/yaml_func_template.go(1 hunks)pkg/aws/aws_eks_update_kubeconfig.go(1 hunks)pkg/component/component_processor.go(3 hunks)pkg/config/cache.go(2 hunks)pkg/config/config.go(3 hunks)pkg/config/utils.go(9 hunks)pkg/spacelift/spacelift_stack_processor.go(9 hunks)pkg/utils/hcl_utils.go(2 hunks)pkg/utils/log_utils.go(2 hunks)pkg/utils/yaml_utils.go(2 hunks)tests/snapshots/TestCLICommands_Valid_Log_Level_in_Environment_Variable.stderr.golden(1 hunks)tests/snapshots/TestCLICommands_atmos_terraform_deploy_locked_component.stderr.golden(1 hunks)tests/test-cases/log-level-validation.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (35)
- cmd/describe_stacks.go
- tests/snapshots/TestCLICommands_Valid_Log_Level_in_Environment_Variable.stderr.golden
- internal/exec/template_funcs_component.go
- internal/exec/yaml_func_exec.go
- cmd/describe_config.go
- pkg/component/component_processor.go
- pkg/aws/aws_eks_update_kubeconfig.go
- internal/exec/yaml_func_template.go
- internal/exec/yaml_func_store.go
- pkg/utils/hcl_utils.go
- internal/exec/template_funcs_gomplate_datasource.go
- pkg/utils/yaml_utils.go
- internal/exec/terraform_generate_backend.go
- internal/exec/vendor_model.go
- pkg/config/cache.go
- pkg/config/utils.go
- pkg/spacelift/spacelift_stack_processor.go
- internal/exec/terraform_outputs.go
- internal/exec/helmfile_generate_varfile.go
- internal/exec/describe_stacks.go
- internal/exec/helmfile.go
- internal/exec/atlantis_generate_repo_config.go
- internal/exec/vendor_utils.go
- internal/exec/terraform_generate_varfile.go
- internal/exec/file_utils.go
- internal/exec/validate_stacks.go
- internal/exec/terraform.go
- internal/exec/terraform_clean.go
- cmd/cmd_utils.go
- internal/exec/workflow_utils.go
- cmd/validate_editorconfig.go
- internal/exec/terraform_generate_backends.go
- internal/exec/utils.go
- internal/exec/shell_utils.go
- pkg/config/config.go
🧰 Additional context used
📓 Learnings (1)
pkg/utils/log_utils.go (1)
Learnt from: mcalhoun
PR: cloudposse/atmos#980
File: pkg/utils/log_utils.go:62-63
Timestamp: 2025-01-30T16:56:43.004Z
Learning: In pkg/utils/log_utils.go, LogTrace is intentionally redirected to LogDebug since charmbracelet logger doesn't support Trace level, maintaining backward compatibility with the original LogTrace functionality.
🪛 golangci-lint (1.62.2)
internal/exec/describe_affected_utils.go
447-447: S1039: unnecessary use of fmt.Sprintf
(gosimple)
454-454: S1039: unnecessary use of fmt.Sprintf
(gosimple)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (13)
tests/snapshots/TestCLICommands_atmos_terraform_deploy_locked_component.stderr.golden (1)
1-1: Well done on standardizing the error format!The addition of the "ERRO" prefix aligns perfectly with the migration to charmbracelet logger and establishes a consistent logging format. This standardization will improve log readability and make error tracking more efficient.
internal/exec/terraform_generate_varfiles.go (2)
269-269: LGTM! Error handling looks good.The error handling has been simplified by removing the
atmosConfigparameter fromLogErrorAndExit. This aligns with the PR objective of migrating to Charmbracelet Logger.
308-308: LGTM! File permission modes updated correctly.The file permission modes have been updated from
0644to0o644for better readability and explicit octal notation. This is a good practice in Go.Also applies to: 313-313, 318-318
internal/exec/describe_affected_utils.go (4)
75-75: LGTM! Clear and informative logging.The logging statements provide good context about the cloning process and repository state.
Also applies to: 86-86, 88-88
128-128: LGTM! Good operational visibility.The logging statements effectively track the progress of Git operations, making it easier to debug issues.
Also applies to: 130-130, 135-135, 154-154
209-209: LGTM! Well-structured checkout progress logging.The logging statements provide clear visibility into the checkout process, making it easy to track the flow of operations.
Also applies to: 237-237, 254-254, 273-273, 280-280, 305-305
1515-1517: LGTM! Clear component status logging.The logging statement provides useful debug information about disabled components.
tests/test-cases/log-level-validation.yaml (1)
73-73: LGTM! Enhanced logging visibility with Charmbracelet Logger.The added debug message expectation aligns well with the migration to Charmbracelet Logger, providing better visibility into environment variable detection.
Let's verify the debug message format matches Charmbracelet Logger's standard format:
✅ Verification successful
Format verified! Your debug message follows Charmbracelet Logger's standard format.
The message format
DEBU Found ENV var ATMOS_LOGS_LEVEL=Debugperfectly matches the logger's conventions, providing clear visibility into environment variable detection.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the debug message format matches Charmbracelet Logger's standard format # Expected format: Level in caps (DEBU) followed by the message # Check the Charmbracelet Logger repository for format examples gh repo clone charmbracelet/log /tmp/charm-log cd /tmp/charm-log rg -A 2 'DEBU' .Length of output: 1625
pkg/utils/log_utils.go (3)
38-58: LGTM! Good error handling implementation.The error logging functions are well implemented with:
- Proper error logging using charmbracelet logger
- Exit code handling from exec.ExitError
- Stack trace printing at debug level
61-63: LGTM! Intentional debug level redirection.The redirection to debug level maintains backward compatibility since charmbracelet logger doesn't support trace level.
66-78: LGTM! Clean implementation of logging functions.The logging functions are correctly mapped to their corresponding charmbracelet logger levels.
cmd/root.go (2)
134-136: LGTM! Proper logger initialization.The logger is correctly initialized before processing commands.
128-131: LGTM! Consistent error handling.Error handling is consistently updated to use charmbracelet logger across all code paths.
Also applies to: 142-148
what
Migrate logging functionality to charmbracelet logger
why
To provide a more standardized logging format and the ability to use lipgloss to style logs in the future.
Summary by CodeRabbit
Based on the comprehensive summary of changes, here are the updated release notes:
Logging Improvements
Error Handling
Code Cleanup
The changes primarily focus on improving the logging and error handling mechanisms throughout the Atmos project, making the code more maintainable and easier to read.