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

Decouple the Target association with the Context object by introducing ContextType #112

Merged
merged 3 commits into from
Oct 12, 2023

Conversation

anujc25
Copy link
Contributor

@anujc25 anujc25 commented Oct 4, 2023

What this PR does / why we need it

Background:

  • Today, Tanzu CLI does not have a concept of Context-Type and we use the same concept Target to represent different types of Context as well as how we group plugins under the Tanzu CLI command tree.
  • In the existing Tanzu CLI code implementation, a single custom datatype Target is used to represent both the Context-Type and Target concept making both concepts tightly coupled.
  • Today, Target also has some values like global which does not make sense as a Context-Type value and the code assumes that value cannot be a value of a Context-Type.
  • Because of the above, Creating a new Target has the effect of representing a new type of context and groups a set of plugins, but sometimes one of the consequences is unintended.

Change:

  • As part of this PR we are introducing a new field called ContextType within the Context object and Deprecating the existing Target field.
  • The Target field will be kept and set correctly to support CLI and plugins using the previous version of Tanzu Plugin Runtime.

Which issue(s) this PR fixes

Fixes #116

Describe testing done for PR

  • Added/Updated Unit tests
  • Added/Updated Compatibility tests

Test Cases that I am trying to cover:

Unit Test Cases:

  1. Set Context with Target only, Get the Context and it should contain Target and ContextType both
  2. Set Context with ContextType Only, Get the Context and it should contain Target and ContextType both
  3. Set Context with Target:X and ContextType:Y, It should throw validation error while setting.

Compatibility Test Cases:

  1. Set the Context with v1.1.0 CLI (with setting only ContextType) , Get the Context with previous version of CLIs and it should contain correct Target
  2. Set the Context with v1.1.0 CLI (with setting only Target), Get the Context with previous version of CLIs and it should contain correct Target
  3. Set the Context with v0.90 CLI (with Target),
  4. Get the Context with v0.28, v0.90 version of CLI should return context with correct Target
  5. Get the Context with v1.1.0 version of CLI should return context with correct Target and ContextType

Release note

Decouple the Target association with the Context object by introducing ContextType

The following APIs are marked as Deprecated:
- `SetCurrentContext` is deprecated. Use `SetActiveContext` instead
- `GetCurrentContext` is deprecated. Use `GetActiveContext` instead
- `RemoveCurrentContext` is deprecated. Use `RemoveActiveContext` instead
- `GetAllCurrentContextsMap` is deprecated. Use `GetAllActiveContextsMap` instead
- `GetAllCurrentContextsList` is deprecated. Use `GetAllActiveContextsList` instead
- `SyncPluginsForTarget` is deprecated. Use `SyncPluginsForContextType` instead

API Change:
- The `ClientConfig` API contains a notable datatype change of the `CurrentContext` field from `map[Target]string` to `map[ContextType]string`.
- If you are using the `CurrentContext` directly by processing the entire `ClientConfig` object some change might be required. However, we encourage teams to use fine-grained API `GetActiveContext` to get active context.

Additional information

Special notes for your reviewer

@anujc25 anujc25 force-pushed the decouple-context-target branch 3 times, most recently from a1735dc to 204945b Compare October 4, 2023 23:09
@anujc25 anujc25 changed the title Draft: Decouple ContextType and Target concept Decouple the Target association with the Context object by introducing ContextType Oct 4, 2023
@anujc25 anujc25 marked this pull request as ready for review October 4, 2023 23:59
@anujc25 anujc25 requested a review from a team as a code owner October 4, 2023 23:59
Copy link
Contributor

@marckhouzam marckhouzam left a comment

Choose a reason for hiding this comment

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

Is it possible to remove the constants TargetTAE and targetTAE since they are not in any official release? Looking at the code, I realize this will complicate things. However, I think it is important so that we make sure we adapt the code to handle having different contextTypes than Targets; that way, if we need to add another new contextType, we will be ready and not need to also add it as a new target. But this implies completely removing the use of context.Target in the code.

The above will probably complicate this PR a lot. Given the time frame, we should discuss doing it in two phases:

  1. continue as you did by introducing contextType but continuing to use Target by making sure the two are in sync
  2. and after, remove all uses of context.Target.

config/types/clientconfig.go Show resolved Hide resolved
config/types/clientconfig.go Outdated Show resolved Hide resolved
config/types/clientconfig.go Outdated Show resolved Hide resolved
config/types/clientconfig.go Outdated Show resolved Hide resolved
config/types/clientconfig_types.go Outdated Show resolved Hide resolved
config/contexts.go Outdated Show resolved Hide resolved
config/contexts.go Outdated Show resolved Hide resolved
config/contexts.go Outdated Show resolved Hide resolved
config/contexts.go Outdated Show resolved Hide resolved
config/contexts.go Outdated Show resolved Hide resolved
@vuil
Copy link
Contributor

vuil commented Oct 10, 2023

Nit: perhaps for consistency, in test cases (e.g. clientconfig_test/context_additionalmetadata_test/context_test) where the use of the Target: field is not consequential, we should be consistent and update them to specify the equivalent ContextType instead.

config/contexts.go Outdated Show resolved Hide resolved
@anujc25 anujc25 force-pushed the decouple-context-target branch 2 times, most recently from 887b0c5 to 4fc0de8 Compare October 10, 2023 18:15
Copy link
Contributor

@marckhouzam marckhouzam left a comment

Choose a reason for hiding this comment

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

A couple of nits on the latest version

config/contexts.go Outdated Show resolved Hide resolved
config/types/clientconfig.go Outdated Show resolved Hide resolved
config/types/clientconfig_helper.go Show resolved Hide resolved
@anujc25 anujc25 force-pushed the decouple-context-target branch 5 times, most recently from 1f89b66 to 4159f36 Compare October 12, 2023 16:04
* Add ContextType to the Context object
* Deprecate existing APIs using Target and implement alternative functions
* Add/Update Unit tests and Compatibility tests
@anujc25
Copy link
Contributor Author

anujc25 commented Oct 12, 2023

Rebased the PR on top of 6cabd02 commit.

@vuil vuil mentioned this pull request Oct 12, 2023
Copy link
Contributor

@vuil vuil left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the changes and revisions as well.

@vuil
Copy link
Contributor

vuil commented Oct 12, 2023

We can probably use a little more documentation (here or in TZC) to clarify the Context Type introduction.

@anujc25
Copy link
Contributor Author

anujc25 commented Oct 12, 2023

We can probably use a little more documentation (here or in TZC) to clarify the Context Type introduction.

Yes agreed. I will be adding more documentation around the Context Type as part of the follow-up PR.

@anujc25 anujc25 merged commit fa06151 into vmware-tanzu:main Oct 12, 2023
4 checks passed
@marckhouzam marckhouzam added this to the v1.1.0 milestone Oct 20, 2023
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.

Decouple the Target association with the Context object
4 participants