Tanzu CLI Configuration - Multi file approach #3954

mpanchajanya · 2022-11-20T15:40:19Z

What this PR does / why we need it

Introduce Multi Config files config.yaml and config-ng.yaml to manage client config data.
Introduce new API's to config-metadata to manage what goes into config-ng.yaml.
contexts and currentContexts will be moved to config-ng.yaml.
New typesettings has been added to config-metadata.yaml to store any internal settings related to config operations which are controlled by core cli.
useUnifiedConfig - Used to move all the config related operations to config-ng.yaml. By default this setting is disabled and will be enabled in next future versions of cli.
Existing getClientConfigNode and getClientConfigNodeNoLock function implementation is updated to handle multiple config files.
New factory and filesytem files are added to perform operations on config-ng.yaml.
TANZU_CONFIG_NEXT_GEN Env key is defined to explicitly set config-ng.yaml file.

Major changes

Read from Config

Check for useUnifiedConfig settings in config-metadata.yaml.

if enabled Read config-ng.yaml and construct yaml node.
If disabled go to (2)

Construct root node with cfgItems from config.yaml and next gen nodes from config-ng.yaml.
Return the multi config root node.

Write to Config

Check for useUnifiedConfig settings in config-metadata.yaml.

if enabled Write to config-ng.yaml.
If disabled go to (2)

Create root cfg node and root next gen node.
Process through the change node and construct the root cfg node and root next gen node based on cfgItems.
Write the root cfg node to config.yaml.
Write the root next gen node to config-ng.yaml.
Also populate the legacyClientConfig directory .tanzu.

Tanzu Command changes

tanzu config get cmd implementation is changed to handle multi files. now it will retrieve the combination of both config.yaml and config-ng.yaml

Minor Bug Fix

Fix the legacyConfigFilePath that uses .tanzu directory to not rely on TANZU_CONFIG env variable that is conflicted with config.yaml.

Example:- Default behaviour

configMetadata:
  settings:
    useUnifiedConfig: false

Outcome :- Only contexts and currentContexts read and write to config-ng.yaml

Example :-

configMetadata:
  settings:
    useUnifiedConfig: true

Outcome :- All config item read and write to config-ng.yaml

Which issue(s) this PR fixes

Fixes #2652
Fixes #3810

Describe testing done for PR

Unit Testing and Integration Testing
Tested tanzu config get, tanzu login, tanzu context create etc..

Release note

contexts and current context are now stored in new config-ng.yaml

Additional information

Special notes for your reviewer

Review the clientconfig_factory.go and config_factory.go first to familiarize with the Multi File Read/Write works.

codecov · 2022-11-23T08:13:30Z

Codecov Report

Merging #3954 (5078164) into main (bef7872) will decrease coverage by 0.72%.
The diff coverage is 60.29%.

@@            Coverage Diff             @@
##             main    #3954      +/-   ##
==========================================
- Coverage   48.41%   47.69%   -0.73%     
==========================================
  Files         434      462      +28     
  Lines       43288    45133    +1845     
==========================================
+ Hits        20957    21525     +568     
- Misses      20340    21564    +1224     
- Partials     1991     2044      +53

Impacted Files	Coverage Δ
cli/core/pkg/command/config.go	`41.07% <0.00%> (ø)`
cli/runtime/config/collectionutils/arrayutils.go	`50.00% <0.00%> (-50.00%)`	⬇️
cli/runtime/config/envs.go	`55.55% <ø> (ø)`
cli/runtime/config/metadata_factory.go	`62.50% <ø> (ø)`
cli/runtime/config/metadata_filesystem.go	`50.00% <ø> (-28.58%)`	⬇️
cli/runtime/config/metadata_lock.go	`59.09% <ø> (ø)`
cli/runtime/config/legacy_clientconfig_factory.go	`40.49% <17.39%> (-11.49%)`	⬇️
...i/runtime/config/clientconfignextgen_filesystem.go	`50.00% <50.00%> (ø)`
cli/runtime/config/nodeutils/delete_nodes.go	`63.15% <50.00%> (ø)`
cli/runtime/config/servers.go	`52.34% <50.00%> (+2.34%)`	⬆️
... and 54 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

cli/runtime/config/lock.go

cli/runtime/config/config_factory.go

cli/runtime/config/nodeutils/helpers.go

