Enhance Atmos CLI: Add Support for Custom Base Path and Config Paths #1091
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/validate_editorconfig.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: golangci-lint
cmd/validate_editorconfig.go
[failure] 56-56:
SA1019: u.LogErrorAndExit is deprecated: Use log.Fatal instead. This function will be removed in a future release. LogErrorAndExit logs errors to std.Error and exits with an error code.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build (macos-latest, macos)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (2)
cmd/validate_editorconfig.go (2)
53-59: Added proper flag handling for--config.This code correctly retrieves the
--configflag values when changed and assigns them to theconfigFilePathsvariable, which addresses the previous review comment from samtholiya. This is a key improvement that aligns with the PR's goal of supporting custom config paths.🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 56-56:
SA1019: u.LogErrorAndExit is deprecated: Uselog.Fatalinstead. This function will be removed in a future release. LogErrorAndExit logs errors to std.Error and exits with an error code.
188-203:❓ Verification inconclusive
Verify
--configflag is properly defined globally.I notice that the
--configflag isn't defined inaddPersistentFlags, which suggests it's now defined at a more global level (likely incmd/root.go). This aligns with the PR's goal of adding global CLI flags.Run this script to confirm the flag is defined globally:
🏁 Script executed:
#!/bin/bash # Check if the --config flag is defined in root.go echo "Checking if --config flag is defined in root.go:" grep -n "config.*StringSlice" cmd/root.goLength of output: 150
Global '--config' flag: Manual Verification Required
Our initial automated check (searching for a
StringSlicedefinition incmd/root.go) didn’t yield any results for the--configflag. This may mean the flag is defined elsewhere or using a different type (e.g. withStringVar). Please double-check manually that the flag is properly defined at the global level as intended by the PR’s goal.
- Verify in
cmd/root.go(or any other global flag initialization file) that the--configflag is defined.- Confirm that its definition uses the appropriate flag type (e.g.
StringVarvs.StringSlice).
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
cmd/validate_editorconfig.go (1)
72-72: 🛠️ Refactor suggestionReplace deprecated u.LogErrorAndExit calls
Multiple instances of the deprecated
u.LogErrorAndExitfunction remain in this file. According to the static analysis, this function is deprecated and will be removed in a future release.Replace all instances with the recommended
log.Fatalor the more graceful error handling approach:-u.LogErrorAndExit(err) +log.Error("Operation failed", "error", err) +os.Exit(1)Also applies to: 104-104, 110-110, 151-151, 156-156
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/validate_editorconfig.go(2 hunks)
🧰 Additional context used
🪛 GitHub Check: golangci-lint
cmd/validate_editorconfig.go
[failure] 57-57:
deep-exit: calls to log.Fatal only in main() or init() functions
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: website-deploy-preview
- GitHub Check: Summary
🔇 Additional comments (1)
cmd/validate_editorconfig.go (1)
8-8: Good update to charmbracelet/log libraryThis import change aligns with modern logging practices in the codebase.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
cmd/validate_editorconfig.go (2)
73-73: 🛠️ Refactor suggestionReplace deprecated function with recommended alternative.
The
u.LogErrorAndExitfunction is deprecated according to previous static analysis warnings.- u.LogErrorAndExit(err) + log.Error("Failed to save config", "error", err) + os.Exit(1)Note that this still has the issue of calling
os.Exitoutside of main/init. Consider refactoring the overall error handling approach across the file.
105-106: 💡 Verification agent🧩 Analysis chain
Update all instances of deprecated error handling.
There are multiple instances of
u.LogErrorAndExitthat should be updated consistently with the same error handling pattern you choose.Also applies to: 111-112, 152-153, 157-158
🏁 Script executed:
#!/bin/bash # Find all instances of u.LogErrorAndExit in this file echo "Instances of deprecated u.LogErrorAndExit in validate_editorconfig.go:" grep -n "u.LogErrorAndExit" cmd/validate_editorconfig.go # Check if any custom error handling helpers exist in the codebase echo -e "\nLooking for potential error handling helpers:" grep -r "func LogError" --include="*.go" pkg/Length of output: 906
Consistent error handling update required
Please replace all instances ofu.LogErrorAndExitincmd/validate_editorconfig.gowith a unified error handling approach (for example, usingu.LogErroralong with an explicit exit if needed). This update applies to the calls on lines 73, 105, 111, 152, and 157. Make sure the new pattern is used across the codebase wherever applicable.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/validate_editorconfig.go(2 hunks)
🧰 Additional context used
🪛 golangci-lint (1.64.8)
cmd/validate_editorconfig.go
[error] 8-8: import "github.com/charmbracelet/log" imported without alias but must be with alias "log" according to config
(importas)
🪛 GitHub Check: golangci-lint
cmd/validate_editorconfig.go
[failure] 8-8:
import "github.com/charmbracelet/log" imported without alias but must be with alias "log" according to config
[failure] 58-58:
deep-exit: calls to os.Exit only in main() or init() functions
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (1)
cmd/validate_editorconfig.go (1)
54-61: Good implementation of config flag handling.The code correctly checks for the
--configflag and retrieves values as a string slice, properly addressing the previous review comment about getting flag values to configFilePaths.🧰 Tools
🪛 GitHub Check: golangci-lint
[failure] 58-58:
deep-exit: calls to os.Exit only in main() or init() functions
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/validate_editorconfig.go (1)
53-58: Good solution for flag value handling.You've addressed the previous feedback by correctly retrieving the
--configflag value and populating theconfigFilePathsvariable. The code checks for flag presence and safely handles the lookup.Consider using the more direct
GetStringSlicemethod instead of manual string splitting:if cmd.Flags().Changed("config") { - config := cmd.Flags().Lookup("config") - if config != nil { - configFilePaths = strings.Split(config.Value.String(), ",") - } + configFiles, err := cmd.Flags().GetStringSlice("config") + if err != nil { + u.LogErrorMsg(fmt.Sprintf("Failed to get config flag values: %v", err)) + return + } + configFilePaths = configFiles }This provides better error handling and ensures correct parsing based on the flag's defined type.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/validate_editorconfig.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
cmd/validate_editorconfig.go (2)
Learnt from: samtholiya
PR: cloudposse/atmos#1077
File: pkg/downloader/custom_github_detector.go:0-0
Timestamp: 2025-04-03T12:14:28.671Z
Learning: The `github.com/charmbracelet/log` package should be imported with the alias `log` according to the project's import alias configuration.
Learnt from: samtholiya
PR: cloudposse/atmos#1077
File: pkg/downloader/custom_github_detector_test.go:0-0
Timestamp: 2025-04-03T12:14:38.761Z
Learning: The `github.com/charmbracelet/log` package should be imported with the alias `log`, not `clog`.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
|
These changes were released in v1.171.0. |
what
why
references
Summary by CodeRabbit
Summary by CodeRabbit
New Features
--base-path,--config, and--config-path) to allow users to specify a custom project base path and define multiple configuration files and directories.Bug Fixes
--configflag from the command-line interface, streamlining configuration management.Documentation