refactor(cli): Use Viper to manage Keptn config #5694

vadasambar · 2021-10-13T15:43:41Z

Fixes #5490

This PR uses Viper internally to manage the Keptn Config.

Why

Viper allows reading config fields from the environment variables
Viper allows aliasing fields
Changing file type of the config is relatively easier
Viper decouples the CLI from the internal representation of the Config

How to test

You can do keptn <any-command> --cli-config ~/.foo/my-keptn-config to use a config stored at path other than ~/.keptn/config

Things to note

Please check go.sum. Adding viper 1.9.0 lead to many changes in go.sum (additive. I have not removed any dependency)

codecov · 2021-11-03T15:25:29Z

Codecov Report

Merging #5694 (586d557) into master (d89cab9) will decrease coverage by 1.10%.
The diff coverage is 65.07%.

❗ Current head 586d557 differs from pull request most recent head 5c5a7be. Consider uploading reports for the commit 5c5a7be to get more accurate results

@@            Coverage Diff             @@
##           master    #5694      +/-   ##
==========================================
- Coverage   54.33%   53.23%   -1.11%     
==========================================
  Files         399      326      -73     
  Lines       23436    22351    -1085     
  Branches     1204     1054     -150     
==========================================
- Hits        12735    11899     -836     
+ Misses       9788     9553     -235     
+ Partials      913      899      -14

Impacted Files	Coverage Δ
cli/cmd/set_config.go	`36.36% <0.00%> (ø)`
cli/pkg/version/keptn_version_checker.go	`44.57% <0.00%> (ø)`
cli/pkg/config/cli_config.go	`57.74% <62.26%> (+6.13%)`	⬆️
cli/cmd/root.go	`69.30% <100.00%> (+6.20%)`	⬆️
cli/cmd/version.go	`52.72% <100.00%> (-5.17%)`	⬇️
cli/pkg/credentialmanager/handler.go	`17.14% <100.00%> (ø)`
cli/pkg/version/version_checker.go	`57.03% <100.00%> (ø)`
...s/ktb-sequence-view/ktb-sequence-view.component.ts	`30.32% <0.00%> (-29.02%)`	⬇️
bridge/client/app/_utils/form.utils.ts	`63.63% <0.00%> (-27.28%)`	⬇️
...ient/app/_interceptors/http-loading-interceptor.ts	`81.81% <0.00%> (-18.19%)`	⬇️
... and 120 more

vadasambar · 2021-11-04T14:20:23Z

cli/cmd/root.go

