Allow to sync and store charts with spaces in name (#3758) #3863
Conversation
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
There was a problem hiding this comment.
LGTM! Thanks for this fix!
However, I wonder what would happen if the chart also contains encoded / (%2F) as part of the name.
Have you also checked if a chart whose id is, for instance, my-repo/bitnami/cool wordpress is also stored properly? I guess so, because (from memory I recall) we were handling the %2F when retrieving data from the database, but not sure atm.
Have a look also at: https://github.com/kubeapps/kubeapps/blob/master/cmd/assetsvc/pkg/utils/postgresql_utils.go
btw, I don't know why Go doesn't have a "safe" version of url.PathUnescape(value), it is always so verbose...
|
That scenario is not tested, but I didn't want to mix things up, due to the issue referencing spaces only. |
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
Signed-off-by: Michael Nelson <minelson@vmware.com> <!-- Before you open the request please review the following guidelines and tips to help it be more easily integrated: - Describe the scope of your change - i.e. what the change does. - Describe any known limitations with your change. - Please run any tests or examples that can exercise your modified code. Thank you for contributing! --> ### Description of the change <!-- Describe the scope of your change - i.e. what the change does. --> In #3863 a change was made to ensure that we unescape spaces in the chart name before syncing data to the database, which unintentionally stopped Kubeapps from being able to retrieve charts that include a slash `/` in the chart name (as is the case when using a unified repository with Harbor). It was [noted on the PR as a possibility](#3863 (review)), but at the time, we didn't have non-OCI helm chart names with slashes that we were aware of, so we didn't follow it up. ### Benefits <!-- What benefits will be realized by the code change? --> A unified Harbor Helm repo can be used with Kubeapps. ### Possible drawbacks <!-- Describe any known limitations with your change --> Haven't checked if the chart will display with the escaped slash or not, but still better to be able to display it rather than error. ### Applicable issues <!-- Enter any applicable Issues here (You can reference an issue using #) --> - fixes #5897 ### Additional information <!-- If there's anything else that's important and relevant to your pull request, mention that information here.--> Signed-off-by: Michael Nelson <minelson@vmware.com>
Signed-off-by: Rafa Castelblanque rcastelblanq@vmware.com
Description of the change
This PR allows to sync and store in DB charts that contain spaces in name. End user will then be allowed to reach to the package details page, and when deploying, the error thrown by Helm will be shown.
Applied only to Helm repos, not to OCI.
Benefits
Kubeapps is transparent with regards to charts containing spaces in name. Those charts are synced and stored properly, and end user can navigate to package details now. However Helm installation will still fail due to its restrictions. See here
Possible drawbacks
Packages that can not be installed are listed.
This fix applies only to spaces in name: should we do the same exercise for other special characters? Helm only allows alphanumeric, dash, dot and underscore.
Applicable issues
Additional information