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

Allow multiple slashes in charts (OCI) #4739

Merged
merged 3 commits into from
May 19, 2022

Conversation

antgamdia
Copy link
Contributor

Description of the change

After the analysis at #4284 (comment), I took some time to start fixing this issue.
This PR is the first step in doing so, but I haven't been able to fully test it IRL.

The changes are twofold:

  1. stop assuming the chart id just has two parts (like "repoName/chartName" but instead allow something like repoName/what/ever/you/add/here by escaping it like repoName/what%2Fever%2Fyou%2Fadd%2Fhere.
  2. since it will be stored escaped, during the OCI pull we need to build the proper repository URL, meaning it has to be unescaped like oci://repositoryURL.....repoName/what/ever/you/add/here

Benefits

OCI charts with slashes as part of their names will work.

Possible drawbacks

N/A

Applicable issues

Additional information

The reason I'm leaving it as a draft is mainly that I haven't been able to fully test it. I added a "kube/apps" chart in a local Harbor instance, but the installation failed in Helm because it isn't supporting chart names having slashes, AFAIK?
Perhaps we need either a GCR env or someone jumping in testing it OR an alternative way to repro it.

What's more, I also wonder whether we have made this assumption (chart ids with two parts) in more places we should revisit.

antgamdia added 2 commits May 19, 2022 00:46
Signed-off-by: Antonio Gamez Diaz <[email protected]>
Signed-off-by: Antonio Gamez Diaz <[email protected]>
@netlify
Copy link

netlify bot commented May 18, 2022

Deploy Preview for kubeapps-dev canceled.

Name Link
🔨 Latest commit 8ebdf65
🔍 Latest deploy log https://app.netlify.com/sites/kubeapps-dev/deploys/62863e3b75dd9a0008ffc67c

Copy link
Contributor

@absoludity absoludity left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Antonio Gamez Diaz <[email protected]>

Conflicts:
	cmd/kubeapps-apis/plugins/pkg/pkgutils/pkgutils.go
@antgamdia antgamdia marked this pull request as ready for review May 19, 2022 14:05
@antgamdia antgamdia merged commit 44119d0 into vmware-tanzu:main May 19, 2022
@antgamdia antgamdia deleted the 4284-oci-escape-ids branch May 19, 2022 14:09
@jeunii
Copy link

jeunii commented May 19, 2022

Hello. Thank you for this. In which release should I be able to test this ?

@antgamdia
Copy link
Contributor Author

👋 Instructions here: #4284 (comment)

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.

Unable to add OCI based GCP's Artifact Registry to Kubeapps
4 participants