Skip to content

Add, delete and update support on helm repo#3617

Merged
msivasubramaniaan merged 5 commits intoredhat-developer:mainfrom
msivasubramaniaan:3609-cud-options-on-helm-repo
Nov 26, 2023
Merged

Add, delete and update support on helm repo#3617
msivasubramaniaan merged 5 commits intoredhat-developer:mainfrom
msivasubramaniaan:3609-cud-options-on-helm-repo

Conversation

@msivasubramaniaan
Copy link
Copy Markdown
Collaborator

Fixes: #3609

Signed-off-by: msivasubramaniaan <msivasub@redhat.com>
Signed-off-by: msivasubramaniaan <msivasub@redhat.com>
@msivasubramaniaan msivasubramaniaan marked this pull request as ready for review November 23, 2023 15:39
@codecov
Copy link
Copy Markdown

codecov bot commented Nov 23, 2023

Codecov Report

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

Comparison is base (2a4f594) 34.22% compared to head (834b6cb) 33.41%.
Report is 1 commits behind head on main.

Files Patch % Lines
src/helm/manageRepository.ts 11.51% 123 Missing ⚠️
...w/helm-manage-repository/manageRepositoryLoader.ts 13.23% 59 Missing ⚠️
src/webview/helm-chart/helmChartLoader.ts 0.00% 27 Missing ⚠️
src/explorer.ts 0.00% 20 Missing ⚠️
src/helm/helm.ts 47.05% 9 Missing ⚠️
src/webview/common-ext/utils.ts 37.50% 5 Missing ⚠️
src/helm/helmCommands.ts 66.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3617      +/-   ##
==========================================
- Coverage   34.22%   33.41%   -0.82%     
==========================================
  Files          82       84       +2     
  Lines        5858     6096     +238     
  Branches     1171     1239      +68     
==========================================
+ Hits         2005     2037      +32     
- Misses       3853     4059     +206     

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

Copy link
Copy Markdown
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.

A few things I noticed:

  • Please add integration tests for the helm commands that you're running
  • It would be nice if the list of charts in the chart view updated when you add/remove a repo, similar to what we do for Devfile registries
  • IMO it's confusing having the list of repos in the tree view and the webview. I think it would be better if we only showed them in the webview, since from the webview you can add, edit and delete repositories, but from the tree view you can't do these things. As an alternative, maybe we can provide add, rename, and delete context menu options and just use quickpicks to set the information, similar to what we did for the devfile registry tree view.

Other things I noticed:

  • I ran into this bug, which makes working with helm repos really annoying, I think we should address it in a separate issue though: #3622

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

@datho7561 The integration test I'll create the separate PR and fixed all other mentioned points. Please have a look

Copy link
Copy Markdown
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.

Looks good, and seems to be working reliably. Thanks, Muthu!

@datho7561
Copy link
Copy Markdown
Contributor

Looks like there are merge conflicts

1 similar comment
@datho7561
Copy link
Copy Markdown
Contributor

Looks like there are merge conflicts

@msivasubramaniaan msivasubramaniaan merged commit 0d6dfaa into redhat-developer:main Nov 26, 2023
@msivasubramaniaan msivasubramaniaan deleted the 3609-cud-options-on-helm-repo branch November 26, 2023 01:38
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.

Manage helm repositories

2 participants