Skip to content

Feature/issue 419 config cmd#1127

Open
omarbassem128 wants to merge 12 commits intokitops-ml:mainfrom
omarbassem128:feature/issue-419-config-cmd
Open

Feature/issue 419 config cmd#1127
omarbassem128 wants to merge 12 commits intokitops-ml:mainfrom
omarbassem128:feature/issue-419-config-cmd

Conversation

@omarbassem128
Copy link

Description

This PR introduces the new config command and natively integrates a config.yaml file into the root CLI options, allowing users to persist global settings without needing to pass flags on every run.

Added Subcommands:

  • config list: Outputs the entire active configuration state in a formatted JSON structure. Evaluates the complete precedence hierarchy to show the exact settings the CLI is using.
  • config set <key> <value>: Writes a new configuration value directly to the config.yaml file to persist global settings across terminal sessions.
  • config get <key>: Retrieves and prints the value of a single, specific configuration setting.
  • config reset: Deletes the config.yaml file, completely clearing any persisted user settings and instantly reverting the CLI back to its default values.

Key Implementations:

  • Global State Management: Integrated the config file reader into the PersistentPreRunE hook so that log-level, progress, and verbosity are properly initialized for all subcommands.
  • Strict Precedence Hierarchy: Built a robust override system to respect standard terminal behavior:
    • Storage Path (configHome): CLI Flag (--config) > Environment Variable ($KITOPS_HOME) > Config File > Defaults.
    • Settings (log-level, etc.): CLI Flags > Config File > Defaults.
  • Load Sequence Architecture: Refactored the boot sequence to ensure the CLI dynamically resolves the correct target storage directory before attempting to load the YAML file.
  • Graceful Degradation & Zero-Value Handling: Handled empty configuration files safely by falling back to Cobra's default options.
    • Replaced fatal crashes with non-fatal warnings when the config file contains invalid strings, allowing the application to safely apply default values and continue running.

Linked issues

AI-Assisted Code

  • This PR contains AI-generated code that I have reviewed and tested
  • I take full responsibility for all code in this PR, regardless of how it was created

- Added 'set' subcommand to the config command suite.
- Implemented dynamic path resolution using constants.DefaultConfigPath to support Linux, macOS, and Windows.
- Added logic to handle missing configuration files by creating them with 0644 permissions.
- Integrated yaml.v3 for marshaling and unmarshaling config data.
- Implemented project-specific error handling using output.Fatalf to comply with maintainer guidelines.
- Isolated os.ErrNotExist from fatal I/O errors to prevent silent data loss.

Issue: kitops-ml#419"

Signed-off-by: Omar Bassem <omarbassem889@gmail.com>
- Extracted core file I/O logic into 'setConfig' function in config.go to separate CLI wiring from business logic.
- Resturcutred error-handling worlflow to remove duplicate code, adhering to DRY principle.

Issue: kitops-ml#419

Signed-off-by: Omar Bassem <omarbassem889@gmail.com>
Implement the `config get <key>` command to allow users to retrieve and print specific configuration values directly from the CLI.

Refactor the underlying configuration logic by extracting YAML parsing and file reading into a shared helper function.
This keeps the file I/O DRY and ensures that missing or unreadable file errors are handled safely and consistently across both the `set` and `get` operations.

Issue: kitops-ml#419

Signed-off-by: Omar Bassem <omarbassem889@gmail.com>
Returning a closure from a function is unnecessary in this case, as there are no dependencies being injected to the wrapper function.

Make functions private for now as they aren't being called outside the package.

Issue: kitops-ml#419

Signed-off-by: Omar Bassem <omarbassem889@gmail.com>
- CLI output is explicitly sorted alphabetically to guarantee stable output and prevent Go's randomized map iteration from confusing the user.
- Change loadConfigFile to be more idiomatic by returning a map rather than accepting a map as an argument.
- Safely handle uninitialized environments by exiting quietly if config.yaml does not exist.

Issue: kitops-ml#419

Signed-off-by: Omar Bassem <omarbassem889@gmail.com>
- Wipe all key-value pairs in the config.yaml file by overwriting it
with an empty map, rather than deleting the file itself.