vuil

Will send additional comments shortly.
Nit: did you mean the section headers in the PR description to be rendered differently in markdown (they are not ATM)

cli/runtime/config/cli_discovery_sources_it_test.go

chandrareddyp

this is a review for below code in in node_types.go file:

type CfgNode struct {
	ForceCreate bool  // Set to True to create nodes of missing keys. Ex: True for Add/Update operations on yaml node, False for Get/Delete operations on yaml node
	Keys        []Key // keys of config nodes passed in hierarchical order. Ex: [ClientOptions, CLI, DiscoverySources] to get the DiscoverySources node from ClientOptions yaml node
}

its not part of this PR, it was there in previous PR #3905
if the flag ForceCreate is set true in case of Get/Delete, then it creates new node with given key, i don't think its good approach, because assume that i am checking if a context or server is available, if not then we are creating server/context with given key and empty value, later if i want to create server/context, i can not create as its already exists.

I feel better to restrict the flag ForceCreate for just Add/Update operations.
its being used in api func FindNode(node *yaml.Node, opts ...Options)

mpanchajanya

/test install-vc7

cli/runtime/config/metadata_settings_api.go

mpanchajanya · 2022-11-30T20:32:40Z

Will send additional comments shortly. Nit: did you mean the section headers in the PR description to be rendered differently in markdown (they are not ATM)

yea fixed it

cli/runtime/config/config_factory.go

cli/runtime/config/nodeutils/delete_nodes.go

…nd new types contexts and currentContexts into config-ng yaml

mpanchajanya · 2022-12-01T05:04:39Z

this is a review for below code in in node_types.go file:
type CfgNode struct {
	ForceCreate bool  // Set to True to create nodes of missing keys. Ex: True for Add/Update operations on yaml node, False for Get/Delete operations on yaml node
	Keys        []Key // keys of config nodes passed in hierarchical order. Ex: [ClientOptions, CLI, DiscoverySources] to get the DiscoverySources node from ClientOptions yaml node
}
its not part of this PR, it was there in previous PR #3905 if the flag ForceCreate is set true in case of Get/Delete, then it creates new node with given key, i don't think its good approach, because assume that i am checking if a context or server is available, if not then we are creating server/context with given key and empty value, later if i want to create server/context, i can not create as its already exists.

I feel better to restrict the flag ForceCreate for just Add/Update operations. its being used in api func FindNode(node *yaml.Node, opts ...Options)

as discussed in earlier meeting closing the comment

mpanchajanya

/test install-vc7

anujc25 · 2022-12-01T07:20:45Z

/test install-vc7

alfredthenarwhal · 2022-12-01T09:51:52Z

@anujc25: /test install-vc7
Commit: cc53f32

Build failed! Build no: 3427

vuil · 2022-12-01T17:30:06Z

cli/runtime/config/config_factory.go

+	}
+
+	// Store the config data to legacy client config file/location
+	err = persistLegacyClientConfig(node)


Thanks for updating the implementation. It is cleaner and more durable to future config definition changes now.

Unless I am missing something, doesn't this line override what L145 is trying to do?

legacy referred is .tanzu folder L145 is config.yaml.

You are right, misread.
The legacy file is being used by any plugins we support at this point. The only place where it even plays any part is in cli/runtime/config/legacy_clientconfig.go CopyLegacyConfigDir() where function will copy the legacy to the main config file if legacy exist and the main file doesn't (which should not be the case given how aggressively we initialize the main config.yaml).
Keeping the line as is okay, but we should eliminate any code dealing as a followup.

cli/runtime/config/nodeutils/helpers.go

cli/runtime/config/config_factory.go

vuil · 2022-12-01T18:01:33Z

cli/runtime/config/clientconfignextgen_filesystem.go

@@ -0,0 +1,37 @@
+// Copyright 2022 VMware, Inc. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+


