Skip to content

If cluster is not up during login, pick new URL#2834

Merged
rgrunber merged 1 commit intoredhat-developer:mainfrom
datho7561:709-cluster-not-up
Apr 17, 2023
Merged

If cluster is not up during login, pick new URL#2834
rgrunber merged 1 commit intoredhat-developer:mainfrom
datho7561:709-cluster-not-up

Conversation

@datho7561
Copy link
Copy Markdown
Contributor

@datho7561 datho7561 commented Apr 12, 2023

When the user is attempting to login to a cluster, check if the cluster is accessible before prompting for credentials. To check if the cluster is accessible, send a GET request to the cluster API URL; if the API endpoint cannot be found:

  • if the URL appears to be a CRC api URL, offer to start CRC
  • if the URL appears to be a Dev Sandbox URL, link to the information page on Dev Sandbox
  • otherwise, display a warning message and return to the step where the user inputs the cluster URL.

This means that if the user tries to login to a cluster that is not running, they don't have to click through the rest of the inputs before receiving this message. Also, if the cluster is not available since the user mistyped the URL, they have the chance to type it in again.

Fixes #709

Signed-off-by: David Thompson davthomp@redhat.com

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 12, 2023

Codecov Report

Patch coverage: 24.13% and project coverage change: +0.02 🎉

Comparison is base (94a146c) 33.88% compared to head (d302ce9) 33.90%.

❗ Current head d302ce9 differs from pull request most recent head 853fb5a. Consider uploading reports for the commit 853fb5a to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2834      +/-   ##
==========================================
+ Coverage   33.88%   33.90%   +0.02%     
==========================================
  Files          70       70              
  Lines        4326     4347      +21     
  Branches      777      781       +4     
==========================================
+ Hits         1466     1474       +8     
- Misses       2860     2873      +13     
Impacted Files Coverage Δ
src/openshift/cluster.ts 10.16% <24.13%> (-0.42%) ⬇️

... and 3 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@datho7561
Copy link
Copy Markdown
Contributor Author

@msivasubramaniaan if you have time, do you mind reviewing this PR? Thank you!

@msivasubramaniaan
Copy link
Copy Markdown
Collaborator

@datho7561 Can you add the video recording for reference?

@datho7561
Copy link
Copy Markdown
Contributor Author

cluster-no-started.mp4

Comment thread src/openshift/cluster.ts Outdated
@datho7561 datho7561 force-pushed the 709-cluster-not-up branch from 87a2ad9 to 1e0e75d Compare April 14, 2023 15:48
@msivasubramaniaan
Copy link
Copy Markdown
Collaborator

@datho7561 if cluster not accessible it good to show the message but I feel it would be better to show some action that make cluster availability again. WDYT?

@datho7561
Copy link
Copy Markdown
Contributor Author

show some action that make cluster availability again

Sure, I think it would be nice to present options to get the cluster running. Here's what I think we could show:

  • Option to run crc start to start OpenShift Local
  • Option to open a link to Dev Sandbox

I guess we can figure out from the URL the user entered if either of these apply, then prompt them to take that action. I'll take a look into adding that.

@msivasubramaniaan
Copy link
Copy Markdown
Collaborator

show some action that make cluster availability again

Sure, I think it would be nice to present options to get the cluster running. Here's what I think we could show:

  • Option to run crc start to start OpenShift Local
  • Option to open a link to Dev Sandbox

I guess we can figure out from the URL the user entered if either of these apply, then prompt them to take that action. I'll take a look into adding that.

Yeah. Simply we can redirect to the cluster webview page.

@datho7561 datho7561 force-pushed the 709-cluster-not-up branch 2 times, most recently from 4d3e9ba to 4c7ac10 Compare April 14, 2023 19:28
@datho7561
Copy link
Copy Markdown
Contributor Author

Okay cool, I've added a prompts that redirect to the corresponding pages in the "Create Cluster" webview.

When the user is attempting to login to a cluster,
check if the cluster is accessible before prompting for credentials.
To check if the cluster is accessible,
send a GET request to the cluster API URL;
if the API endpoint cannot be found:
- if the URL appears to be a CRC api URL, offer to start CRC
- if the URL appears to be a Dev Sandbox URL, link to the information page on Dev Sandbox
- otherwise, display a warning message and return to the step where the user inputs the cluster URL.

This means that if the user tries to login to a cluster that is not running,
they don't have to click through the rest of the inputs
before receiving this message.
Also, if the cluster is not available since the user mistyped the URL,
they have the chance to type it in again.

Fixes redhat-developer#709

Signed-off-by: David Thompson <davthomp@redhat.com>
@datho7561 datho7561 force-pushed the 709-cluster-not-up branch from 4c7ac10 to 853fb5a Compare April 14, 2023 19:34
Copy link
Copy Markdown
Collaborator

@msivasubramaniaan msivasubramaniaan left a comment

Choose a reason for hiding this comment

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

LGTM

@rgrunber
Copy link
Copy Markdown
Member

@datho7561 change looks fine. Only odd thing I noticed were some rather large delays between entering, say https://api.crc.testing:6443 and the notification asking whether to create the cluster / try another. I would just verify it behaves as expected in those cases.

@datho7561
Copy link
Copy Markdown
Contributor Author

I noticed were some rather large delays between entering, say https://api.crc.testing:6443 and the notification asking whether to create the cluster / try another

Yeah, that's expected unfortunately. It takes a while to figure out the URL is unreachable.

@rgrunber rgrunber merged commit f3d18b5 into redhat-developer:main Apr 17, 2023
@datho7561 datho7561 deleted the 709-cluster-not-up branch April 18, 2023 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Login to Cluster command should check if cluster is started

3 participants