refactor(cli): Use Viper to manage Keptn config by vadasambar · Pull Request #5694 · keptn/keptn

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

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

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

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

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

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

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

cli/pkg/config/cli_config.go

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

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

config passed as parameter overwrites config stored in the Viper

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

cli/pkg/config/cli_config.go

Helper function to get default keptn config file path

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

cli/pkg/config/cli_config_test.go

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

Viper needs mapstructure to unmarshal the json into a struct.

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

cli/pkg/config/cli_config.go

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

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) <[email protected]>

sonarqubecloud · 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