🐛 Fix workspace output when enter into home workspace #2830
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
some unit tests failed, maybe these are related? |
In any case it would be great to ensure there is a unit test covering the fixed bug. |
qiujian16
force-pushed
the
fix-homews
branch
from
c1e5908 to
ed0b757
Compare
|
test updated. actually the original test case should cover this, but th cluster annotation set in the test case is not the same as what it is in the real server. |
| @@ -753,7 +753,7 @@ func TestUse(t *testing.T) { | |||
| AuthInfos: map[string]*clientcmdapi.AuthInfo{"test": {Token: "test"}}, | |||
| }, | |||
| existingObjects: map[logicalcluster.Name][]string{ | |||
| core.RootCluster: {"~"}, | |||
| core.RootCluster: {"~", homeWorkspaceLogicalCluster.String()}, | |||
There was a problem hiding this comment.
Not sure I understand why this addition is necessary.
Afaict, it the failing case would be better tested if we distinguish between homeWorkspaceLogicalCluster name and homeWorkspaceLogicalCluster hierarchical path. Afaik for now we use the same value (path everywhere). But the failing test was precisely related to the fact that those 2 values are different.
There was a problem hiding this comment.
this is not necessary, removed.
There was a problem hiding this comment.
Afaict, it the failing case would be better tested if we distinguish between homeWorkspaceLogicalCluster name and homeWorkspaceLogicalCluster hierarchical path. Afaik for now we use the same value (path everywhere). But the failing test was precisely related to the fact that those 2 values are different.
what about this part of my comment ?
Signed-off-by: Jian Qiu <jqiu@redhat.com>
qiujian16
force-pushed
the
fix-homews
branch
from
ed0b757 to
2e51f5e
Compare
| @@ -215,8 +215,21 @@ func (o *UseWorkspaceOptions) Run(ctx context.Context) error { | |||
| if err != nil { | |||
| return err | |||
| } | |||
| newServerHost = homeWorkspace.Spec.URL | |||
|
|
|||
| u, homeCluster, err := pluginhelpers.ParseClusterURL(homeWorkspace.Spec.URL) | |||
There was a problem hiding this comment.
homeWorkspace.Spec.URL has the path. We just have to extract it from the URL. We have a helper here somewhere.
There was a problem hiding this comment.
Would rather not go through the LogicalCluster for that.
There was a problem hiding this comment.
I do not think currently the value lf homeWorkspace.Spec.URL is the same as path annotation in LogicalCluster. homeWorkspace.Spec.URL is https://xxx/clusters/kvdk2spgmbix while path annotation is user:kcp-admin. Should homeWorkspace.Spec.URL be https://xxx/clusters/user:kcp-admin?
There was a problem hiding this comment.
Would rather not go through the LogicalCluster for that.
@sttts what would be an alternate solution ?
There was a problem hiding this comment.
Should homeWorkspace.Spec.URL be https://xxx/clusters/user:kcp-admin?
Yes, I think it should.
There was a problem hiding this comment.
Ah, so that means that the problem is at a different level, when this URL is set in the Spec.
|
I tested this PR and it does resolve issue #2737 |
|
Replaced by #2846 |
Summary
the workspace output when run
kubectl ws "~"is not user friendly.I was
with the fix, it is changed to
$ kubectl ws "~"
Current workspace is "user:kcp-admin".
Related issue(s)
Fixes #2737