rancher / dashboard

The Rancher UI
https://rancher.com
Apache License 2.0
441 stars 243 forks source link

[RFE] Add deprecation warning on SRIOV chart in UI #11042

Closed manuelbuil closed 2 days ago

manuelbuil commented 1 month ago

Is your feature request related to a problem? Please describe. A clear and concise description of what the problem is. Ex. I'm always frustrated when [...]

SRIOV charts will no longer be updated in https://github.com/rancher/charts because their new home is https://github.com/suse-edge/charts. If a user wants to consume SRIOV chart, that user will need to include the new helm repo in Rancher. Currently, the chart is in sync in both repos.

For the v2.9 release, charts will continue to be in both helm repos. For the v2.10 release we expect the chart to not be available via Rancher default charts Therefore, it would be nice to add a deprecation warning in v2.9 so that users are aware of the situation and know what is the way forward.

Describe the solution you'd like A clear and concise description of what you want to happen.

I would like to add a warning in the UI so that users picking SRIOV chart know that it is deprecated and that it will be removed in the v2.10 cycle

Describe alternatives you've considered A clear and concise description of any alternative solutions or features you've considered.

Additional context Add any other context or screenshots about the feature request here.

manuelbuil commented 1 month ago

@mgfritch FYI

gaktive commented 1 month ago

Proposed copy:

(With a Warning sign) This chart will no longer be updated and be removed in 2.10. Please migrate to https://github.com/suse-edge/charts. Check the migrating guide for more information [link]

This may require a corresponding ticket in https://github.com/rancher/docs but putting release-note on this to start. @btat what's the best way to proceed? @manuelbuil will have more details.

manuelbuil commented 1 month ago

I have tested today and the upgrade is pretty painless if you use the same name to install the chart coming from the edge repo. As it does a helm upgrade behind the curtains, the new version of the chart points at the new chart:

azureuser@terraform-mbuil-vm1:~$ helm history sriov -n cattle-sriov-system
REVISION    UPDATED                     STATUS      CHART                                   APP VERSION DESCRIPTION     
1           Tue May 21 14:15:27 2024    superseded  sriov-103.1.0+up0.1.0                   1.2.0       Install complete
2           Tue May 21 15:09:25 2024    deployed    sriov-network-operator-1.2.3+up0.1.0    1.2.0       Upgrade complete
azureuser@terraform-mbuil-vm1:~$ helm history sriov-crd -n cattle-sriov-system
REVISION    UPDATED                     STATUS      CHART                       APP VERSION DESCRIPTION     
1           Tue May 21 14:15:25 2024    superseded  sriov-crd-103.1.0+up0.1.0               Install complete
2           Tue May 21 15:09:24 2024    deployed    sriov-crd-1.2.3+up0.1.0                 Upgrade complete
btat commented 1 month ago

@gaktive @manuelbuil I created https://github.com/rancher/rancher-docs/issues/1297 for the docs portion. We can move docs-specific discussion there.

gaktive commented 1 month ago

Perhaps an annotation on the chart would help out here since having custom deprecation warnings for each chart will not be scalable. Assigning to @nwmac only to figure out a strategy but someone else can work on this.

nwmac commented 1 month ago

@manuelbuil My suggestion would be:

Add support for a new chart annotation catalog.cattle.io/deprecated (similar to catalog.cattle.io/experimental).

When present, the UI will show that the chart is deprecated (this will be shown in the same place as we show 'Experimental' and would take precedence, so we only show one or the other).

When present, we would also show a deprecation notice on the chart details page.

manuelbuil commented 1 month ago

@manuelbuil My suggestion would be:

Add support for a new chart annotation catalog.cattle.io/deprecated (similar to catalog.cattle.io/experimental).

When present, the UI will show that the chart is deprecated (this will be shown in the same place as we show 'Experimental' and would take precedence, so we only show one or the other).

When present, we would also show a deprecation notice on the chart details page.

Sounds like a good idea

gaktive commented 1 month ago

Dev will sync with @kwwii on some of the design to visualize a chart to not use.

This lives in Apps & Marketplace

mgfritch commented 1 month ago

Add support for a new chart annotation catalog.cattle.io/deprecated (similar to catalog.cattle.io/experimental).

I added this new annotation while deprecating rancher sriov chart (https://github.com/rancher/charts/pull/4025). And I also set the helm deprecated chart field (https://helm.sh/docs/topics/charts/#deprecating-charts) with a message about the new chart location in the the NOTES.txt

However, I'm wondering if we really need to add the additional rancher catalog.cattle.io/deprecated annotation? Maybe the existing helm deprecated field from the Chart.yaml is enough?

cc @manuelbuil @nwmac

nwmac commented 3 weeks ago

@manuelbuil @mgfritch @momesgin Yes - good idea - we can read the deprecated flag off of the chart version, so no need for a custom annotation.

momesgin commented 3 weeks ago

@nwmac As I have discovered so far, once the deprecated field is set to true we don't display the chart on the charts list page, unless you put deprecated as a query in the url(e.g. /c/local/apps/charts?deprecated). We can show a deprecated badge on an extension card based on the deprecated field like here:

Image

But in the chart's details page we don't even make the request to get a deprecated chart's data, I'm not sure why exactly this happens, and it's not just Sriov, I also found another chart called Sysdig with the same situation.

Another concern that I have is about the proposed copy:

This chart will no longer be updated and be removed in 2.10. Please migrate to https://github.com/suse-edge/charts. Check the migrating guide for more information [link]

it's a very specific warning message for Sriov, but the UI should display a generic message for all deprecated charts with some dynamic fields like the chart's name, e.g. {{chartName}} will be deprecated.... Unless the UI can read the deprecation message from Chart.yaml, not sure about NOTES.txt that's been mentioned.

nwmac commented 3 weeks ago

@momesgin Okay - so that logic for deprecated charts must be somewhere in the UI code - I'd suggest we always show deprecated charts - or maybe we need to check with @kwwii on having a checkbox for this.

That text is specific - the chart should include that in its readme - we will not use that text in the UI - we should only show a generic banner saying that the chart is deprecated.

yonasberhe23 commented 2 days ago

e2e tests are sufficient. moving to done