question: what's the rationale for naming a set of files in this dir _filesystem.go?
maybe _file.go would be more appropriate?
(If we elect to change, and especially since there are preexisting files already committed, let's do it as a followup.)

vuil

Thanks for the changes! Looks good to me.
Couple of minor things.
At top of the PR description, where you described a lot of what the PR does, please also add a line or two about the why.

vuil · 2022-12-01T20:36:56Z

cli/runtime/config/config_factory_test.go

+}
+
+func TestPersistConfig(t *testing.T) {
+	// Setup config test data


not block, but should you need to rebase, one more suggestion:

As implemented the persistConfig functionality will actually preserve any nodes in the main config.yaml that is not on the hardcode list. It would be good to have a test communicate that behavior.

perhaps something like this in TestPersistConfig

--- a/cli/runtime/config/config_factory_test.go +++ b/cli/runtime/config/config_factory_test.go @@ -25,6 +25,7 @@ clientOptions: local: name: default-local path: standalone + # extra comment my local admin source - local: name: admin-local path: admin @@ -99,6 +100,9 @@ contexts: contextType: tmc currentContext: k8s: test-mc +# extraneous fiels/comments are preserved on file update +extrafield: + extrasubfield: 1 ` expectedCfg := `apiVersion: config.tanzu.vmware.com/v1alpha1 clientOptions: @@ -110,6 +114,7 @@ clientOptions: local: name: default-local path: standalone + # extra comment my local admin source - local: name: admin-local path: admin @@ -184,6 +189,9 @@ contexts: contextType: tmc currentContext: k8s: test-mc +# extraneous fiels/comments are preserved on file update +extrafield: + extrasubfield: 1 `

addressing this in follow up PR

anujc25

Looks good to me.

chandrareddyp

Great job. LGTM!
Thanks.

anujc25 · 2022-12-01T22:40:30Z

This PR looks good to get in. If there are any other minor changes required, we can do it as a followup.

vmwclabot added the cla-not-required label Nov 20, 2022

mpanchajanya force-pushed the pj/multi_file_config branch 4 times, most recently from 56569c3 to 97e9b60 Compare November 20, 2022 21:01

mpanchajanya marked this pull request as ready for review November 21, 2022 16:54

mpanchajanya requested a review from a team as a code owner November 21, 2022 16:54

mpanchajanya force-pushed the pj/multi_file_config branch from 17b77f8 to 0b9f7b6 Compare November 22, 2022 08:26

mpanchajanya force-pushed the pj/multi_file_config branch 8 times, most recently from a56f703 to 8fd1dc1 Compare November 30, 2022 06:44

mpanchajanya mentioned this pull request Nov 30, 2022

Tanzu CLI Configuration - Replace Strategy #3905

Merged

chandrareddyp suggested changes Nov 30, 2022

View reviewed changes

vuil reviewed Nov 30, 2022

View reviewed changes

cli/runtime/config/cli_discovery_sources_it_test.go Outdated Show resolved Hide resolved

chandrareddyp reviewed Nov 30, 2022

View reviewed changes

mpanchajanya commented Nov 30, 2022

View reviewed changes

vuil reviewed Nov 30, 2022

View reviewed changes

cli/runtime/config/metadata_settings_api.go Outdated Show resolved Hide resolved

cli/runtime/config/metadata_settings_api.go Outdated Show resolved Hide resolved

mpanchajanya force-pushed the pj/multi_file_config branch from c73a13e to c5eab42 Compare November 30, 2022 23:56

vuil reviewed Dec 1, 2022

View reviewed changes

cli/runtime/config/config_factory.go Outdated Show resolved Hide resolved

cli/runtime/config/config_factory.go Outdated Show resolved Hide resolved

mpanchajanya force-pushed the pj/multi_file_config branch from 6e198c2 to 95042b8 Compare December 1, 2022 00:40

anujc25 reviewed Dec 1, 2022

View reviewed changes

cli/runtime/config/nodeutils/delete_nodes.go Outdated Show resolved Hide resolved

cli/runtime/config/nodeutils/delete_nodes.go Outdated Show resolved Hide resolved

mpanchajanya and others added 2 commits November 30, 2022 22:51

Implement multi file config to store parts of config in config yaml a…

04a9a1a

…nd new types contexts and currentContexts into config-ng yaml

Fixes from replace pr

7342eca

mpanchajanya added 7 commits November 30, 2022 22:51

fix settings naming

95299cf

test data helper func

ec6c6e1

fix tests after rebase

20d3bb1

clean up tests

f507b85

Refactor the Multi Config read and write

48eedc6

Rename test files with suffix _test

af05602

Test fix and address comments

48b9665

mpanchajanya force-pushed the pj/multi_file_config branch from 79fdc19 to 48b9665 Compare December 1, 2022 04:59

minor fix

cc53f32

mpanchajanya closed this Dec 1, 2022

mpanchajanya reopened this Dec 1, 2022

mpanchajanya commented Dec 1, 2022

View reviewed changes

fix test

9b6a809

vuil reviewed Dec 1, 2022

View reviewed changes

cli/runtime/config/nodeutils/helpers.go Outdated Show resolved Hide resolved

vuil reviewed Dec 1, 2022

View reviewed changes

cli/runtime/config/config_factory.go Outdated Show resolved Hide resolved

vuil reviewed Dec 1, 2022

View reviewed changes

fix tests and persistConfig method to restore existing nodes from config

5078164

vuil approved these changes Dec 1, 2022

View reviewed changes

vuil reviewed Dec 1, 2022

View reviewed changes

mpanchajanya requested a review from chandrareddyp December 1, 2022 21:33

anujc25 reviewed Dec 1, 2022

View reviewed changes

chandrareddyp approved these changes Dec 1, 2022

View reviewed changes

anujc25 added the ok-to-merge PRs should be labelled with this before merging label Dec 1, 2022

mpanchajanya merged commit 1988c63 into main Dec 1, 2022

mpanchajanya deleted the pj/multi_file_config branch December 1, 2022 22:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tanzu CLI Configuration - Multi file approach #3954

Tanzu CLI Configuration - Multi file approach #3954

mpanchajanya commented Nov 20, 2022 •

edited

Loading

codecov bot commented Nov 23, 2022 •

edited

Loading

vuil left a comment

chandrareddyp left a comment

mpanchajanya left a comment

mpanchajanya commented Nov 30, 2022

mpanchajanya commented Dec 1, 2022

mpanchajanya left a comment

anujc25 commented Dec 1, 2022

alfredthenarwhal commented Dec 1, 2022

vuil Dec 1, 2022 •

edited

Loading

mpanchajanya Dec 1, 2022

vuil Dec 1, 2022

vuil Dec 1, 2022

vuil left a comment

vuil Dec 1, 2022 •

edited

Loading

mpanchajanya Dec 1, 2022

anujc25 left a comment

chandrareddyp left a comment

anujc25 commented Dec 1, 2022

		@@ -0,0 +1,37 @@
		// Copyright 2022 VMware, Inc. All Rights Reserved.
		// SPDX-License-Identifier: Apache-2.0

Tanzu CLI Configuration - Multi file approach #3954

Tanzu CLI Configuration - Multi file approach #3954

Conversation

mpanchajanya commented Nov 20, 2022 • edited Loading

What this PR does / why we need it

Major changes

Read from Config

Write to Config

Tanzu Command changes

Minor Bug Fix

Which issue(s) this PR fixes

Describe testing done for PR

Release note

Additional information

Special notes for your reviewer

codecov bot commented Nov 23, 2022 • edited Loading

Codecov Report

vuil left a comment

Choose a reason for hiding this comment

chandrareddyp left a comment

Choose a reason for hiding this comment

mpanchajanya left a comment

Choose a reason for hiding this comment

mpanchajanya commented Nov 30, 2022

mpanchajanya commented Dec 1, 2022

mpanchajanya left a comment

Choose a reason for hiding this comment

anujc25 commented Dec 1, 2022

alfredthenarwhal commented Dec 1, 2022

vuil Dec 1, 2022 • edited Loading

Choose a reason for hiding this comment

mpanchajanya Dec 1, 2022

Choose a reason for hiding this comment

vuil Dec 1, 2022

Choose a reason for hiding this comment

vuil Dec 1, 2022

Choose a reason for hiding this comment

vuil left a comment

Choose a reason for hiding this comment

vuil Dec 1, 2022 • edited Loading

Choose a reason for hiding this comment

mpanchajanya Dec 1, 2022

Choose a reason for hiding this comment

anujc25 left a comment

Choose a reason for hiding this comment

chandrareddyp left a comment

Choose a reason for hiding this comment

anujc25 commented Dec 1, 2022

mpanchajanya commented Nov 20, 2022 •

edited

Loading

codecov bot commented Nov 23, 2022 •

edited

Loading

vuil Dec 1, 2022 •

edited

Loading

vuil Dec 1, 2022 •

edited

Loading