-
Notifications
You must be signed in to change notification settings - Fork 308
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
Conversation
@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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
Thank you for reviewing |
Fixes Issue
Fixes #118
Changes proposed
Check List (Check all the applicable boxes)
Screenshots
Note to reviewers