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

SkySingh04 · 2024-10-09T22:00:13Z

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

Signed-off-by: Akash Singh <akashsingh2210670@gmail.com>

SkySingh04 · 2024-10-09T22:01:37Z

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

Signed-off-by: Akash Singh <akashsingh2210670@gmail.com>

amisevsk

Thanks for the contribution! I have a few initial comments

cmd/root.go

pkg/cmd/config/cmd.go

pkg/cmd/config/config.go

amisevsk · 2024-10-10T18:08:43Z

pkg/cmd/config/config.go

+// DefaultConfig returns a Config struct with default values.
+func DefaultConfig() *Config {
+	return &Config{
+		LogLevel:  "info",


This should use the constants from the output package

Could you confirm if we should also do this for Progress and ConfigDir as well? If so, we will need to change the var names in output package for Progress to Capitalise and export

Signed-off-by: Akash Singh <akashsingh2210670@gmail.com>

SkySingh04 · 2024-10-11T07:23:59Z

@amisevsk Your requested changes have been made :D

Signed-off-by: Akash <akashsingh2210670@gmail.com>

cmd/root.go

pkg/cmd/config/config.go

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 · 2024-10-17T00:42:50Z

@amisevsk Your requested changes have been made!

gorkem · 2024-10-17T13:48:07Z

Can you add the license header for config.go?

SkySingh04 · 2024-10-17T18:30:49Z

@gorkem license header has been added!

SkySingh04 · 2024-10-21T14:45:51Z

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

gorkem

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.

gorkem · 2024-10-21T17:05:41Z

pkg/cmd/config/cmd.go

+	if len(args) == 0 {
+		return fmt.Errorf("no configuration key provided")
+	}
+	opts.key = args[0]


The way that config command is implemented is set/get/list/reset are not subcommands but rather args. Therefore the args[0] in here receives the value of the subcommand (get/set/list/reset). This works for no argument subcommands (list/reset) but everything else does not work.

I think a better way to do this would be to add the (set/get/list/reset) as subcommands.

Refactored to include set/get/list/reset as subcommands.

gorkem · 2024-10-21T17:28:27Z

pkg/cmd/config/config.go

+		cfg = DefaultConfig()
+	}
+
+	v := reflect.ValueOf(cfg).Elem().FieldByName(strings.Title(opts.key))


Replace strings.Title with cases.Title from the golang.org/x/text/cases package.

Refactored!

pkg/cmd/config/config.go

gorkem · 2024-10-21T17:40:43Z

pkg/cmd/config/cmd.go

+	}
+
+	cmd.Args = cobra.MinimumNArgs(1)
+	cmd.Flags().SortFlags = false


Not needed if there are no flags.

Co-authored-by: Gorkem Ercan <gorkem.ercan@gmail.com>

… configCmd

SkySingh04 · 2024-10-24T11:52:31Z

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

gorkem

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

gorkem · 2024-10-24T19:51:09Z

pkg/cmd/config/config.go

+		return "", err
+	}
+
+	v := reflect.ValueOf(cfg).Elem().FieldByName(strings.Title(opts.key))


Can you also update this to use cases.Title?

amisevsk · 2024-11-01T02:18:04Z

pkg/cmd/config/config.go

+	caser := cases.Title(language.Und)
+	v := reflect.ValueOf(cfg).Elem().FieldByName(caser.String(opts.key))
+	if !v.IsValid() {
+		return fmt.Errorf("unknown configuration key: %s", opts.key)
+	}


Caser is causing issues here; since we're using camelcase for config settings (LogLevel), attempting to modify case will mangle fields, turning LogLevel, logLevel, etc. into Loglevel

This has been fixed!

amisevsk · 2024-11-01T02:31:47Z

pkg/cmd/config/cmd.go

+				return fmt.Errorf("failed to complete options: %w", err)
+			}
+			if err := setConfig(ctx, opts); err != nil {
+				return fmt.Errorf("failed to set config: %w", err)


A quirk of how we've implemented error handling in commands means that returned errors here are gobbled up (we have to do this to avoid Cobra's default error printing, prints the usage doc every time -- see here)

This means the caser issue I mentioned is getting silently eaten, and the CLI is just exiting with status code 1. Instead, you want to replace the returns from RunE functions with output.Fatalf or output.Fatalln (using %s for errors instead of %w), both here and in the other subcommands.

