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

There is no any error/warning notification when Helm Chart cannot be installed #4172 #4206

Conversation

vrubezhny
Copy link
Contributor

Fixes: #4172

@vrubezhny
Copy link
Contributor Author

The PR enables the error reporting on Helm Chart installation failure:

Screencast.from.2024-06-14.03-21-15.webm

@codecov-commenter
Copy link

codecov-commenter commented Jun 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 43.69%. Comparing base (da60441) to head (ad8bedd).
Report is 291 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #4206       +/-   ##
===========================================
+ Coverage   32.37%   43.69%   +11.31%     
===========================================
  Files          85       95       +10     
  Lines        6505     7711     +1206     
  Branches     1349     1647      +298     
===========================================
+ Hits         2106     3369     +1263     
+ Misses       4399     4342       -57     

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

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

@mohitsuman
Copy link
Collaborator

@vrubezhny, after the Helm Chart is installed, a "VS Code notification bar indicating the installation is successful" should appear. This notification aids in determining whether the installation process succeeded or failed.

@vrubezhny
Copy link
Contributor Author

@vrubezhny, after the Helm Chart is installed, a "VS Code notification bar indicating the installation is successful" should appear. This notification aids in determining whether the installation process succeeded or failed.

@mohitsuman As you can see on the screencasts the issue #4172 (comment) and here after the fix #4206 (comment) there is "Installing ....` information message when the installation starts, but there is no any "VSCode notification bars" appearing after the successful installation.

Do you want me to add such a notification message? (I suppose it should be an informational message then).

In case of a failure, I believe, the error message appearing on the Helm Chart installation pane is enough to notify the user, isn't it?

datho7561
datho7561 previously approved these changes Jun 17, 2024
Copy link
Contributor

@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.

Works well, but the code can be cleaned up a little. Feel free to fix it or merge as-is.

I think it's okay for the time being to not include a notification that the chart was installed successfully, since currently the user is notified to this fact by the popup window within the webview closing.

src/helm/helm.ts Outdated Show resolved Hide resolved
…installed redhat-developer#4172

Fixes: redhat-developer#4172

Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com>
@vrubezhny vrubezhny force-pushed the fix-helm-chart-installation-failures branch from 8e7a963 to ad8bedd Compare June 17, 2024 16:15
@vrubezhny
Copy link
Contributor Author

I think it's okay for the time being to not include a notification that the chart was installed successfully, since currently the user is notified to this fact by the popup window within the webview closing.

So OK. I'm merging it in. Feel free to open an issue if we need any other notifications to be added

@vrubezhny vrubezhny merged commit 4569cb4 into redhat-developer:main Jun 17, 2024
3 of 4 checks passed
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.

There is no any error/warning notification when Helm Chart cannot be installed
5 participants