- Add an interactive warning prompt before execution to ensure the user
explicitly approves the destructive action.

- Create pathHelper() to adhere to the DRY principle by returning both
the Default and ConfigYaml paths.

Issue: kitops-ml#419

Signed-off-by: Omar Bassem <omarbassem889@gmail.com>
Follow idiomatic Go practices by consolidating errors to their scope
and removing unnecessary prefixes

Issue: kitops-ml#419

Signed-off-by: Omar Bassem <omarbassem889@gmail.com>
- Use a struct to hold the global flags statically to provide type
  safety and reject invalid keys during configuration updates.
- Format key-value pairs using JSON pretty-printing for improved
  terminal readability.

Issue: kitops-ml#419

Signed-off-by: Omar Bassem <omarbassem889@gmail.com>
- Make the list command recognize when the config file does not exist and print an appropriate error message.
- Make the set command ignore the absence of the config file and create a new file to store the key-value pair.

Issue: kitops-ml#419

Signed-off-by: Omar Bassem <omarbassem889@gmail.com>
- Establish path precedence for configHome: CLI Flag > KITOPS_HOME Env Var > Config File > Defaults.
- Establish setting precedence for log-level, progress, and verbosity: CLI Flags > Config File > Defaults.
- Resolve config file load sequence to ensure the target directory is determined before reading the YAML file.
- Implement graceful error handling to safely fall back to default values if the config file contains invalid strings.

Issue: kitops-ml#419

Signed-off-by: Omar Bassem <omarbassem889@gmail.com>
@omarbassem128
Copy link
Author

@amisevsk Appreciate the feedback.

Comment on lines +136 to +144
func PathHelper() (string, error) {
path, err := constants.DefaultConfigPath()
if err != nil {
return "", fmt.Errorf("failed to get default path: %w", err)
}
configYamlPath := constants.ConfigYamlPath(path)

return configYamlPath, nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

How do you handle cases where a non-default config path is being used? I can override the default config path by setting either $KITOPS_HOME or using the --config flag.

Copy link
Author

Choose a reason for hiding this comment

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

Yes this should be resolved by using the context set in the root.go file. Currently working on it.

Copy link
Author

Choose a reason for hiding this comment

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

I found some bugs that I am trying to fix while passing the context. Which is why it is taking some time.

Copy link
Author

Choose a reason for hiding this comment

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

I solved it and other bugs in this new commit

Copy link
Author

@omarbassem128 omarbassem128 Mar 22, 2026

Choose a reason for hiding this comment

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

Having configHome in the config.yaml led to a very confusing chicken-and-egg situation which is why I removed it completely. What do you think ? Now if the user wants a custom path to their config file, they should use the --config flag with every command, whether it is set, list, get or reset.

cmd/root.go Outdated
Comment on lines +90 to +91
output.SetOut(cmd.OutOrStdout())
output.SetErr(cmd.ErrOrStderr())
Copy link
Contributor

Choose a reason for hiding this comment

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

You have already done some logging; these should be set before we do any output

Copy link
Author

Choose a reason for hiding this comment

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

Moved them to the top

…te errors

-Remove `configHome` from the `Config` struct and related switch statements to eliminate a circular dependency.
The CLI now strictly determines the configuration path via ephemeral flags, persistent environment variables, or hardcoded defaults before attempting file I/O.
-Strip fallback logic from `root.go` (`PersistentPreRunE`) that incorrectly attempted to parse the storage directory from the file itself.
-Update `saveConfigFile` to utilize `os.MkdirAll`, ensuring parent directories are safely generated before writing `config.yaml`.
 This prevents fatal "no such file or directory" panics on fresh installations.

Issue: kitops-ml#419

Signed-off-by: Omar Bassem <omarbassem889@gmail.com>
- Move `output.SetOut` and `output.SetErr` to the very beginning of the `PersistentPreRunE` function.
- This guarantees that Cobra's stream management is set up before any file system operations occur,
ensuring that early initialization errors (like failing to locate the config directory) are correctly routed to the custom logger and can be reliably captured by automated tests.

Issue: kitops-ml#419

Signed-off-by: Omar Bassem <omarbassem889@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow CLI configuration to be saved to a file

2 participants