From 9c26e3af78d2dee62b9e0706e7b0b66da7fad898 Mon Sep 17 00:00:00 2001 From: Stephan Butler Date: Fri, 4 Sep 2026 14:38:59 +0200 Subject: [PATCH] fix: better support for argo style oci refs --- chartvalidator/checker/applications.go | 37 ++------------ chartvalidator/checker/applications_test.go | 50 ------------------- .../checker/engine_chart_rendering.go | 6 ++- .../checker/engine_chart_rendering_test.go | 4 +- 4 files changed, 10 insertions(+), 87 deletions(-) diff --git a/chartvalidator/checker/applications.go b/chartvalidator/checker/applications.go index 103b050..352cade 100644 --- a/chartvalidator/checker/applications.go +++ b/chartvalidator/checker/applications.go @@ -114,28 +114,11 @@ func extractApplicationCharts(doc map[string]any, envName, path string) []ChartR chartName := str(src["chart"]) repoURL := str(src["repoURL"]) + // Argo requires one of `path`, `chart`, or `ref` on every source, so + // a source with no chart field is a values-only ref source, or a git + // App-of-Apps path. It has nothing to render. if chartName == "" { - // Argo's native OCI sources carry no separate `chart` field. - // The chart itself is the full repoURL, for example - // oci://ghcr.io/interledger/charts/merchant. The chart name - // is that URL's last path segment. - // - // This check only recognizes an explicit "oci://" scheme. It - // does not recognize the scheme-less form ApplicationSets - // accept (see normalizeRepoURL). A scheme-less git remote, - // for example git@github.com:org/repo.git, also has no - // "://". Reading it as OCI would misread it as a chart. - // - // A non-OCI source with no chart field is a values-only ref - // source, or a git App-of-Apps path. It has nothing to - // render. - if !isOCIRepo(repoURL) { - continue - } - chartName = lastPathSegment(repoURL) - if chartName == "" { - continue - } + continue } charts = append(charts, ChartRenderParams{ @@ -149,18 +132,6 @@ func extractApplicationCharts(doc map[string]any, envName, path string) []ChartR return charts } -// lastPathSegment returns a URL or path's final "/"-separated, non-empty -// segment. It names a chart when repoURL already points at the chart -// itself. In that case, no separate chart name field exists. -func lastPathSegment(url string) string { - trimmed := strings.TrimRight(url, "/") - if trimmed == "" { - return "" - } - idx := strings.LastIndex(trimmed, "/") - return trimmed[idx+1:] -} - // applicationValueFiles maps an Application source's helm.valueFiles onto paths // this tool can read, which are relative to the checker's working directory. func applicationValueFiles(src map[string]any, appName, path string) []string { diff --git a/chartvalidator/checker/applications_test.go b/chartvalidator/checker/applications_test.go index 5265bee..18d5a3f 100644 --- a/chartvalidator/checker/applications_test.go +++ b/chartvalidator/checker/applications_test.go @@ -127,56 +127,6 @@ spec: assert.Equal(t, "example", charts[0].ChartName) } -// Argo's native OCI sources carry no separate `chart` field. repoURL is the -// full chart path. The chart name is its last path segment. -func TestChartsFromApplications_OCISourceWithoutChartField(t *testing.T) { - root := t.TempDir() - writeApplicationFile(t, root, "cards-playground", "merchant.yaml", `apiVersion: argoproj.io/v1alpha1 -kind: Application -metadata: - name: merchant -spec: - sources: - - repoURL: git@github.com:interledger/testnet-deploy.git - targetRevision: main - ref: values - - repoURL: oci://ghcr.io/interledger/charts/merchant - targetRevision: 0.0.4 - helm: - valueFiles: - - $values/env/cards-playground/merchant/merchant.yaml -`) - - charts, err := chartsFromApplications("cards-playground", filepath.Join(root, "cards-playground")) - require.NoError(t, err) - require.Len(t, charts, 1, "the values-only ref source must not yield a chart") - - c := charts[0] - assert.Equal(t, "merchant", c.ChartName) - assert.Equal(t, "oci://ghcr.io/interledger/charts/merchant", c.RepoURL) - assert.Equal(t, "0.0.4", c.ChartVersion) -} - -// A scheme-less repoURL with no chart field is not read as an OCI chart. -// It is indistinguishable from an SCP-style git remote, for example -// git@github.com:org/repo.git. That remote also has no "://". -func TestChartsFromApplications_SchemeLessRepoWithoutChartFieldIsSkipped(t *testing.T) { - root := t.TempDir() - writeApplicationFile(t, root, "cards-playground", "merchant.yaml", `apiVersion: argoproj.io/v1alpha1 -kind: Application -metadata: - name: merchant -spec: - source: - repoURL: ghcr.io/interledger/charts/merchant - targetRevision: 0.0.4 -`) - - charts, err := chartsFromApplications("cards-playground", filepath.Join(root, "cards-playground")) - require.NoError(t, err) - assert.Empty(t, charts) -} - func TestChartsFromApplications_MultipleChartSourcesInOneApp(t *testing.T) { root := t.TempDir() writeApplicationFile(t, root, "ilf-1", "two-charts.yaml", `apiVersion: argoproj.io/v1alpha1 diff --git a/chartvalidator/checker/engine_chart_rendering.go b/chartvalidator/checker/engine_chart_rendering.go index 62d4b71..ba3bb5a 100644 --- a/chartvalidator/checker/engine_chart_rendering.go +++ b/chartvalidator/checker/engine_chart_rendering.go @@ -164,8 +164,10 @@ func isOCIRepo(repoURL string) bool { func resolveChartReference(chart ChartRenderParams) string { if isOCIRepo(chart.RepoURL) { trimmed := strings.TrimSuffix(chart.RepoURL, "/") - // Argo's native OCI sources already carry the chart name as - // repoURL's last segment. See lastPathSegment in applications.go. + // An Argo OCI source names the chart twice: repoURL's last segment + // is the chart, and the `chart` field repeats it. Argo requires + // both, because it resolves the revision from repoURL alone, and + // its Application schema requires a `chart` field. // // Only append ChartName when repoURL is a registry or namespace // path that still needs it. Otherwise the reference gets the diff --git a/chartvalidator/checker/engine_chart_rendering_test.go b/chartvalidator/checker/engine_chart_rendering_test.go index 184f023..62aae5e 100644 --- a/chartvalidator/checker/engine_chart_rendering_test.go +++ b/chartvalidator/checker/engine_chart_rendering_test.go @@ -95,8 +95,8 @@ func TestRenderOCIRepoNoScheme(t *testing.T) { assert.Equal(t, expectedCommand, actualCommand) } -// repoURL can already carry the chart name as its last segment. Argo's -// native OCI sources work this way; see lastPathSegment in applications.go. +// repoURL can already carry the chart name as its last segment. Argo's OCI +// sources work this way, and they set the `chart` field to that same name. // The chart reference must not repeat the chart name in that case. func TestRenderOCIRepoChartEmbeddedInRepoURL(t *testing.T) { mockExecutor := createMockExecutor()