Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sowmya-sl The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
cd2b534 to
d4ef340
Compare
Add dedicated endpoint and handler for installing Helm charts from OCI registries. This provides a simplified installation flow for OCI charts that doesn't require HelmChartRepository lookup. Changes: - Add InstallChartFromURL function in actions for OCI/direct URL installations - Add HandleinstallChartFromURL handler with URL validation - Register /api/helm/oci-chart endpoint in server routes - Add tests for the new methods HELM-598
Add zot server for testing OCI chart operations with TLS and non-TLS modes. - Add zot server scripts (zot.sh, zotWithoutTls.sh, zot-stop.sh) and config files - Add downloadHelm.sh to download helm v3.19.0 for OCI chart uploads - Add OCI registry test case to TestInstallChartFromURL - Add uploadOciCharts.sh and uploadOciChartsWithoutTls.sh for chart uploads
- Add optional chart_version to the OCI chart install API: HelmRequest.ChartVersion and pass it through to InstallChartFromURL so the frontend can send a chart version when the URL has no tag. Set ChartPathOptions.Version from the parameter or from the OCI URL tag (chartVersionFromOCIURL). - Validate chart URLs with url.Parse and a regex on scheme+host+path so query parameters (e.g. ?token=...) are allowed; keep accepting oci:// and http(s)://*.tgz or *.tar.gz. - In test setup: set HELM_VERSION from build info or, when missing (test binaries), from go.mod; replace panics in TestMain with defer/recover so cleanup script failures do not mask test results. - Harden testdata scripts: use [[ ]] in downloadZot.sh; make zot-stop.sh tolerate pkill when zot is not running.
- Rename chartVersionFromOCIURL to chartVersionFromURL and extend it to extract versions from HTTP(S) chart archive filenames using the Helm naming convention (<name>-<version>.tgz), in addition to OCI URL tags. - Simplify isValidChartURL to use url.Parse with scheme/path checks instead of reconstructing a URL string for regex matching. - Streamline version assignment in InstallChartFromURL. - Fix setup_test.go: replace broken defer/recover patterns with direct panic for setup errors and warning logs for teardown; return error from setHelmVersionFromBuildInfo instead of silently ignoring failures. - Align uploadOciCharts.sh flag convention from "tls"/"no-tls" to "--tls"/"--no-tls" and update callers in setup_test.go to match. - Make zot-stop.sh tolerant of missing processes to prevent CI teardown failures. - Add missing skipTLSVerify field to InstallChartFromURL test cases.
Add frontend UI and backend endpoints for installing Helm charts from OCI registries. Frontend: - Add HelmOCIChartForm for entering OCI chart URL and version - Add HelmOCIChartInstallPage for the installation workflow - Add HelmOCIInstallForm for configuring chart values - Add validation utils for OCI URL format - Add OCI Chart menu action in HelmTabbedPage Backend: - Add HandleURLChartGet endpoint for fetching chart info from URL - Add GetChartFromURL action for loading charts from OCI/HTTP URLs - Register /api/helm/oci-chart-get route
d4ef340 to
5866f40
Compare
webbnh
left a comment
There was a problem hiding this comment.
I've only made it part-way through. I've got a bunch of nits and small comments, and I found one bug, but I've hit upon a significant problem in the HTTP URL support, which I think requires us to change our approach.
| "Errors in the form data.": "Errors in the form data.", | ||
| "Invalid Form Schema - {{errorText}}": "Invalid Form Schema - {{errorText}}", | ||
| "Invalid YAML - {{errorText}}": "Invalid YAML - {{errorText}}", | ||
| "Must be a valid OCI registry URL (e.g., oci://...) or a valid HTTP/HTTPS tarball URL (e.g., https://example.com/chart-1.0.0.tgz)": "Must be a valid OCI registry URL (e.g., oci://...) or a valid HTTP/HTTPS tarball URL (e.g., https://example.com/chart-1.0.0.tgz)", |
There was a problem hiding this comment.
I doubt that there is any requirement for the key to be the same as the value (certainly not in the non-English cases!...). Moreover, I'm concerned that trying to make them the same is going to lead to a future failure where we "fix" one and neglect to make the other match (e.g., if we mean to change the value and change just the key instead). (This is to say nothing about how cumbersome they are to use in the code.)
So, I recommend using a key which is representative and mnemonic (and unique, obviously) but reasonably short. For instance,
| "Must be a valid OCI registry URL (e.g., oci://...) or a valid HTTP/HTTPS tarball URL (e.g., https://example.com/chart-1.0.0.tgz)": "Must be a valid OCI registry URL (e.g., oci://...) or a valid HTTP/HTTPS tarball URL (e.g., https://example.com/chart-1.0.0.tgz)", | |
| "Invalid chart URL": "Must be a valid OCI registry URL (e.g., oci://...) or a valid HTTP/HTTPS tarball URL (e.g., https://example.com/chart-1.0.0.tgz)", |
There was a problem hiding this comment.
@webbnh We did not create this file changes. This is generated automatically by executing yarn i18n for en; for other languages, it is done by a translation team. So the key should be equal to the value here. Some examples in this file are
"Edit {{label}}": "Edit {{label}}",
I don't think we should change anything here. Changing the wording of the warning is a different matter which I can do.
| "Errors in the form data.": "Errors in the form data.", | ||
| "Invalid Form Schema - {{errorText}}": "Invalid Form Schema - {{errorText}}", | ||
| "Invalid YAML - {{errorText}}": "Invalid YAML - {{errorText}}", | ||
| "Must be a valid OCI registry URL (e.g., oci://...) or a valid HTTP/HTTPS tarball URL (e.g., https://example.com/chart-1.0.0.tgz)": "Must be a valid OCI registry URL (e.g., oci://...) or a valid HTTP/HTTPS tarball URL (e.g., https://example.com/chart-1.0.0.tgz)", |
There was a problem hiding this comment.
Must be a valid OCI registry URL
Isn't it supposed to be an OCI repository? (An OCI registry URL would not uniquely identify a chart.)
| (value.startsWith('http') && value.includes('://')) || | ||
| value.endsWith('.tgz') || | ||
| value.endsWith('.tar.gz') |
There was a problem hiding this comment.
I think we're suffering from a naming problem: this function is called getHelmOCIValidationSchema(), but these tests here are all about stuff related to a URL with an HTTP(S) scheme.
| export const getHelmOCIValidationSchema = (t: TFunction) => { | ||
| return yup.object().shape({ |
There was a problem hiding this comment.
I'm not a Javascript expert, but defining an exported constant which is set to the value of an "arrow function" that contains code sure seems like it would be better expressed by using a function declaration:
| export const getHelmOCIValidationSchema = (t: TFunction) => { | |
| return yup.object().shape({ | |
| export function getHelmOCIValidationSchema(t: TFunction) { | |
| return yup.object().shape({ |
On the other hand, if you really love that arrow operator, you don't need the return statement if you drop the code block:
| export const getHelmOCIValidationSchema = (t: TFunction) => { | |
| return yup.object().shape({ | |
| export const getHelmOCIValidationSchema = (t: TFunction) => yup.object().shape({ |
| (value) => { | ||
| if (!value) return false; | ||
| return ( | ||
| value.startsWith('oci://') || | ||
| (value.startsWith('http') && value.includes('://')) || | ||
| value.endsWith('.tgz') || | ||
| value.endsWith('.tar.gz') | ||
| ); | ||
| }, |
There was a problem hiding this comment.
I think the expression is incorrect: if the URL has an HTTP/HTTPS scheme, you want it also to be a compressed tar archive -- the above will allow any file if the URL starts with http and any scheme if the file ends with a compressed tar archive filename extension -- I think you want the || on line 20 to be an && (and you need parenthesis around the last two terms).
Independent of that, given that you're already heavy into boolean algebra, here, you can just roll the if condition into the rest of the expression. And, with the if statement gone, you can drop the code block and just yield the value directly:
| (value) => { | |
| if (!value) return false; | |
| return ( | |
| value.startsWith('oci://') || | |
| (value.startsWith('http') && value.includes('://')) || | |
| value.endsWith('.tgz') || | |
| value.endsWith('.tar.gz') | |
| ); | |
| }, | |
| (value) => !!value && ( | |
| value.startsWith('oci://') || | |
| (value.startsWith('http') && value.includes('://')) && | |
| (value.endsWith('.tgz') || value.endsWith('.tar.gz')) | |
| ), |
But, with that many conditions, I'm wondering if a regular expression wouldn't be better....
| (value) => { | |
| if (!value) return false; | |
| return ( | |
| value.startsWith('oci://') || | |
| (value.startsWith('http') && value.includes('://')) || | |
| value.endsWith('.tgz') || | |
| value.endsWith('.tar.gz') | |
| ); | |
| }, | |
| (value) => value?.match(/^oci:\/\/|^https?:\/\/.*\.t(?:ar\.)?gz$/), |
or
| (value) => { | |
| if (!value) return false; | |
| return ( | |
| value.startsWith('oci://') || | |
| (value.startsWith('http') && value.includes('://')) || | |
| value.endsWith('.tgz') || | |
| value.endsWith('.tar.gz') | |
| ); | |
| }, | |
| (value) => value?.match(/^oci:\/\/|^https?:\/\/.*\.(?:tar\.gz|tgz)$/), |
| export interface HelmOCIChartFormProps { | ||
| namespace: string; | ||
| onNext: () => void; | ||
| } | ||
|
|
||
| const HelmOCIChartForm: FC<FormikProps<HelmOCIChartFormData> & HelmOCIChartFormProps> = ({ |
There was a problem hiding this comment.
There's nothing OCI-specific, here, right? That is, this form will also work for HTTP(S) scheme URLs, right? So, we should rename it more generically. Perhaps HelmChartURLForm*.
| const isOCI = values.chartURL.startsWith('oci://'); | ||
| const filename = values.chartURL.split('/').pop() || ''; |
There was a problem hiding this comment.
Rather than tearing apart the URL yourself, consider instantiating a URL object and using url.protocol and url.pathname. (Maybe URL.parse() would be better than the URL() c'tor, since it doesn't throw exceptions for bad inputs.) This will avoid problems with query parameters and other surprising things at the end of the URL.
| // e.g. "exateapigator-0.1.0.tgz" -> name "exateapigator", version "0.1.0" | ||
| const base = filename.replace(/\.(tgz|tar\.gz)$/i, ''); | ||
| const lastDash = base.lastIndexOf('-'); | ||
| if (lastDash >= 0) { | ||
| chartName = base.slice(0, lastDash); | ||
| chartVersion = base.slice(lastDash + 1); | ||
| } else { | ||
| chartName = base; | ||
| } |
There was a problem hiding this comment.
Since you're using a regex at line 45, why not do the whole job with that?
| // e.g. "exateapigator-0.1.0.tgz" -> name "exateapigator", version "0.1.0" | |
| const base = filename.replace(/\.(tgz|tar\.gz)$/i, ''); | |
| const lastDash = base.lastIndexOf('-'); | |
| if (lastDash >= 0) { | |
| chartName = base.slice(0, lastDash); | |
| chartVersion = base.slice(lastDash + 1); | |
| } else { | |
| chartName = base; | |
| } | |
| // e.g. "exateapigator-0.1.0.tgz" -> name "exateapigator", version "0.1.0" | |
| const matches = filename.match(/^(.*?)(?:-([^-]*))?\.(?:tgz|tar\.gz)$/i); | |
| const chartName = matches ? matches[1] ?? "" : ""; | |
| const chartVersion = matches ? matches[2] ?? "" : ""; |
However, trying to pull the version out of the URL is not going to work in all cases: for instance, I believe that exateapigator-0.1.0-alpha.rc-1.tgz is a legal Helm package name (see the SemVer 2.0 spec). So, I don't think that we can extract the version from the URL, and I don't believe that Helm itself even tries. According to Cursor/gpt-5.2-codex, Helm uses the URL to download the chart package, extracts the Chart.yaml file, and pulls the version from the Metadata.Version field, rather than parsing the URL. (Normally, it would have stored this value in the Helm repository, but, in the case of installing from a URL, that's not available.)
So, I think we need to pause and reapproach this. I'm wondering if the backend can help.
Please review only the HELM-609: Add OCI chart installation support commit.