Remove unused flag code in root #1103
Conversation
|
We added this in an emergency patch release. I don’t believe the underlying issue is resolved. |
So these lines were updated because there was a code that actually assigned these values. You are right to say the Patchy work is not getting refactored and that is not the intent of this pr. The intent of this pr is to clear this unwanted condition. Just cleaning things :) |
samtholiya
force-pushed
the
feature/dev-3074-remove-unused-flags-value-code
branch
from
252686f to
08f1a58
Compare
samtholiya
force-pushed
the
feature/dev-3074-remove-unused-flags-value-code
branch
from
08f1a58 to
3e091f9
Compare
...estCLICommands_Valid_log_file_in_flag_should_be_priortized_over_env_and_config.stdout.golden
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
...pshots/TestCLICommands_Valid_log_level_in_env_should_be_priortized_over_config.stderr.golden
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1103 +/- ##
==========================================
+ Coverage 21.29% 21.64% +0.35%
==========================================
Files 172 172
Lines 19309 19292 -17
==========================================
+ Hits 4112 4176 +64
+ Misses 14425 14336 -89
- Partials 772 780 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
We need to distinguish between log output, which is for developers and console output, which is for users. Previously, the code was not clear about this and muddled it by using log levels to control console output. Hence it makes sense why you are condensing it into log statements, but the original logic was flawed.
Please introduce a function so we make it clear and deliberate when we are showing console output as opposed to log output .
For example, in the future, we might introduce a quiet flag, which affects the console output, but does not affect the log output.
…ps://github.com/cloudposse/atmos into feature/dev-3074-remove-unused-flags-value-code
samtholiya
force-pushed
the
feature/dev-3074-remove-unused-flags-value-code
branch
from
4751348 to
69fc1b4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/config/config.go (3)
395-410: The log configuration logic looks correct but could benefit from refactoring.The function appropriately checks environment variables and command-line flags for logging configuration. The priority order (flags override environment variables) is logical.
However, the TODO comment indicates this is a quick patch. Consider revisiting this in a future PR to integrate with Cobra's built-in flag handling system.
418-418: Add period at end of comment.The static code analyzer correctly flagged this missing period at the end of the comment.
-// Function to manually parse flags with double dash "--" like Cobra +// Function to manually parse flags with double dash "--" like Cobra.
419-447: Flag parsing implementation could be simplified.The flag parsing logic works correctly but could be simplified as suggested by static analysis tools.
Consider these improvements:
- Simplify the if-else chain using early returns
- Use a switch statement for better readability
- Return a potential error for better error handling
func parseFlags() map[string]string { args := os.Args flags := make(map[string]string) for i := 0; i < len(args); i++ { arg := args[i] // Check if the argument starts with '--' (double dash) if strings.HasPrefix(arg, "--") { // Strip the '--' prefix and check if it's followed by a value arg = arg[2:] - if strings.Contains(arg, "=") { - // Case like --flag=value - parts := strings.SplitN(arg, "=", 2) - flags[parts[0]] = parts[1] - } else if i+1 < len(args) && !strings.HasPrefix(args[i+1], "--") { - // Case like --flag value - flags[arg] = args[i+1] - i++ // Skip the next argument as it's the value - } else { - // Case where flag has no value, e.g., --flag (we set it to "true") - flags[arg] = "true" - } - } else { - // It's a regular argument, not a flag, so we skip - continue + + switch { + case strings.Contains(arg, "="): + // Case like --flag=value + parts := strings.SplitN(arg, "=", 2) + flags[parts[0]] = parts[1] + case i+1 < len(args) && !strings.HasPrefix(args[i+1], "--"): + // Case like --flag value + flags[arg] = args[i+1] + i++ // Skip the next argument as it's the value + default: + // Case where flag has no value, e.g., --flag (we set it to "true") + flags[arg] = "true" + } + continue } + + // It's a regular argument, not a flag, so we skip } return flags }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/root.go(2 hunks)pkg/config/config.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/root.go
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: Summary
🔇 Additional comments (4)
pkg/config/config.go (4)
10-10: Appropriate import addition.New import aligns with the strings manipulation needs in
parseFlagsfunction.
389-389: Good integration of log configuration.Calling
setLogConfigat the end ofInitCliConfigensures log settings are applied after all other configuration steps, centralizing the logging configuration.
412-417: Valid concern about manual flag parsing.The TODO correctly acknowledges that manual flag parsing is not ideal when using Cobra. This approach works for now, but should be revisited in a future refactoring effort.
419-447:✅ Verification successful
Verify this function handles all use cases in the codebase.
The
parseFlagsfunction assumes all flags start with--, but doesn't handle short-form flags (-f). Ensure this matches all flag usage patterns in the codebase.
🏁 Script executed:
#!/bin/bash # Description: Search for flag pattern usage in the codebase to verify compatibility # Search for short flags (-f) in the codebase echo "Searching for short flags that might not be handled by parseFlags..." rg "flag\.(StringP|BoolP|IntP)" --type go # Check if Cobra is being used for command-line parsing echo "Checking for Cobra command-line parsing..." rg "cobra\.(Command|OnInitialize|AddCommand)" --type go # Check how flags are defined in the codebase echo "Checking how flags are registered in the codebase..." rg "flag\.(String|Bool|Int)" --type go # Specifically check for logs-level and logs-file flags echo "Checking for logs-level and logs-file flags..." rg "(logs-level|logs-file)" --type goLength of output: 15536
Flag Parsing Approach Verified – Only Long Flags in Use
After a thorough search of our codebase:
- There are no instances of short-form flag usage (like
-f) detected.- Flags registered using Go’s
flagpackage consistently use long names (e.g.,"--logs-level"), matching the expectations of theparseFlagsfunction.- Cobra is employed extensively for command parsing elsewhere, and it manages both long and short flags; however,
parseFlagsappears to be designed only for contexts where long flags are in use.Recommendation:
Add a brief comment to theparseFlagsfunction explaining that it is intended solely for long flag formats (e.g.,--flag=valueor--flag value), clarifying its limitations for any future developers.
samtholiya
force-pushed
the
feature/dev-3074-remove-unused-flags-value-code
branch
2 times, most recently
from
9bc24a5 to
27fa69e
Compare
samtholiya
force-pushed
the
feature/dev-3074-remove-unused-flags-value-code
branch
from
27fa69e to
2755e69
Compare
Fixed that. Happy to have this new test case in the mainline that helped a lot. |
tests/snapshots/TestCLICommands_Invalid_Log_Level_in_Environment_Variable.stderr.golden
Show resolved
Hide resolved
...estCLICommands_Valid_log_file_in_flag_should_be_priortized_over_env_and_config.stdout.golden
Show resolved
Hide resolved
|
💥 This pull request now has conflicts. Could you fix it @samtholiya? 🙏 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/config/config.go (2)
396-411: Temporary logging configuration solution needs better documentation.The function properly centralizes log configuration from both environment variables and command line flags, which is architecturally sound. However, the TODO comment doesn't clearly explain what issue this is patching.
-// TODO: This is a quick patch to mitigate the issue we can look for better code later +// TODO: This is a quick patch to centralize log configuration handling that was previously +// scattered across the codebase. In the future, we should refactor this to use a more +// structured approach that integrates better with Cobra's flag handling.
419-446: Manual flag parsing implementation works but has limitations.This manual flag parsing implementation is functional for basic cases but could become a maintenance burden. It works well for the current use case of extracting logging flags, but be cautious about extending this approach.
Consider limiting the scope of this function to only parse specific logging-related flags rather than all double-dash arguments, which would reduce potential conflicts with Cobra:
-func parseFlags() map[string]string { +func parseLoggingFlags() map[string]string { args := os.Args flags := make(map[string]string) + loggingFlags := []string{"logs-level", "logs-file"} for i := 0; i < len(args); i++ { arg := args[i] // Check if the argument starts with '--' (double dash) if !strings.HasPrefix(arg, "--") { continue } // Strip the '--' prefix and check if it's followed by a value arg = arg[2:] + // Only process logging-related flags + isLoggingFlag := false + for _, flagName := range loggingFlags { + if arg == flagName || strings.HasPrefix(arg, flagName+"=") { + isLoggingFlag = true + break + } + } + if !isLoggingFlag { + continue + } switch {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/config/config.go(2 hunks)pkg/config/utils.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Lint (golangci)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (3)
pkg/config/utils.go (1)
361-361: Consistent logging format update.The change from
u.LogDebug(fmt.Sprintf(...))to structured logging withlog.Debug(..., ...)is a good improvement that aligns with modern logging practices.pkg/config/config.go (2)
390-390: Good centralization of log configuration.This call to
setLogConfigfinalizes the log configuration after all other configurations have been processed, ensuring a consistent approach to logging setup.
413-418: The TODOs accurately acknowledge the flag parsing concerns.The comments correctly identify that manual flag parsing isn't ideal compared to leveraging Cobra's built-in capabilities.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/config/config.go (3)
397-398: Consider improving the TODO comment clarity.The current TODO comment references an issue without explaining what the actual issue is. This makes it harder for future developers to understand the context without access to the issue tracking system.
- // TODO: This is a quick patch to mitigate the issue we can look for better code later - // Issue: https://linear.app/cloudposse/issue/DEV-3093/create-a-cli-command-core-library + // TODO: This is a quick patch to handle log configuration from both environment variables and CLI flags. + // A more comprehensive solution will be implemented as part of the CLI command core library. + // Issue: https://linear.app/cloudposse/issue/DEV-3093/create-a-cli-command-core-library
421-447: Consider adding error handling to the flag parsing function.The current implementation of
parseFlags()makes certain assumptions about the format of command-line arguments, but doesn't handle potential errors or edge cases such as duplicate flags.-func parseFlags() map[string]string { +func parseFlags() (map[string]string, error) { args := os.Args flags := make(map[string]string) for i := 0; i < len(args); i++ { arg := args[i] // Check if the argument starts with '--' (double dash) if !strings.HasPrefix(arg, "--") { continue } // Strip the '--' prefix and check if it's followed by a value arg = arg[2:] + // Check if flag name is empty + if len(arg) == 0 { + return nil, fmt.Errorf("empty flag name found at position %d", i) + } switch { case strings.Contains(arg, "="): // Case like --flag=value parts := strings.SplitN(arg, "=", 2) + if _, exists := flags[parts[0]]; exists { + return nil, fmt.Errorf("duplicate flag: %s", parts[0]) + } flags[parts[0]] = parts[1] case i+1 < len(args) && !strings.HasPrefix(args[i+1], "--"): // Case like --flag value + if _, exists := flags[arg]; exists { + return nil, fmt.Errorf("duplicate flag: %s", arg) + } flags[arg] = args[i+1] i++ // Skip the next argument as it's the value default: // Case where flag has no value, e.g., --flag (we set it to "true") + if _, exists := flags[arg]; exists { + return nil, fmt.Errorf("duplicate flag: %s", arg) + } flags[arg] = "true" } } - return flags + return flags, nil }This would require updating the calling code in
setLogConfig:- flagKeyValue := parseFlags() + flagKeyValue, err := parseFlags() + if err != nil { + u.LogWarning(fmt.Sprintf("error parsing flags: %v", err)) + flagKeyValue = make(map[string]string) + }
402-405: Consider consistent error handling for flag parsing results.Currently, the code silently accepts whatever value is provided for
logs-levelwithout validation. Consider adding validation to ensure the value is a valid log level.flagKeyValue := parseFlags() if v, ok := flagKeyValue["logs-level"]; ok { + // Validate log level before setting + if _, err := u.ParseLogLevel(v); err != nil { + u.LogWarning(fmt.Sprintf("invalid log level: %s", v)) + } else { atmosConfig.Logs.Level = v + } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/config/config.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Analyze (go)
- GitHub Check: Lint (golangci)
- GitHub Check: Summary
🔇 Additional comments (3)
pkg/config/config.go (3)
390-390: Good structuring by centralizing log config setting.The addition of this function call ensures that logging configuration happens at a consistent point in the initialization flow, after all other config sources have been processed.
414-419: Good use of TODO comments for technical debt tracking.The comments clearly document why manual flag parsing is being used and the intention to revisit this approach in the future.
396-412:✅ Verification successful
The log config priority logic is clear and straightforward.
The implementation properly handles environment variables and command-line flags for logging configuration, with flags taking priority over environment variables.
Let's verify if there are any calls elsewhere in the codebase that might be setting these same values:
🏁 Script executed:
#!/bin/bash # Search for other code that might be setting log levels or files rg -p "atmosConfig.Logs.(Level|File) ?=" --type goLength of output: 2769
Log configuration priority verified across codebase
The log configuration function in
pkg/config/config.gocorrectly applies environment variables followed by flag overrides. Our review of the repository shows that while several tests and utility files (such as those inpkg/workflow/workflow_test.go,pkg/vender/*, and various files underinternal/exec/) reference or assign log levels for validation purposes, none of them conflict with the priority logic established in this function.
- The tests and internal utilities typically compare against or set a default log level (e.g.,
u.LogLevelTrace), which verifies the expected behavior rather than interfering with the configuration ordering.- This consistent usage confirms that the current implementation is safe and effective.
what
why
references
Summary by CodeRabbit
New Features
Refactor
Tests
Chores