Update `tanzu` context APIs to support ClusterGroup #142

anujc25 · 2023-12-20T14:27:40Z

What this PR does / why we need it

Updates tanzu context APIs to support ClusterGroup
Updates the GetKubeconfigForContext to make it more generalized. Users can pass resource options as opts argument.

- func GetKubeconfigForContext(contextName, projectName, spaceName string) ([]byte, error) {
+ func GetKubeconfigForContext(contextName string, opts ...ResourceOptions) ([]byte, error) {

Example usage of GetKubeconfigForContext:

GetKubeconfigForContext("context-name-1", ForProject("test-project"))
GetKubeconfigForContext("context-name-1", ForProject("test-project"), ForClusterGroup("clustergroup1"))
GetKubeconfigForContext("context-name-1", ForProject("test-project"), ForSpace("space1"))

Updates the SetTanzuContextActiveResource to make it more generalized that now takes the resourceInfo object.

- func SetTanzuContextActiveResource(contextName, projectName, spaceName string, opts ...CommandOptions) error {
+ func SetTanzuContextActiveResource(contextName string, resourceInfo ResourceInfo, opts ...CommandOptions) error {

Example usage of SetTanzuContextActiveResource:

SetTanzuContextActiveResource("test-context", ResourceInfo{ProjectName: "projectA", SpaceName: "spaceA"})
SetTanzuContextActiveResource("test-context", ResourceInfo{ProjectName: "projectA", ClusterGroupName: "clustergroupB"})

Which issue(s) this PR fixes

Fixes #143

Describe testing done for PR

Release note

Add support for `clustergroup` for the `tanzu` context. This updates the API signature for the `GetKubeconfigForContext` and the `SetTanzuContextActiveResource`

Additional information

Special notes for your reviewer

anujc25 · 2023-12-20T16:00:43Z

config/tanzu_context.go

-//	ex: kubeconfig's cluster.server URL:  https://endpoint/org/orgid/project/<projectName>/space/<spaceName>
-func GetKubeconfigForContext(contextName, projectName, spaceName string) ([]byte, error) {
+// Note: Specifying `spaceName` and `clusterGroupName` both at the same time is incorrect input.
+func GetKubeconfigForContext(contextName string, opts ...ResourceOptions) ([]byte, error) {


This function only supports getting kubeconfig for the tanzu context-type and no other context type.

Should we consider renaming this function to be more specific like GetKubeconfigForTanzuContext?
Or
Should we consider supporting the kubernetes context-type and make this API more generic? cc @prkalle @vuil @marckhouzam

Does renaming cause a backwards compatibility problem?

Initially these methods were under tae package and it was not an issue as user has to call them as tae.GetKubeconfigForContext, but later due to renaming the context type to tanzu and re-organizing them under config package, it is causing confusion. So, i feel it is good to rename to avoid confusion. I believe these changes are breaking ( I see SetTanzuContextActiveResource()signature also changed), so we can use this oppurtunity if possible (there should be only 2 affected plugins project and space).

Thanks for the feedback @prkalle and @marckhouzam.

Does renaming cause a backwards compatibility problem?

I think all the APIs related to the tanzu context are experimental APIs and we should be able to make changes to the API at the moment. Also, my existing changes do update the API signature as well.

I will keep this thread open to get feedback from @vuil as well. However, I am not planning to do this rename as part of this PR. If we decide, I will create follow-up PR for the rename.

I think the idea of supporting the kubernetes context-type is interesting.
I'm not sure if it would be easy.

I'd suggest keeping the name look into add suport the kubernetes context-type in a followup, since the usecase for wanting to use this API for tanzu context is not that different from when context type is kubernetes. I also think it should be quite straightfoward to add this support.

prkalle

The changes LGTM.
I would wait for others opinions regarding the discussion regarding renaming the method name GetKubeconfigForContext().

anujc25 · 2023-12-21T07:13:35Z

I would wait for others opinions regarding the discussion regarding renaming the method name GetKubeconfigForContext().

I am not planning to do any method name change as part of this PR. If we decide, I can create a follow-up PR to rename the method.

marckhouzam

LGTM
One nit about adding a extra test.

config/tanzu_context.go

marckhouzam · 2023-12-21T21:43:33Z

config/tanzu_context.go

-//	ex: kubeconfig's cluster.server URL:  https://endpoint/org/orgid/project/<projectName>/space/<spaceName>
-func GetKubeconfigForContext(contextName, projectName, spaceName string) ([]byte, error) {
+// Note: Specifying `spaceName` and `clusterGroupName` both at the same time is incorrect input.
+func GetKubeconfigForContext(contextName string, opts ...ResourceOptions) ([]byte, error) {


I think the idea of supporting the kubernetes context-type is interesting.
I'm not sure if it would be easy.

config/tanzu_context_test.go

anujc25 added 2 commits December 15, 2023 17:18

Add support for clustergroup

00c78b0

Add unit tests

450decc

anujc25 requested a review from a team as a code owner December 20, 2023 14:27

vmwclabot added the cla-not-required label Dec 20, 2023

anujc25 force-pushed the api-to-support-clustergroups branch from 48120d8 to 5d84d5c Compare December 20, 2023 15:05

anujc25 commented Dec 20, 2023

View reviewed changes

anujc25 force-pushed the api-to-support-clustergroups branch from fb79f95 to 450decc Compare December 20, 2023 16:23

prkalle self-requested a review December 21, 2023 00:16

prkalle reviewed Dec 21, 2023

View reviewed changes

anujc25 added kind/feature docs-impact issues with documentation impact labels Dec 21, 2023

anujc25 added this to the v1.2.0 milestone Dec 21, 2023

anujc25 mentioned this pull request Dec 21, 2023

Update tanzu context update tanzu-active-resource to support --clustergroup flag vmware-tanzu/tanzu-cli#621

Merged

marckhouzam approved these changes Dec 21, 2023

View reviewed changes

anujc25 added 2 commits December 26, 2023 11:49

Add unit tests for GetTanzuContextActiveResource with clustergroup

0d6933e

Fix linter errors

363c7ea

anujc25 merged commit ad86606 into vmware-tanzu:main Dec 26, 2023
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update `tanzu` context APIs to support ClusterGroup #142

Update `tanzu` context APIs to support ClusterGroup #142

anujc25 commented Dec 20, 2023 •

edited

Loading

anujc25 Dec 20, 2023

marckhouzam Dec 20, 2023

prkalle Dec 20, 2023 •

edited

Loading

anujc25 Dec 21, 2023

anujc25 Dec 21, 2023

marckhouzam Dec 21, 2023

vuil Dec 26, 2023

prkalle left a comment

anujc25 commented Dec 21, 2023

marckhouzam left a comment

marckhouzam Dec 21, 2023

Update tanzu context APIs to support ClusterGroup #142

Update tanzu context APIs to support ClusterGroup #142

Conversation

anujc25 commented Dec 20, 2023 • edited Loading

What this PR does / why we need it

Which issue(s) this PR fixes

Describe testing done for PR

Release note

Additional information

Special notes for your reviewer

anujc25 Dec 20, 2023

Choose a reason for hiding this comment

marckhouzam Dec 20, 2023

Choose a reason for hiding this comment

prkalle Dec 20, 2023 • edited Loading

Choose a reason for hiding this comment

anujc25 Dec 21, 2023

Choose a reason for hiding this comment

anujc25 Dec 21, 2023

Choose a reason for hiding this comment

marckhouzam Dec 21, 2023

Choose a reason for hiding this comment

vuil Dec 26, 2023

Choose a reason for hiding this comment

prkalle left a comment

Choose a reason for hiding this comment

anujc25 commented Dec 21, 2023

marckhouzam left a comment

Choose a reason for hiding this comment

marckhouzam Dec 21, 2023

Choose a reason for hiding this comment

Update `tanzu` context APIs to support ClusterGroup #142

Update `tanzu` context APIs to support ClusterGroup #142

anujc25 commented Dec 20, 2023 •

edited

Loading

prkalle Dec 20, 2023 •

edited

Loading