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

Improve error message when podman is present but podman machine isn't #3408

Conversation

datho7561
Copy link
Collaborator

@datho7561 datho7561 commented Oct 6, 2023

Fixes #3405, Fixes #3665

Signed-off-by: David Thompson davidethompson@me.com

const podmanOnPath = which('podman');
if (podmanOnPath) {
const SETUP_INSTRUCTIONS = 'Open setup instructions';
void window.showErrorMessage('Podman is present on the system, but is not fully set up yet.', SETUP_INSTRUCTIONS)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jeffmaury what do you think about this wording?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's gonna fix the issue as Component.odo.isPodmanPresent would return true and code won't be executed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

odo version only seems to report podman as installed if the podman machine is present, so in the case that the binary is on the path but the machine is not set up, it will go to this case. I've tested it on my Windows machine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jeffmaury if you have time, could you please tell what you think of the wording of this message? I'll double check that it shows up under the right circumstances.

@codecov
Copy link

codecov bot commented Oct 6, 2023

Codecov Report

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

Comparison is base (bb79639) 32.37% compared to head (e306126) 32.30%.
Report is 2 commits behind head on main.

Files Patch % Lines
src/openshift/component.ts 15.00% 17 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3408      +/-   ##
==========================================
- Coverage   32.37%   32.30%   -0.08%     
==========================================
  Files          85       85              
  Lines        6505     6513       +8     
  Branches     1349     1351       +2     
==========================================
- Hits         2106     2104       -2     
- Misses       4399     4409      +10     

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

@datho7561
Copy link
Collaborator Author

I'm going to work on #3665 as a part of this issue, since odo is not longer actively developed

@datho7561 datho7561 force-pushed the 3405-bad-diagnostic-when-podmachine-not-setup branch 3 times, most recently from 34c424f to 5fdabb2 Compare January 10, 2024 19:10
@datho7561
Copy link
Collaborator Author

Okay. I've updated the pull request so that it no longer uses odo. This also means the logic is a bit easier to follow. When you get a chance, could you please review this PR, @msivasubramaniaan ? The three cases I'm trying to handle are:

  1. podman is not present on the system. When the user tries to run a Devfile component on podman, there is an error message telling the user to install podman
  2. podman is present on the system, but the podman machine is stopped (you can stop the podman machine using podman machine stop). When the user tries to run a Devfile component on podman, there is an error message that tells the user that podman setup is not complete, with a link to the setup docs.
  3. podman is present and the podman machine is running. When the user tries to run a Devfile component, it starts as expected.

This can only be tested on Windows or macOS, since Linux computers (usually) don't use the podman machine (since they can run containers without starting a separate Linux VM).

@datho7561 datho7561 marked this pull request as ready for review January 10, 2024 20:45
@mohitsuman
Copy link
Collaborator

@msivasubramaniaan can you please review this PR soon ?

@msivasubramaniaan
Copy link
Collaborator

@datho7561 Shall I get demo video of it? that would help me to know the step to test the same

@datho7561
Copy link
Collaborator Author

Sorry, I don't have access to a Windows computer today. However, here are the steps to try it out:

  1. Make sure podman is installed and available on path
  2. Stop the podman machine (podman machine stop)
  3. Try to run Dev on podman for a Devfile component. You should get an error message telling you to finish podman setup, along with a button that opens the podman setup documentation

datho7561 and others added 2 commits January 31, 2024 17:03
Fixes redhat-developer#3405

Signed-off-by: David Thompson <davidethompson@me.com>
Signed-off-by: David Thompson <davthomp@redhat.com>
@datho7561 datho7561 force-pushed the 3405-bad-diagnostic-when-podmachine-not-setup branch from 5fdabb2 to e306126 Compare January 31, 2024 22:04
Copy link
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

Sorry for taking much time to review this one.
I have tested all the scenarios on my local and it is working as expected. Thanks @datho7561 for this PR

@datho7561 datho7561 merged commit 008a0f9 into redhat-developer:main Feb 14, 2024
4 of 6 checks passed
@datho7561 datho7561 deleted the 3405-bad-diagnostic-when-podmachine-not-setup branch February 14, 2024 14:04
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.

Investigate replacing Odo.isPodmanPresent podman installed but propose to install podman
4 participants