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

added yaml editor for helm values #4406

Conversation

msivasubramaniaan
Copy link
Collaborator

No description provided.

Signed-off-by: msivasubramaniaan <msivasub@redhat.com>
@codecov-commenter
Copy link

codecov-commenter commented Aug 30, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 13 lines in your changes missing coverage. Please review.

Project coverage is 43.49%. Comparing base (da60441) to head (255115e).
Report is 478 commits behind head on main.

Files with missing lines Patch % Lines
src/webview/helm-chart/helmChartLoader.ts 33.33% 10 Missing ⚠️
src/helm/helmCommands.ts 77.77% 2 Missing ⚠️
src/helm/helm.ts 50.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #4406       +/-   ##
===========================================
+ Coverage   32.37%   43.49%   +11.12%     
===========================================
  Files          85       98       +13     
  Lines        6505     8014     +1509     
  Branches     1349     1688      +339     
===========================================
+ Hits         2106     3486     +1380     
- Misses       4399     4528      +129     

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

@vrubezhny
Copy link
Contributor

At the first look... I cannot install a Helm Chart due to "Install" button inaccessible (it's just not visible under the YAML editor and we have no vertical scroll at the same time):

image

@msivasubramaniaan
Copy link
Collaborator Author

At the first look... I cannot install a Helm Chart due to "Install" button inaccessible (it's just not visible under the YAML editor and we have no vertical scroll at the same time):

image

Here is the screenshot on my machine
image
Anyway I am look at the issue which u mentioned. Mean time may I know your screen size?

Signed-off-by: msivasubramaniaan <msivasub@redhat.com>
@vrubezhny
Copy link
Contributor

Here is the screenshot on my machine

Just make the window less in height:

image

The YAML editor scroll appears for a long YAML properties file, but the whole chart window still doesn't show the button if it doesn't fit.

Signed-off-by: msivasubramaniaan <msivasub@redhat.com>
@msivasubramaniaan
Copy link
Collaborator Author

Here is the screenshot on my machine

Just make the window less in height:

image

The YAML editor scroll appears for a long YAML properties file, but the whole chart window still doesn't show the button if it doesn't fit.

Can you please check with the latest code?

@vrubezhny
Copy link
Contributor

Here is the screenshot on my machine

Just make the window less in height:
The YAML editor scroll appears for a long YAML properties file, but the whole chart window still doesn't show the button if it doesn't fit.

Can you please check with the latest code?

What should be changed now? I still see the same 16 rows in the editor... Or it's about font size/spacing?
Or I didn't updated properly?

Anyway, imho, looks good. (However for the small screen/lower resolution users it still could be problematic.)

I would probably make it having like 12 rows of text and think about making the header part (Icon/description + name + version selection) more compact - currently it spreads to at least 1/3 of the overall height. But we can improve it later.
@datho7561 WDYT?

The following questions are not for this PR, but probably for a future:

  • As far as I understand, there are some values that are predefined and can be used as is, but there are some that are required to be edited/entered - can we highlight them somehow, so user surely know which values are mandatory and must be provided?
  • Are we able to validate the entered values before creating a chart? I mean, f. i. port: admin doesn't look like a correct value in the Redhat Wildfly Chart shown above:
  livenessProbe:
    httpGet:
      path: /health/live
      port: admin

@msivasubramaniaan
Copy link
Collaborator Author

Here is the screenshot on my machine

Just make the window less in height:
The YAML editor scroll appears for a long YAML properties file, but the whole chart window still doesn't show the button if it doesn't fit.

Can you please check with the latest code?

What should be changed now? I still see the same 16 rows in the editor... Or it's about font size/spacing? Or I didn't updated properly?

Anyway, imho, looks good. (However for the small screen/lower resolution users it still could be problematic.)

I would probably make it having like 12 rows of text and think about making the header part (Icon/description + name + version selection) more compact - currently it spreads to at least 1/3 of the overall height. But we can improve it later. @datho7561 WDYT?

The following questions are not for this PR, but probably for a future:

  • As far as I understand, there are some values that are predefined and can be used as is, but there are some that are required to be edited/entered - can we highlight them somehow, so user surely know which values are mandatory and must be provided?
  • Are we able to validate the entered values before creating a chart? I mean, f. i. port: admin doesn't look like a correct value in the Redhat Wildfly Chart shown above:
  livenessProbe:
    httpGet:
      path: /health/live
      port: admin

@vrubezhny The schema validation will be taken care on feature PR. @mohitsuman wants this feature in the upcoming release.

Signed-off-by: msivasubramaniaan <msivasub@redhat.com>
Copy link
Contributor

@vrubezhny vrubezhny 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 to me. Thanks!

@vrubezhny vrubezhny merged commit 2d66894 into redhat-developer:main Sep 3, 2024
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.

Allow options to configure Helm Chart with build image and other details
3 participants