@@ -62,6 +62,8 @@ func init() {
 	rootCmd.PersistentFlags().BoolVarP(&mocking, "mock", "", false, "Disables communication to a Keptn endpoint")
 	rootCmd.PersistentFlags().StringVarP(&namespace, "namespace", "n", "keptn",
 		"Specify the namespace where Keptn should be installed, used and uninstalled in")
+	rootCmd.PersistentFlags().StringVarP(&cfgFile, "config-file", "", "",


I tried -c (1, 2, 3, 4, 5) and -f shorthands but both are being used in the subcommands. I have left the short hand field empty for now. We can add a shorthand in the future if needed or if there's a good short hand we haven't used and is intuitive, we can go with that.

vadasambar · 2021-11-04T14:26:21Z

cli/cmd/set_config.go

@@ -32,7 +32,7 @@ Supported keys:
 			cmd.SilenceUsage = false
 			return errors.New("required arguments KEY and VALUE")
 		}
-		configMng = config.NewCLIConfigManager()
+		configMng = config.NewCLIConfigManager("")


Function signature NewCLIConfigManager is changed to support custom config file path. I have done a global replace NewCLIConfigManager() -> NewCLIConfigManager("")

vadasambar · 2021-11-04T14:33:29Z

cli/go.mod

@@ -20,6 +20,7 @@ require (
 	github.com/mitchellh/mapstructure v1.4.2
 	github.com/spf13/cast v1.4.1 // indirect
 	github.com/spf13/cobra v1.2.1
+	github.com/spf13/viper v1.9.0


Latest version of Viper at the time of this PR

https://pkg.go.dev/github.com/spf13/viper?tab=versions

vadasambar · 2021-11-04T14:39:48Z

cli/go.sum

@@ -34,6 +34,7 @@ cloud.google.com/go/bigquery v1.8.0/go.mod h1:J5hqkt3O0uAFnINi6JXValWIb1v0goeZM7
 cloud.google.com/go/datastore v1.0.0/go.mod h1:LXYbyblFSglQ5pkeyhO+Qmw7ukd3C+pD7TKLgZqpHYE=
 cloud.google.com/go/datastore v1.1.0/go.mod h1:umbIZjpQpHh4hmRpGhH4tLFup+FVzqBi1b3c64qFpCk=
 cloud.google.com/go/firestore v1.1.0/go.mod h1:ulACoGHTpvq5r8rxGJ4ddJZBZqakUQqClKRT5SZwBmk=
+cloud.google.com/go/firestore v1.6.0/go.mod h1:afJwI0vaXwAG54kI7A//lP/lSPDkQORQuMkv56TxEPU=


All the changes in this file are because of two reasons:

I added viper as a dependency (CI was failing) with

# In the keptn root directory $ go get cli/pkg/config

I updated the dependency from viper 1.8.1 -> 1.9.0 in the go.mod because 1.9.0 is the latest stable version at the time of this comment. 1.8.1 was added automatically on running the go get command above. After updating the go.mod file I ran the same command again

# In the keptn root directory $ go get cli/pkg/config

Did git pull upstream master --rebase and resolved merge conflicts. I accepted both the changes (from upstream master and my branch)

vadasambar · 2021-11-04T14:40:52Z

cli/pkg/config/cli_config.go

 }

+var mgr *CLIConfigManager


Maintain an internal CLIConfigManager struct to avoid re-initializing new viper instances.

vadasambar · 2021-11-04T14:45:14Z

cli/pkg/config/cli_config.go

-	if err := json.Unmarshal(data, &cliConfig); err != nil {
+
+	if err := c.viper.Unmarshal(&cliConfig, func(dConfig *mapstructure.DecoderConfig) {
+		dConfig.DecodeHook = mapstructure.StringToTimeHookFunc(time.RFC3339)


StringToTimeHookFunc decode hook was added because viper.Unmarshal does not know how to unmarshal json time string into *time.Time (it throws an error if we don't add the decode hook)

vadasambar · 2021-11-04T14:47:43Z

cli/pkg/config/cli_config.go

+}
+
+// GetCLIConfig gets the already loaded configuration
+func (c *CLIConfigManager) GetCLIConfig() (CLIConfig, error) {


Many places in the code use LoadCLIConfig which reads in the config file every time LoadCLIConfig function is called. GetCLIConfig is lighter version of the function. It only returns the config already loaded in the Viper.

vadasambar · 2021-11-04T14:48:56Z

cli/pkg/config/cli_config.go

 	data, err := json.Marshal(config)
 	if err != nil {
 		return fmt.Errorf("error when marshalling config file: %w", err)
 	}
-	if err := ioutil.WriteFile(c.CLIConfigPath, []byte(data), 0644); err != nil {
+
+	if err := c.viper.MergeConfig(bytes.NewReader(data)); err != nil {


config passed as parameter overwrites config stored in the Viper

vadasambar · 2021-11-04T14:49:54Z

cli/pkg/config/cli_config.go

 	return nil
 }
+
+// GetKeptnDefaultConfigPath returns default Keptn Config file path
+func GetKeptnDefaultConfigPath() (string, error) {


Helper function to get default keptn config file path

vadasambar · 2021-11-04T14:51:30Z

cli/pkg/config/cli_config_test.go

@@ -60,6 +64,12 @@ func TestStoreCLIConfig(t *testing.T) {
 	}

 	data, err := fileutils.ReadFileAsStr(mng.CLIConfigPath)
+
+	// This is to remove the indentation viper adds
+	data = strings.ReplaceAll(data, " ", "")


Viper stores the config like this

{ "automatic_version_check":true, "current-context":"", "kube_context_check":true, "last_version_check":"2020-02-20T00:00:00Z" }

But we want to flatten it out (remove indentations) so that we can compare it with the testConfig

vadasambar · 2021-11-04T15:31:56Z

cli/pkg/config/cli_config.go

-	KubeContextCheck      bool       `json:"kube_context_check"`
-	LastVersionCheck      *time.Time `json:"last_version_check"`
-	CurrentContext        string     `json:"current-context"`
+	AutomaticVersionCheck bool       `json:"automatic_version_check" mapstructure:"automatic_version_check"`


Viper needs mapstructure to unmarshal the json into a struct.

vadasambar · 2021-11-04T15:33:32Z

cli/pkg/config/cli_config.go

 }

 // CLIConfigManager manages the path of the CLI config
 type CLIConfigManager struct {
 	CLIConfigPath string
+	viper         *viper.Viper


I have kept the field private for now. We can make this public in the future if required.

bacherfl · 2021-11-15T07:11:53Z

cli/pkg/config/cli_config.go

-	dir, err := keptnutils.GetKeptnDirectory()
-	if err != nil {
-		log.Fatal(err)
+		fmt.Printf("no value for --config-file was specified\n defaulting to: %s\n", cfgFile)


please use logging.PrintLog() with VerboseLevel instead of fmt.Printf

I will change this. Thanks for the review 🙏

- custom config file path - this is an overwrite of yesterday's PR - use decode hook to unmarshal time - move viper to CLIConfigMgr - try out writing config using viper - add string param to `NewCLIConfigManager` - use json marshal with viper to write the config -- add a new func to get already loaded cli config -- this would prevent reading in the cli config from the file every time - add viper to go.mod - clean up -- remove shorthand for `config-file` -- shorthand `c` and `f` are already used in subcommands (go test fails) - rename `KeptnConfigPath` -> `GetKeptnDefaultConfigPath` - fix spacing - add viper to go.sum - move cli config path check to NewCLIConfigManager - use specified config path in LoadCLIConfig - print msg about using default config path if none is specified -- print `data` and `testConfig` to analyse the difference -- CI is failing because of ^ - remove indentation added by viper in the tests - add `\n` to log msg - use the same config mgr everywhere -- make viper private (can make it public later if needed) - use viper 1.9.0 -- latest version at the time of this commit - revert cli/main.go (changes not needed) - replace fmt.Printf with verbose level logging.PrintLog - update go.sum Signed-off-by: Suraj Banakar (vadasambar) <surajrbanakar@gmail.com>

sonarcloud · 2021-11-16T16:18:05Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.8% Duplication

bacherfl

lgtm

vadasambar force-pushed the refactor/5490/use-viper-in-root-cmd branch from 29cd494 to 15966bf Compare October 15, 2021 17:03

vadasambar force-pushed the refactor/5490/use-viper-in-root-cmd branch 3 times, most recently from d891e1a to 4c17d6d Compare November 2, 2021 17:56

vadasambar changed the title ~~refactor(cli): (WIP) migrate CLIConfigManager to viper~~ refactor(cli): (WIP) use viper to manage Keptn config Nov 2, 2021

vadasambar force-pushed the refactor/5490/use-viper-in-root-cmd branch from ac615c9 to 1fe2872 Compare November 3, 2021 16:20

vadasambar changed the title ~~refactor(cli): (WIP) use viper to manage Keptn config~~ refactor(cli): Use viper to manage Keptn config Nov 4, 2021

vadasambar commented Nov 4, 2021

View reviewed changes

vadasambar force-pushed the refactor/5490/use-viper-in-root-cmd branch 3 times, most recently from d6e4051 to 8e9701a Compare November 4, 2021 15:28

vadasambar commented Nov 4, 2021

View reviewed changes

vadasambar marked this pull request as ready for review November 4, 2021 15:59

vadasambar requested a review from a team as a code owner November 4, 2021 15:59

vadasambar changed the title ~~refactor(cli): Use viper to manage Keptn config~~ refactor(cli): Use Viper to manage Keptn config Nov 4, 2021

bacherfl reviewed Nov 15, 2021

View reviewed changes

vadasambar force-pushed the refactor/5490/use-viper-in-root-cmd branch 2 times, most recently from 428afea to ef509e0 Compare November 16, 2021 16:04

vadasambar force-pushed the refactor/5490/use-viper-in-root-cmd branch from ef509e0 to 5c5a7be Compare November 16, 2021 16:15

bacherfl approved these changes Nov 17, 2021

View reviewed changes

bacherfl merged commit 498d893 into keptn:master Nov 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(cli): Use Viper to manage Keptn config #5694

refactor(cli): Use Viper to manage Keptn config #5694

vadasambar commented Oct 13, 2021 •

edited

codecov bot commented Nov 3, 2021 •

edited

vadasambar Nov 4, 2021 •

edited

vadasambar Nov 4, 2021 •

edited

vadasambar Nov 4, 2021

vadasambar Nov 4, 2021 •

edited

vadasambar Nov 4, 2021

vadasambar Nov 4, 2021

vadasambar Nov 4, 2021 •

edited

vadasambar Nov 4, 2021

vadasambar Nov 4, 2021

vadasambar Nov 4, 2021

vadasambar Nov 4, 2021

vadasambar Nov 4, 2021

bacherfl Nov 15, 2021

vadasambar Nov 15, 2021

sonarcloud bot commented Nov 16, 2021

bacherfl left a comment

refactor(cli): Use Viper to manage Keptn config #5694

refactor(cli): Use Viper to manage Keptn config #5694

Conversation

vadasambar commented Oct 13, 2021 • edited

Why

How to test

Things to note

codecov bot commented Nov 3, 2021 • edited

Codecov Report

vadasambar Nov 4, 2021 • edited

Choose a reason for hiding this comment

vadasambar Nov 4, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadasambar Nov 4, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadasambar Nov 4, 2021 • edited

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

Choose a reason for hiding this comment

sonarcloud bot commented Nov 16, 2021

bacherfl left a comment

Choose a reason for hiding this comment

vadasambar commented Oct 13, 2021 •

edited

codecov bot commented Nov 3, 2021 •

edited

vadasambar Nov 4, 2021 •

edited

vadasambar Nov 4, 2021 •

edited

vadasambar Nov 4, 2021 •

edited

vadasambar Nov 4, 2021 •

edited