With the change:

❯ kit config set LogLevel debug [ERROR] Failed to set config: unknown configuration key: LogLevel

That is a super interesting implementation! I have refactored and fixed this!

…onsistent logging

…for consistency

SkySingh04 · 2024-11-01T16:47:56Z

@amisevsk Your requested changes have been made!

SkySingh04 added 2 commits October 10, 2024 02:45

Added config command to cli

f447f86

Signed-off-by: Akash Singh <akashsingh2210670@gmail.com>

Added overriding of flags from config for root command

7c69dc7

Signed-off-by: Akash Singh <akashsingh2210670@gmail.com>

SkySingh04 changed the title ~~Config cmd~~ [feat:] Added Config Command to Kitops CLI Oct 9, 2024

fixes jozu-ai#419

840248c

Signed-off-by: Akash Singh <akashsingh2210670@gmail.com>

amisevsk reviewed Oct 10, 2024

View reviewed changes

refactoring and resolving comments

5455886

Signed-off-by: Akash Singh <akashsingh2210670@gmail.com>

Jwilliamsr added the hacktoberfest label Oct 11, 2024

fixed typo ProgressStyle -> progressStyle

9fcfda9

Signed-off-by: Akash <akashsingh2210670@gmail.com>

SkySingh04 requested a review from amisevsk October 13, 2024 19:42

amisevsk reviewed Oct 15, 2024

View reviewed changes

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

SkySingh04 requested a review from amisevsk October 17, 2024 00:42

Refactor config.go and add license header

c3d308f

gorkem requested changes Oct 21, 2024

View reviewed changes

SkySingh04 and others added 3 commits October 24, 2024 17:15

Update pkg/cmd/config/config.go

edb0ba1

Co-authored-by: Gorkem Ercan <gorkem.ercan@gmail.com>

Refactor config.go and add golang.org/x/text v0.19.0 dependency

1a3b378

Merge branch 'configCmd' of https://github.com/SkySingh04/kitops into…

9e45526

… configCmd

SkySingh04 requested a review from gorkem October 24, 2024 11:52

Refactor config command and add example usage

305ec26

gorkem requested changes Oct 24, 2024

View reviewed changes

Refactor config.go and remove unused import

e694998

amisevsk reviewed Nov 1, 2024

View reviewed changes

Refactor error handling in config commands to use output.Fatalf for c…

62069b3

…onsistent logging

Refactor config command to improve error handling with output.Fatalf …

bfb0ada

…for consistency

SkySingh04 requested review from amisevsk and gorkem November 4, 2024 12:02

amisevsk mentioned this pull request Nov 5, 2024

Brew issues #563

Closed

This was linked to issues Nov 5, 2024

Brew issues #563

Closed

Allow CLI configuration to be saved to a file #419

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

SkySingh04 commented Oct 9, 2024

SkySingh04 commented Oct 9, 2024

amisevsk left a comment

amisevsk Oct 10, 2024

SkySingh04 Oct 11, 2024

SkySingh04 commented Oct 11, 2024

SkySingh04 commented Oct 17, 2024

gorkem commented Oct 17, 2024

SkySingh04 commented Oct 17, 2024

SkySingh04 commented Oct 21, 2024

gorkem left a comment

gorkem Oct 21, 2024

SkySingh04 Oct 24, 2024

gorkem Oct 21, 2024

SkySingh04 Oct 24, 2024

gorkem Oct 21, 2024

SkySingh04 Oct 24, 2024

SkySingh04 commented Oct 24, 2024

gorkem left a comment

gorkem Oct 24, 2024

SkySingh04 Oct 25, 2024

amisevsk Nov 1, 2024

SkySingh04 Nov 1, 2024

amisevsk Nov 1, 2024

SkySingh04 Nov 1, 2024

SkySingh04 commented Nov 1, 2024

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

Are you sure you want to change the base?

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

Conversation

SkySingh04 commented Oct 9, 2024

Description

Linked Issues

SkySingh04 commented Oct 9, 2024

amisevsk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SkySingh04 commented Oct 11, 2024

SkySingh04 commented Oct 17, 2024

gorkem commented Oct 17, 2024

SkySingh04 commented Oct 17, 2024

SkySingh04 commented Oct 21, 2024

gorkem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SkySingh04 commented Oct 24, 2024

gorkem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SkySingh04 commented Nov 1, 2024