Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[feat:] Added Config Command to Kitops CLI #523

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

SkySingh04
Copy link

Description

This PR introduces the new config command for the KitOps CLI. It allows users to manage the configuration of CLI settings more easily. With this command, users can set, get, list, and reset configuration values for persistent options like storage paths, credentials, logging levels, and other preferences.

Key additions:

  • Config command: Introduced the config command with four subcommands:

    • set: Allows users to set configuration values.
    • get: Retrieves the value of a specified configuration key.
    • list: Lists all current configuration options and their values.
    • reset: Resets all configurations back to default values.
  • Persistent root flags: The root command now uses configuration settings from the config.json file for default values like --log-level, --progress, and --verbose, while still allowing overrides via command-line arguments.

Note: This PR supports Config for only root command flags, as it is not clearly mentioned if it should be implemented for all flags.

Linked Issues

Closes issue #419

@SkySingh04 SkySingh04 changed the title Config cmd [feat:] Added Config Command to Kitops CLI Oct 9, 2024
@SkySingh04
Copy link
Author

@amisevsk Could you please review this PR and suggest upon how we can improve it?

Signed-off-by: Akash Singh <[email protected]>
Copy link
Contributor

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! I have a few initial comments

cmd/root.go Outdated Show resolved Hide resolved
cmd/root.go Outdated Show resolved Hide resolved
cmd/root.go Show resolved Hide resolved
pkg/cmd/config/cmd.go Outdated Show resolved Hide resolved
pkg/cmd/config/cmd.go Outdated Show resolved Hide resolved
pkg/cmd/config/config.go Outdated Show resolved Hide resolved
pkg/cmd/config/config.go Outdated Show resolved Hide resolved
@SkySingh04
Copy link
Author

@amisevsk Your requested changes have been made :D

@SkySingh04 SkySingh04 requested a review from amisevsk October 13, 2024 19:42
cmd/root.go Outdated Show resolved Hide resolved
cmd/root.go Outdated Show resolved Hide resolved
pkg/cmd/config/config.go Outdated Show resolved Hide resolved
pkg/cmd/config/config.go Outdated Show resolved Hide resolved
pkg/cmd/config/config.go Outdated Show resolved Hide resolved
This commit refactors the code related to loading and saving the configuration file. It introduces a new function `ConfigFilePath` in the `consts.go` file to generate the path for the configuration file based on the `configHome` directory. The `LoadConfig` function now returns the default configuration if the file doesn't exist, and the `SaveConfig` function creates the directory before saving the file. These changes improve the reliability and maintainability of the configuration handling.

Fixes jozu-ai#419
@SkySingh04 SkySingh04 requested a review from amisevsk October 17, 2024 00:42
@SkySingh04
Copy link
Author

@amisevsk Your requested changes have been made!

@gorkem
Copy link
Contributor

gorkem commented Oct 17, 2024

Can you add the license header for config.go?

@SkySingh04
Copy link
Author

@gorkem license header has been added!

@SkySingh04
Copy link
Author

@amisevsk @gorkem Waiting for a response on this so that we can proceed further!

Copy link
Contributor

@gorkem gorkem left a comment

Choose a reason for hiding this comment

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

The code that manually parses subcommands and arguments has logic errors that needs to be fixed. The code manually inspects args[0] to determine the subcommand, which is not the standard practice. It is better to Define each subcommand (set, get, list, reset) as separate Cobra commands.

pkg/cmd/config/cmd.go Outdated Show resolved Hide resolved
pkg/cmd/config/config.go Outdated Show resolved Hide resolved
pkg/cmd/config/config.go Outdated Show resolved Hide resolved
pkg/cmd/config/cmd.go Outdated Show resolved Hide resolved
@SkySingh04
Copy link
Author

@gorkem Your requested changes have been made, could you kindly review?

@SkySingh04 SkySingh04 requested a review from gorkem October 24, 2024 11:52
Copy link
Contributor

@gorkem gorkem left a comment

Choose a reason for hiding this comment

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

Much better but setting the logLevel did not work for me. Using reflection to find keys maybe too fragile.

pkg/cmd/config/config.go Outdated Show resolved Hide resolved
pkg/cmd/config/config.go Outdated Show resolved Hide resolved
pkg/cmd/config/cmd.go Outdated Show resolved Hide resolved
@SkySingh04
Copy link
Author

@amisevsk Your requested changes have been made!

@SkySingh04 SkySingh04 requested review from amisevsk and gorkem November 4, 2024 12:02
@amisevsk amisevsk mentioned this pull request Nov 5, 2024
This was linked to issues Nov 5, 2024
@gorkem gorkem marked this pull request as draft December 17, 2024 06:31
@SkySingh04
Copy link
Author

@gorkem @amisevsk Any reason for this PR to be marked as draft? Any changes that are still pending?

@gorkem gorkem marked this pull request as ready for review December 18, 2024 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow CLI configuration to be saved to a file Brew issues
4 participants