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

When turned to using service account for a Sandbox a wrong warning me… #3879

Conversation

vrubezhny
Copy link
Contributor

…ssage appears #3878

Fixes: #3878

Copy link

codecov bot commented Feb 5, 2024

Codecov Report

Attention: 55 lines in your changes are missing coverage. Please review.

Comparison is base (da60441) 32.37% compared to head (6feb4ba) 45.42%.
Report is 36 commits behind head on main.

Files Patch % Lines
src/openshift/cluster.ts 17.64% 28 Missing ⚠️
.../webview/create-component/createComponentLoader.ts 36.36% 7 Missing ⚠️
src/registriesView.ts 78.26% 5 Missing ⚠️
src/util/loginUtil.ts 44.44% 5 Missing ⚠️
src/webview/devfile-registry/registryViewLoader.ts 57.14% 3 Missing ⚠️
src/deployment.ts 60.00% 2 Missing ⚠️
src/util/kubeUtils.ts 66.66% 2 Missing ⚠️
src/webview/common-ext/createComponentHelpers.ts 50.00% 2 Missing ⚠️
src/explorer.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #3879       +/-   ##
===========================================
+ Coverage   32.37%   45.42%   +13.05%     
===========================================
  Files          85       85               
  Lines        6505     6487       -18     
  Branches     1349     1341        -8     
===========================================
+ Hits         2106     2947      +841     
+ Misses       4399     3540      -859     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@datho7561
Copy link
Collaborator

I haven't run into this issue, and I'm not able to reproduce it, so I have no idea if this fixes it.

@vrubezhny
Copy link
Contributor Author

vrubezhny commented Feb 5, 2024

In order to reproduce you need to have at least two contexts pointing to a Sandbox cluster, one of them is to be a valid account (valid user and namespace), and the second one some "outdated" (pointing to a different project or an outdated username), f.i. I have:

  • a working context:
  - context: 
      cluster: api-sandbox-m3-1530-p1-openshiftapps-com:6443
      namespace: vrubezhny-dev
      user: vrubezhny/api-sandbox-m3-1530-p1-openshiftapps-com:6443
    name: vrubezhny-dev/api-sandbox-m3-1530-p1-openshiftapps-com:6443/vrubezhny
  • an "outdated" context:
  - context:
      cluster: api-sandbox-m3-1530-p1-openshiftapps-com:6443
      namespace: vrubezhny-2-dev
      user: vrubezhny-2/api-sandbox-m3-1530-p1-openshiftapps-com:6443
    name: vrubezhny-2-dev/api-sandbox-m3-1530-p1-openshiftapps-com:6443/vrubezhny-2

Steps to reproduce:

  • Log out from your Sandbox account

  • Switch to some other OS or Kind context (so you'll be able to switch to any of your Sandbox accounts later)

  • Copy your Sandbox's active user token to clipboard

  • Switch to the real working Sandbox context using your token and when asked if you want to use a service account token - answer Yes.

  • Observe the switching is complete with no Warnings

  • Log out from your Sandbox account and switch to any other OS or Kind context

  • Again copy your Sandbox's active user token to clipboard (if needed)

  • Switch to the "outdated" Sandbox context using your token and when asked if you want to use a service account token - answer Yes (actually if you choose No the Warning message is still to appear)

  • Observe the switching is complete with but the mentioned Warning appeared this time (because the real project and user you're switched to are different from the ones specified in the selected context)

  • Observe (in your Kube config) that finally the context you're switched to is your real working Sandbox context, not the "outdated" one

@datho7561
Copy link
Collaborator

I think I probably misunderstand something. If you're trying switching to the outdated context, but then using the token causes you to switch to the valid context, then it makes sense to present the warning that the namespace is different.

@vrubezhny
Copy link
Contributor Author

vrubezhny commented Feb 7, 2024

I think I probably misunderstand something. If you're trying switching to the outdated context, but then using the token causes you to switch to the valid context, then it makes sense to present the warning that the namespace is different.

This is exactly what it should do.
But bug is that when you're switching to a VALID sandbox context and select "switch to using service account" you still be informed on that project/namespace is different while the cluster/user/namespace are actually the same.
(And the issue is that I get context info too early (before it's really logged in and switched), so the new context is compared not against the selected context, but the previously used one.

@vrubezhny
Copy link
Contributor Author

@datho7561 here is the demo of the wrong warning message appearance:

Screencast.from.2024-02-12.15-02-20.webm

We're going to switch to project vrubezhny-dev on a sandbox cluster, once logged in we got proposed to switch to the service account and it's done after we agreed. So, finally we're logged in and switched to the required vrubezhny-dev project on the required sandbox cluster - so no any warning is to be displayed. But it IS still displayed.

Copy link
Collaborator

@datho7561 datho7561 left a comment

Choose a reason for hiding this comment

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

Thanks for the screencast, Victor. For whatever reason, I can't reproduce your issue on my computer. However, this change seems reasonable, and doesn't cause any new issues from my testing, so if it fixes the bug for you, I think we should go ahead and merge this.

First though, it seems there is a function that becomes unused after your change, so we should probably remove it

src/util/kubeUtils.ts Outdated Show resolved Hide resolved
…ssage appears redhat-developer#3878

Fixes: redhat-developer#3878

Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com>
@vrubezhny vrubezhny force-pushed the fix-check-cluster-user-namespace-after-login-to-sandbox branch from b06bcc1 to 6feb4ba Compare February 13, 2024 16:45
Copy link
Collaborator

@datho7561 datho7561 left a comment

Choose a reason for hiding this comment

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

Looks good. Thank you, Victor!

@datho7561 datho7561 added this to the 1.12.0 milestone Feb 13, 2024
@datho7561 datho7561 merged commit 3fc9300 into redhat-developer:main Feb 13, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

When turned to using service account for a Sandbox a wrong warning message appears
2 participants