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

Display UI indication if chart is upgradable #211

Merged
merged 1 commit into from
Feb 10, 2023
Merged

Conversation

undera
Copy link
Collaborator

@undera undera commented Feb 8, 2023

Fixes Issue

Fixes #118

Changes proposed

Check List (Check all the applicable boxes)

  • My code follows the code style of this project.
  • My change requires changes to the documentation.
  • I have updated the documentation accordingly.
  • All new and existing tests passed.
  • The title of my pull request is a short description of the requested changes.

Screenshots

Selection_001

Note to reviewers

@undera
Copy link
Collaborator Author

undera commented Feb 8, 2023

@hdm23061993 Please review if this matches your expectation

@@ -79,6 +79,20 @@ function buildChartCard(elm) {

loadChartHistory(chart.namespace, chart.name, elm.chart_name)
})

// check if upgrade is possible
$.getJSON("/api/helm/repositories/latestver?name=" + elm.chartName).fail(function (xhr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I was reviewing the change locally and I observed that it takes a while for the API call to work and then display if the chart is upgradeable and this is happening eachtime we visit the 'Installed chart' page

Can we cache this response so that if the user revists the 'Installed chart' page again then we don't make the API call and rather use cache?

It will be a better UX. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

I already tested this locally using cache and seems to be working fine
If you agree then I will push the fix

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With cache, the question is when/how to invalidate that cache. If user updates the repo - the cache has to be cleared.
Feel free to propose PR with your approach to that

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I tested the code using cache and it was working fine. I upgraded and downloaded releases and each time it was showing the correct result.
Let me create a PR and you can review it

@harshit-mehtaa
Copy link
Contributor

@undera If you want you can go ahead and merge this. The part with caching is already in a different PR #213

@undera undera merged commit 61b67f8 into main Feb 10, 2023
@undera undera deleted the upgrade-indication branch February 10, 2023 12:36
@undera
Copy link
Collaborator Author

undera commented Feb 10, 2023

Thank you for reviewing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show upgardable option on Installed charts
2 participants