Consolidate create/switch/update context with --connect #2112
Conversation
✅ Deploy Preview for vcluster-docs canceled.
|
There was a problem hiding this comment.
@johannesfrey looks good to me, but I'm a little scared to remove the update-current flag, since it is referenced a lot. Can we basically just remove the functionality of it, but still have it as a hidden flag that is deprecated?
| @@ -164,7 +164,7 @@ func CreatePlatform(ctx context.Context, options *CreateOptions, globalFlags *fl | |||
| // check if we should connect to the vcluster or print the kubeconfig | |||
| if options.Connect || options.Print { | |||
| return ConnectPlatform(ctx, &ConnectOptions{ | |||
| UpdateCurrent: options.UpdateCurrent, | |||
| UpdateCurrent: true, | |||
There was a problem hiding this comment.
I think ConnectPlatform and UpdateKubeconfig above duplicate some of the connection functionality
There was a problem hiding this comment.
The ConnectPlatform() function ends with a writkubeconfig() function
There was a problem hiding this comment.
Good point. I checked the code and it looks like ConnectPlatform does call platform.CreateVirtualClusterInstanceOptions internally, so essentially doing the same as the block L148-L162 you mentioned above. I went ahead and removed this block and checked create in the different combinations (--connect=true, --connect=false, --connect=false --print=true once without a platform and once with being logged into the platform). Looks good so far.
pkg/cli/create_platform.go
Outdated
| @@ -145,9 +145,9 @@ func CreatePlatform(ctx context.Context, options *CreateOptions, globalFlags *fl | |||
| } | |||
| log.Donef("Successfully created the virtual cluster %s in project %s", virtualClusterName, options.Project) | |||
|
|
|||
| if options.CreateContext { | |||
| if options.Connect { | |||
There was a problem hiding this comment.
I wonder if this block could just be removed
There was a problem hiding this comment.
I removed it. See the comment above.
johannesfrey
force-pushed
the
consolidate-vcluster-connect
branch
from
9c98907
to
e75b188
Compare
FabianKramm
force-pushed
the
consolidate-vcluster-connect
branch
from
e75b188
to
111dcde
Compare
What issue type does this pull request address? (keep at least one, remove the others)
/kind enhancement
What does this pull request do? Which issues does it resolve? (use
resolves #<issue_number>if possible)resolves #
Please provide a short message that should be published in the vcluster release notes
Fixed an issue where vcluster ...
What else do we need to know?