From e92a1385dc7945d63be6d390ad323f149263346b Mon Sep 17 00:00:00 2001 From: vjda Date: Fri, 17 Apr 2026 15:47:58 +0200 Subject: [PATCH 1/6] fix(helm): collect OCI push build-info for subpaths Resolve the repo key and subpath inside the helm push build-info flow by validating a small candidate set against the OCI artifacts Helm actually uploaded, so build-info no longer assumes every push lands at repository root. Keep the change scoped to push.go, propagate the resolved Artifactory storage path into artifact lookup and manifest-folder properties, and add focused tests for forms 1-4 plus the path-based handlePushCommand cases. --- artifactory/commands/helm/push.go | 161 +++++++-- artifactory/commands/helm/push_test.go | 445 +++++++++++++++++++++++++ 2 files changed, 578 insertions(+), 28 deletions(-) create mode 100644 artifactory/commands/helm/push_test.go diff --git a/artifactory/commands/helm/push.go b/artifactory/commands/helm/push.go index 25008942..11519e53 100644 --- a/artifactory/commands/helm/push.go +++ b/artifactory/commands/helm/push.go @@ -3,16 +3,19 @@ package helm import ( "encoding/json" "fmt" + + "os" + "path" + "strconv" + "strings" + "time" + ioutils "github.com/jfrog/gofrog/io" "github.com/jfrog/jfrog-cli-artifactory/artifactory/commands/ocicontainer" "github.com/jfrog/jfrog-client-go/artifactory" "github.com/jfrog/jfrog-client-go/artifactory/services" servicesUtils "github.com/jfrog/jfrog-client-go/artifactory/services/utils" "github.com/jfrog/jfrog-client-go/utils/io/content" - "os" - "strconv" - "strings" - "time" "github.com/jfrog/build-info-go/entities" "github.com/jfrog/jfrog-client-go/utils/log" @@ -29,20 +32,20 @@ func handlePushCommand(buildInfo *entities.BuildInfo, helmArgs []string, service } appendModuleAndBuildAgentIfAbsent(buildInfo, chartName, chartVersion) log.Debug("Processing push command for chart: ", filePath, " to registry: ", registryURL) - repoName := extractRepositoryNameFromURL(registryURL) + repoKey, subpath, _, resultMap, err := resolveOCIPushArtifacts(registryURL, chartName, chartVersion, serviceManager) + if err != nil { + return err + } timestamp := strconv.FormatInt(time.Now().UnixNano()/int64(time.Millisecond), 10) buildProps := fmt.Sprintf("build.name=%s;build.number=%s;build.timestamp=%s", buildName, buildNumber, timestamp) if project != "" { buildProps += fmt.Sprintf(";build.project=%s", project) } - resultMap, err := searchPushedArtifacts(serviceManager, repoName, chartName, chartVersion, buildProps) - if err != nil { - return fmt.Errorf("failed to search oci layers for %s : %s: %w", chartName, chartVersion, err) - } - if len(resultMap) == 0 { - return fmt.Errorf("no oci layers found for chart: %s : %s", chartName, chartVersion) + manifestFolderPath := path.Join(subpath, chartName) + if err = applyBuildPropertiesOnManifestFolder(serviceManager, repoKey, manifestFolderPath, chartVersion, buildProps); err != nil { + return fmt.Errorf("failed to apply build properties on OCI manifest folder for %s : %s: %w", chartName, chartVersion, err) } - artifactManifest, err := getManifest(resultMap, serviceManager, repoName) + artifactManifest, err := getManifest(resultMap, serviceManager, repoKey) if err != nil { return fmt.Errorf("failed to get manifest") } @@ -69,12 +72,73 @@ func handlePushCommand(buildInfo *entities.BuildInfo, helmArgs []string, service return saveBuildInfo(buildInfo, buildName, buildNumber, project) } -// searchPushedArtifacts searches for pushed OCI artifacts using a search pattern -func searchPushedArtifacts(serviceManager artifactory.ArtifactoryServicesManager, repoName, chartName, chartVersion string, buildProperties string) (map[string]*servicesUtils.ResultItem, error) { +type repoCandidate struct { + repoKey string + subpath string +} + +func resolveOCIPushArtifacts(registryURL, chartName, chartVersion string, sm artifactory.ArtifactoryServicesManager) (repoKey, subpath, storagePath string, resultMap map[string]*servicesUtils.ResultItem, err error) { + rawReference := strings.TrimRight(strings.TrimPrefix(registryURL, oci), "/") + if !strings.Contains(rawReference, "/") { + repoKey = extractRepositoryFromHostSubdomain(rawReference) + if repoKey == "" { + return "", "", "", nil, fmt.Errorf("could not resolve OCI push repository key for %q", registryURL) + } + storagePath = path.Join(chartName, chartVersion) + resultMap, err = searchPushedArtifacts(sm, repoKey, storagePath) + if err != nil { + return "", "", "", nil, fmt.Errorf("failed to search oci layers for %s : %s: %w", chartName, chartVersion, err) + } + if len(resultMap) == 0 { + return "", "", "", nil, fmt.Errorf("could not resolve OCI push repository key for %q", registryURL) + } + return repoKey, "", storagePath, resultMap, nil + } + ref, err := parseOCIReference(rawReference) + if err != nil { + return "", "", "", nil, fmt.Errorf("failed to parse OCI registry URL %q: %w", registryURL, err) + } + for _, candidate := range generateRepoCandidates(ref.Registry, ref.Repository) { + if candidate.repoKey == "" { + continue + } + candidateStoragePath := path.Join(candidate.subpath, chartName, chartVersion) + candidateResultMap, searchErr := searchPushedArtifacts(sm, candidate.repoKey, candidateStoragePath) + if searchErr != nil { + return "", "", "", nil, fmt.Errorf("failed to search oci layers for %s : %s: %w", chartName, chartVersion, searchErr) + } + if len(candidateResultMap) == 0 { + continue + } + return candidate.repoKey, candidate.subpath, candidateStoragePath, candidateResultMap, nil + } + return "", "", "", nil, fmt.Errorf("could not resolve OCI push repository key for %q", registryURL) +} + +func generateRepoCandidates(registry, repository string) []repoCandidate { + if repository == "" { + return []repoCandidate{{repoKey: extractRepositoryFromHostSubdomain(registry)}} + } + segments := strings.FieldsFunc(repository, func(r rune) bool { + return r == '/' + }) + if len(segments) == 0 { + return nil + } + candidates := []repoCandidate{{repoKey: segments[0], subpath: strings.Join(segments[1:], "/")}} + hostRepoKey := extractRepositoryFromHostSubdomain(registry) + if len(segments) > 1 && hostRepoKey != "" && hostRepoKey != segments[0] { + candidates = append(candidates, repoCandidate{repoKey: hostRepoKey, subpath: repository}) + } + return candidates +} + +// searchPushedArtifacts searches for pushed OCI artifacts using a search pattern. +func searchPushedArtifacts(serviceManager artifactory.ArtifactoryServicesManager, repoKey, storagePath string) (map[string]*servicesUtils.ResultItem, error) { aqlQuery := fmt.Sprintf(`{ "repo": "%s", - "path": "%s/%s" - }`, repoName, chartName, chartVersion) + "path": "%s" + }`, repoKey, storagePath) searchParams := services.SearchParams{ CommonParams: &servicesUtils.CommonParams{ Aql: servicesUtils.Aql{ItemsFind: aqlQuery}, @@ -100,28 +164,20 @@ func searchPushedArtifacts(serviceManager artifactory.ArtifactoryServicesManager log.Debug("Found OCI artifact: ", item.Name, " (path: ", item.Path, "/", item.Name, ", sha256: ", item.Sha256, ")") } } - if buildProperties != "" { - err = overwriteReaderWithManifestFolder(reader, repoName, chartName, chartVersion) - if err != nil { - return nil, err - } - reader.Reset() - addBuildPropertiesOnArtifacts(serviceManager, reader, buildProperties) - } return artifacts, nil } // updateReaderContents updates the reader contents by writing the specified JSON value to all file paths -func overwriteReaderWithManifestFolder(reader *content.ContentReader, repo, path, name string) error { +func overwriteReaderWithManifestFolder(reader *content.ContentReader, repoKey, manifestFolderPath, manifestFolderName string) error { if reader == nil { return fmt.Errorf("reader is nil") } jsonData := map[string]interface{}{ "results": []map[string]interface{}{ { - "repo": repo, - "path": path, - "name": name, + "repo": repoKey, + "path": manifestFolderPath, + "name": manifestFolderName, "type": "folder", }, }, @@ -142,6 +198,55 @@ func overwriteReaderWithManifestFolder(reader *content.ContentReader, repo, path return nil } +func newManifestFolderReader(repoKey, manifestFolderPath, manifestFolderName string) (reader *content.ContentReader, cleanup func() error, err error) { + tmpFile, err := os.CreateTemp("", "jfrog-helm-push-manifest-folder-*.json") + if err != nil { + return nil, nil, err + } + tmpFilePath := tmpFile.Name() + if err = tmpFile.Close(); err != nil { + _ = os.Remove(tmpFilePath) + return nil, nil, err + } + reader = content.NewContentReader(tmpFilePath, content.DefaultKey) + cleanup = func() error { + var closeErr error + ioutils.Close(reader, &closeErr) + removeErr := os.Remove(tmpFilePath) + if closeErr != nil { + return closeErr + } + if removeErr != nil && !os.IsNotExist(removeErr) { + return removeErr + } + return nil + } + if err = overwriteReaderWithManifestFolder(reader, repoKey, manifestFolderPath, manifestFolderName); err != nil { + _ = cleanup() + return nil, nil, err + } + reader.Reset() + return reader, cleanup, nil +} + +func applyBuildPropertiesOnManifestFolder(serviceManager artifactory.ArtifactoryServicesManager, repoKey, manifestFolderPath, manifestFolderName, buildProps string) (err error) { + if buildProps == "" { + return nil + } + reader, cleanup, err := newManifestFolderReader(repoKey, manifestFolderPath, manifestFolderName) + if err != nil { + return err + } + defer func() { + cleanupErr := cleanup() + if err == nil && cleanupErr != nil { + err = cleanupErr + } + }() + addBuildPropertiesOnArtifacts(serviceManager, reader, buildProps) + return nil +} + func addBuildPropertiesOnArtifacts(serviceManager artifactory.ArtifactoryServicesManager, reader *content.ContentReader, buildProps string) { propsParams := services.PropsParams{ Reader: reader, diff --git a/artifactory/commands/helm/push_test.go b/artifactory/commands/helm/push_test.go new file mode 100644 index 00000000..9cffafd0 --- /dev/null +++ b/artifactory/commands/helm/push_test.go @@ -0,0 +1,445 @@ +package helm + +import ( + "archive/tar" + "compress/gzip" + "encoding/json" + "fmt" + "io" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/jfrog/build-info-go/entities" + "github.com/jfrog/jfrog-client-go/artifactory" + "github.com/jfrog/jfrog-client-go/artifactory/services" + servicesUtils "github.com/jfrog/jfrog-client-go/artifactory/services/utils" + "github.com/jfrog/jfrog-client-go/utils/io/content" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type pushSearchCall struct { + repo string + path string +} + +type pushPropsCall struct { + props string + item servicesUtils.ResultItem +} + +type pushTestServiceManager struct { + artifactory.EmptyArtifactoryServicesManager + t *testing.T + searchResults map[pushSearchCall][]servicesUtils.ResultItem + searchCalls []pushSearchCall + propsCalls []pushPropsCall + remoteContents map[string]string +} + +func newPushTestServiceManager(t *testing.T) *pushTestServiceManager { + return &pushTestServiceManager{ + t: t, + searchResults: map[pushSearchCall][]servicesUtils.ResultItem{}, + remoteContents: map[string]string{}, + } +} + +func (m *pushTestServiceManager) SearchFiles(params services.SearchParams) (*content.ContentReader, error) { + m.t.Helper() + call := parsePushSearchCall(m.t, params.CommonParams.Aql.ItemsFind) + m.searchCalls = append(m.searchCalls, call) + return newPushSearchReader(m.t, m.searchResults[call]), nil +} + +func (m *pushTestServiceManager) SetProps(params services.PropsParams) (int, error) { + m.t.Helper() + item := new(servicesUtils.ResultItem) + require.NoError(m.t, params.Reader.NextRecord(item)) + m.propsCalls = append(m.propsCalls, pushPropsCall{props: params.Props, item: *item}) + return 1, nil +} + +func (m *pushTestServiceManager) ReadRemoteFile(path string) (io.ReadCloser, error) { + body, ok := m.remoteContents[path] + if !ok { + return nil, fmt.Errorf("unexpected remote path: %s", path) + } + return io.NopCloser(strings.NewReader(body)), nil +} + +func parsePushSearchCall(t *testing.T, aql string) pushSearchCall { + t.Helper() + var query struct { + Repo string `json:"repo"` + Path string `json:"path"` + } + require.NoError(t, json.Unmarshal([]byte(aql), &query)) + return pushSearchCall{repo: query.Repo, path: query.Path} +} + +func newPushSearchReader(t *testing.T, items []servicesUtils.ResultItem) *content.ContentReader { + t.Helper() + tmpFile, err := os.CreateTemp(t.TempDir(), "push-search-*.json") + require.NoError(t, err) + defer func() { + require.NoError(t, tmpFile.Close()) + }() + payload := map[string]any{"results": items} + data, err := json.Marshal(payload) + require.NoError(t, err) + _, err = tmpFile.Write(data) + require.NoError(t, err) + return content.NewContentReader(tmpFile.Name(), content.DefaultKey) +} + +func newOCIArtifact(repo, storagePath, name, sha256 string) servicesUtils.ResultItem { + return servicesUtils.ResultItem{ + Repo: repo, + Path: storagePath, + Name: name, + Type: "file", + Sha256: sha256, + } + +} + +func createChartArchive(t *testing.T, chartName, chartVersion string) string { + t.Helper() + chartPath := filepath.Join(t.TempDir(), fmt.Sprintf("%s-%s.tgz", chartName, chartVersion)) + file, err := os.Create(chartPath) + require.NoError(t, err) + defer func() { + require.NoError(t, file.Close()) + }() + gzWriter := gzip.NewWriter(file) + defer func() { + require.NoError(t, gzWriter.Close()) + }() + tarWriter := tar.NewWriter(gzWriter) + defer func() { + require.NoError(t, tarWriter.Close()) + }() + chartYAML := fmt.Sprintf("apiVersion: v2\nname: %s\nversion: %s\n", chartName, chartVersion) + header := &tar.Header{ + Name: fmt.Sprintf("%s/Chart.yaml", chartName), + Mode: 0o600, + Size: int64(len(chartYAML)), + } + require.NoError(t, tarWriter.WriteHeader(header)) + _, err = tarWriter.Write([]byte(chartYAML)) + require.NoError(t, err) + return chartPath +} + +func createPushManifestJSON(configDigest, layerDigest string) string { + return fmt.Sprintf(`{"config":{"digest":"%s"},"layers":[{"digest":"%s","mediaType":"application/vnd.oci.image.layer.v1.tar+gzip"}]}`, + configDigest, layerDigest) +} + +func TestResolveOCIPushArtifacts(t *testing.T) { + const ( + chartName = "chart" + chartVersion = "0.1.0" + ) + chartStoragePath := chartName + "/" + chartVersion + + tests := []struct { + name string + registryURL string + responses map[pushSearchCall][]servicesUtils.ResultItem + expectedRepoKey string + expectedSubpath string + expectedPath string + expectedCalls []pushSearchCall + expectedErrorText string + }{ + { + name: "form 1 without path resolves host repo locally", + registryURL: "oci://helm-repo.art.com", + responses: map[pushSearchCall][]servicesUtils.ResultItem{ + {repo: "helm-repo", path: chartStoragePath}: {newOCIArtifact("helm-repo", chartStoragePath, "manifest.json", "manifest")}, + }, + expectedRepoKey: "helm-repo", + expectedSubpath: "", + expectedPath: chartStoragePath, + expectedCalls: []pushSearchCall{{repo: "helm-repo", path: chartStoragePath}}, + }, + { + name: "form 2 virtual host with subpath", + registryURL: "oci://helm-repo.art.com/team-a/charts", + responses: map[pushSearchCall][]servicesUtils.ResultItem{ + {repo: "team-a", path: "charts/" + chartStoragePath}: nil, + {repo: "helm-repo", path: "team-a/charts/" + chartStoragePath}: {newOCIArtifact("helm-repo", "team-a/charts/"+chartStoragePath, "manifest.json", "manifest")}, + }, + expectedRepoKey: "helm-repo", + expectedSubpath: "team-a/charts", + expectedPath: "team-a/charts/" + chartStoragePath, + expectedCalls: []pushSearchCall{ + {repo: "team-a", path: "charts/" + chartStoragePath}, + {repo: "helm-repo", path: "team-a/charts/" + chartStoragePath}, + }, + }, + { + name: "host-only URL with trailing slash is normalized", + registryURL: "oci://helm-repo.art.com/", + responses: map[pushSearchCall][]servicesUtils.ResultItem{ + {repo: "helm-repo", path: chartStoragePath}: {newOCIArtifact("helm-repo", chartStoragePath, "manifest.json", "manifest")}, + }, + expectedRepoKey: "helm-repo", + expectedSubpath: "", + expectedPath: chartStoragePath, + expectedCalls: []pushSearchCall{{repo: "helm-repo", path: chartStoragePath}}, + }, + { + name: "form 3 repo in path without extra subpath", + registryURL: "oci://art.company.com/helm-repo", + responses: map[pushSearchCall][]servicesUtils.ResultItem{ + {repo: "helm-repo", path: chartStoragePath}: {newOCIArtifact("helm-repo", chartStoragePath, "manifest.json", "manifest")}, + }, + expectedRepoKey: "helm-repo", + expectedSubpath: "", + expectedPath: chartStoragePath, + expectedCalls: []pushSearchCall{{repo: "helm-repo", path: chartStoragePath}}, + }, + { + name: "form 4 repo in path with extra subpath", + registryURL: "oci://art.company.com/helm-repo/staging/libs", + responses: map[pushSearchCall][]servicesUtils.ResultItem{ + {repo: "helm-repo", path: "staging/libs/" + chartStoragePath}: {newOCIArtifact("helm-repo", "staging/libs/"+chartStoragePath, "manifest.json", "manifest")}, + }, + expectedRepoKey: "helm-repo", + expectedSubpath: "staging/libs", + expectedPath: "staging/libs/" + chartStoragePath, + expectedCalls: []pushSearchCall{{repo: "helm-repo", path: "staging/libs/" + chartStoragePath}}, + }, + { + name: "tries next candidate when plausible path-first candidate has no artifact", + registryURL: "oci://helm-repo.art.com/folder/subfolder", + responses: map[pushSearchCall][]servicesUtils.ResultItem{ + {repo: "folder", path: "subfolder/" + chartStoragePath}: nil, + {repo: "helm-repo", path: "folder/subfolder/" + chartStoragePath}: {newOCIArtifact("helm-repo", "folder/subfolder/"+chartStoragePath, "manifest.json", "manifest")}, + }, + expectedRepoKey: "helm-repo", + expectedSubpath: "folder/subfolder", + expectedPath: "folder/subfolder/" + chartStoragePath, + expectedCalls: []pushSearchCall{ + {repo: "folder", path: "subfolder/" + chartStoragePath}, + {repo: "helm-repo", path: "folder/subfolder/" + chartStoragePath}, + }, + }, + { + name: "chooses real artifact when both multi-label host and first path segment are plausible", + registryURL: "oci://helm-prod.company.example/team-a/charts", + responses: map[pushSearchCall][]servicesUtils.ResultItem{ + {repo: "team-a", path: "charts/" + chartStoragePath}: {newOCIArtifact("team-a", "charts/"+chartStoragePath, "manifest.json", "wrong")}, + {repo: "helm-prod", path: "team-a/charts/" + chartStoragePath}: nil, + }, + expectedRepoKey: "team-a", + expectedSubpath: "charts", + expectedPath: "charts/" + chartStoragePath, + expectedCalls: []pushSearchCall{{repo: "team-a", path: "charts/" + chartStoragePath}}, + }, + { + name: "tries host-based candidate without dash in repo key", + registryURL: "oci://helmrepo.company.example/team-a/charts", + responses: map[pushSearchCall][]servicesUtils.ResultItem{ + {repo: "team-a", path: "charts/" + chartStoragePath}: nil, + {repo: "helmrepo", path: "team-a/charts/" + chartStoragePath}: {newOCIArtifact("helmrepo", "team-a/charts/"+chartStoragePath, "manifest.json", "manifest")}, + }, + expectedRepoKey: "helmrepo", + expectedSubpath: "team-a/charts", + expectedPath: "team-a/charts/" + chartStoragePath, + expectedCalls: []pushSearchCall{ + {repo: "team-a", path: "charts/" + chartStoragePath}, + {repo: "helmrepo", path: "team-a/charts/" + chartStoragePath}, + }, + }, + { + name: "returns error when no candidate resolves", + registryURL: "oci://art.company.com/helm-repo/team-a", + responses: map[pushSearchCall][]servicesUtils.ResultItem{}, + expectedErrorText: "could not resolve OCI push repository key", + expectedCalls: []pushSearchCall{ + {repo: "helm-repo", path: "team-a/" + chartStoragePath}, + {repo: "art", path: "helm-repo/team-a/" + chartStoragePath}, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + serviceManager := newPushTestServiceManager(t) + serviceManager.searchResults = tt.responses + + repoKey, subpath, storagePath, resultMap, err := resolveOCIPushArtifacts(tt.registryURL, chartName, chartVersion, serviceManager) + + if tt.expectedErrorText != "" { + require.Error(t, err) + assert.ErrorContains(t, err, tt.expectedErrorText) + assert.Nil(t, resultMap) + } else { + require.NoError(t, err) + assert.Equal(t, tt.expectedRepoKey, repoKey) + assert.Equal(t, tt.expectedSubpath, subpath) + assert.Equal(t, tt.expectedPath, storagePath) + assert.NotEmpty(t, resultMap) + } + assert.Equal(t, tt.expectedCalls, serviceManager.searchCalls) + }) + } +} + +func TestSearchPushedArtifactsUsesResolvedStoragePath(t *testing.T) { + serviceManager := newPushTestServiceManager(t) + serviceManager.searchResults[pushSearchCall{repo: "helm-repo", path: "team-a/charts/chart/0.1.0"}] = []servicesUtils.ResultItem{ + newOCIArtifact("helm-repo", "team-a/charts/chart/0.1.0", "manifest.json", "manifest"), + } + + resultMap, err := searchPushedArtifacts(serviceManager, "helm-repo", "team-a/charts/chart/0.1.0") + require.NoError(t, err) + assert.Len(t, resultMap, 1) + assert.Equal(t, []pushSearchCall{{repo: "helm-repo", path: "team-a/charts/chart/0.1.0"}}, serviceManager.searchCalls) +} + +func TestNewManifestFolderReader(t *testing.T) { + reader, cleanup, err := newManifestFolderReader("helm-repo", "team-a/charts/chart", "0.1.0") + require.NoError(t, err) + defer func() { + require.NoError(t, cleanup()) + }() + + item := new(servicesUtils.ResultItem) + require.NoError(t, reader.NextRecord(item)) + assert.Equal(t, "helm-repo", item.Repo) + assert.Equal(t, "team-a/charts/chart", item.Path) + assert.Equal(t, "0.1.0", item.Name) + assert.Equal(t, "folder", item.Type) +} + +func TestOverwriteReaderWithManifestFolderUsesRealArtifactoryPath(t *testing.T) { + reader := newPushSearchReader(t, []servicesUtils.ResultItem{ + newOCIArtifact("old-repo", "ignored", "manifest.json", "manifest"), + }) + + require.NoError(t, overwriteReaderWithManifestFolder(reader, "helm-repo", "team-a/charts/chart", "0.1.0")) + reader.Reset() + item := new(servicesUtils.ResultItem) + require.NoError(t, reader.NextRecord(item)) + assert.Equal(t, "helm-repo", item.Repo) + assert.Equal(t, "team-a/charts/chart", item.Path) + assert.Equal(t, "0.1.0", item.Name) + assert.Equal(t, "folder", item.Type) +} + +func TestHandlePushCommandResolvesOCIPaths(t *testing.T) { + tests := []struct { + name string + registryURL string + responses map[pushSearchCall][]servicesUtils.ResultItem + expectedSearches []pushSearchCall + expectedManifest string + expectedPropsRepo string + expectedPropsPath string + expectedPropsName string + }{ + { + name: "form 1 keeps root-only flow without extra disambiguation", + registryURL: "oci://helm-repo.art.com", + responses: map[pushSearchCall][]servicesUtils.ResultItem{ + {repo: "helm-repo", path: "chart/0.1.0"}: { + newOCIArtifact("helm-repo", "chart/0.1.0", "manifest.json", "manifest-sha"), + newOCIArtifact("helm-repo", "chart/0.1.0", "sha256__config", "config-sha"), + newOCIArtifact("helm-repo", "chart/0.1.0", "sha256__layer", "layer-sha"), + }, + }, + expectedSearches: []pushSearchCall{{repo: "helm-repo", path: "chart/0.1.0"}}, + expectedManifest: "helm-repo/chart/0.1.0/manifest.json", + expectedPropsRepo: "helm-repo", + expectedPropsPath: "chart", + expectedPropsName: "0.1.0", + }, + { + name: "path-based push uses real manifest folder with subpath", + registryURL: "oci://helm-repo.art.com/team-a/charts", + responses: map[pushSearchCall][]servicesUtils.ResultItem{ + {repo: "team-a", path: "charts/chart/0.1.0"}: nil, + {repo: "helm-repo", path: "team-a/charts/chart/0.1.0"}: { + newOCIArtifact("helm-repo", "team-a/charts/chart/0.1.0", "manifest.json", "manifest-sha"), + newOCIArtifact("helm-repo", "team-a/charts/chart/0.1.0", "sha256__config", "config-sha"), + newOCIArtifact("helm-repo", "team-a/charts/chart/0.1.0", "sha256__layer", "layer-sha"), + }, + }, + expectedSearches: []pushSearchCall{ + {repo: "team-a", path: "charts/chart/0.1.0"}, + {repo: "helm-repo", path: "team-a/charts/chart/0.1.0"}, + }, + expectedManifest: "helm-repo/team-a/charts/chart/0.1.0/manifest.json", + expectedPropsRepo: "helm-repo", + expectedPropsPath: "team-a/charts/chart", + expectedPropsName: "0.1.0", + }, + { + name: "form 3 path-based repo without extra subpath keeps chart root paths", + registryURL: "oci://art.company.com/helm-repo", + responses: map[pushSearchCall][]servicesUtils.ResultItem{ + {repo: "helm-repo", path: "chart/0.1.0"}: { + newOCIArtifact("helm-repo", "chart/0.1.0", "manifest.json", "manifest-sha"), + newOCIArtifact("helm-repo", "chart/0.1.0", "sha256__config", "config-sha"), + newOCIArtifact("helm-repo", "chart/0.1.0", "sha256__layer", "layer-sha"), + }, + }, + expectedSearches: []pushSearchCall{{repo: "helm-repo", path: "chart/0.1.0"}}, + expectedManifest: "helm-repo/chart/0.1.0/manifest.json", + expectedPropsRepo: "helm-repo", + expectedPropsPath: "chart", + expectedPropsName: "0.1.0", + }, + { + name: "form 4 path-based repo with extra subpath uses resolved manifest folder", + registryURL: "oci://art.company.com/helm-repo/staging/libs", + responses: map[pushSearchCall][]servicesUtils.ResultItem{ + {repo: "helm-repo", path: "staging/libs/chart/0.1.0"}: { + newOCIArtifact("helm-repo", "staging/libs/chart/0.1.0", "manifest.json", "manifest-sha"), + newOCIArtifact("helm-repo", "staging/libs/chart/0.1.0", "sha256__config", "config-sha"), + newOCIArtifact("helm-repo", "staging/libs/chart/0.1.0", "sha256__layer", "layer-sha"), + }, + }, + expectedSearches: []pushSearchCall{{repo: "helm-repo", path: "staging/libs/chart/0.1.0"}}, + expectedManifest: "helm-repo/staging/libs/chart/0.1.0/manifest.json", + expectedPropsRepo: "helm-repo", + expectedPropsPath: "staging/libs/chart", + expectedPropsName: "0.1.0", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Setenv("JFROG_CLI_HOME_DIR", t.TempDir()) + serviceManager := newPushTestServiceManager(t) + serviceManager.searchResults = tt.responses + serviceManager.remoteContents[tt.expectedManifest] = createPushManifestJSON("sha256:config", "sha256:layer") + + buildInfo := &entities.BuildInfo{ + Modules: []entities.Module{{Id: "chart:0.1.0", Type: "helm"}}, + BuildAgent: &entities.Agent{Name: "Helm", Version: "test"}, + } + chartPath := createChartArchive(t, "chart", "0.1.0") + + err := handlePushCommand(buildInfo, []string{chartPath, tt.registryURL}, serviceManager, "build-name", "42", "proj") + require.NoError(t, err) + assert.Equal(t, tt.expectedSearches, serviceManager.searchCalls) + require.Len(t, serviceManager.propsCalls, 1) + assert.Equal(t, tt.expectedPropsRepo, serviceManager.propsCalls[0].item.Repo) + assert.Equal(t, tt.expectedPropsPath, serviceManager.propsCalls[0].item.Path) + assert.Equal(t, tt.expectedPropsName, serviceManager.propsCalls[0].item.Name) + assert.Contains(t, serviceManager.propsCalls[0].props, "build.name=build-name") + assert.Contains(t, serviceManager.propsCalls[0].props, "build.number=42") + assert.Contains(t, serviceManager.propsCalls[0].props, "build.project=proj") + require.Len(t, buildInfo.Modules, 1) + assert.Len(t, buildInfo.Modules[0].Artifacts, 3) + }) + } +} From e9db596692dd6e246c1b837fc95b642380aed2fe Mon Sep 17 00:00:00 2001 From: vjda Date: Fri, 17 Apr 2026 16:24:58 +0200 Subject: [PATCH 2/6] fix(helm): resolve OCI dependency subpaths Resolve OCI dependency layer lookups when charts live under non-root subpaths in Artifactory-backed OCI repositories. The previous flow always searched `chart/version` at the repo root. That worked for root-only layouts, but it missed valid OCI dependencies whose `dep.Repository` points to a nested subpath. Keep the change intentionally scoped to the OCI dependency flow in `layers.go`. Parse the dependency reference structure, generate a small candidate set, and validate candidates with the real artifact search already used by the dependency path. This preserves the existing Classic Helm behavior, avoids reopening the broader shared-resolver exploration, and aligns the error policy with observed AQL behavior: empty results are normal misses, while search errors remain operational failures. As a small follow-up, move the OCI candidate helper into `repository.go` so both push and dependency flows share the same primitive without leaving package-level OCI logic buried in `push.go`. --- artifactory/commands/helm/layers.go | 50 ++++-- artifactory/commands/helm/layers_test.go | 157 +++++++++++++++++++ artifactory/commands/helm/push.go | 23 --- artifactory/commands/helm/push_test.go | 32 +++- artifactory/commands/helm/repository.go | 23 +++ artifactory/commands/helm/repository_test.go | 55 +++++++ 6 files changed, 302 insertions(+), 38 deletions(-) diff --git a/artifactory/commands/helm/layers.go b/artifactory/commands/helm/layers.go index 4c635002..acd33645 100644 --- a/artifactory/commands/helm/layers.go +++ b/artifactory/commands/helm/layers.go @@ -2,6 +2,9 @@ package helm import ( "fmt" + "path" + "strings" + "github.com/jfrog/build-info-go/entities" ioutils "github.com/jfrog/gofrog/io" "github.com/jfrog/jfrog-cli-artifactory/artifactory/commands/ocicontainer" @@ -10,7 +13,6 @@ import ( "github.com/jfrog/jfrog-client-go/artifactory/services" servicesUtils "github.com/jfrog/jfrog-client-go/artifactory/services/utils" "github.com/jfrog/jfrog-client-go/utils/log" - "strings" ) type manifest struct { @@ -64,24 +66,46 @@ func processDependency(dep entities.Dependency, serviceManager artifactory.Artif // addOCILayersForDependency adds all OCI layers for a dependency that has checksums func addOCILayersForDependency(dep entities.Dependency, serviceManager artifactory.ArtifactoryServicesManager, processedDependencies *[]entities.Dependency) { - versionPath := extractDependencyPath(dep.Id) - if versionPath == "" { + chartName, chartVersion, err := parseDependencyID(dep.Id) + if err != nil { log.Error("Failed to find a valid version for dependency: ", dep.Id) return } - repoName := extractRepositoryNameFromURL(dep.Repository) - if repoName == "" { - log.Error("Failed to find a valid repository for dependency: ", dep.Id) - return + registryReference := strings.TrimRight(strings.TrimPrefix(dep.Repository, oci), "/") + var candidates []ociRepoCandidate + if !strings.Contains(registryReference, "/") { + candidates = []ociRepoCandidate{{repoKey: extractRepositoryFromHostSubdomain(registryReference)}} + } else { + ref, parseErr := parseOCIReference(registryReference) + if parseErr != nil { + log.Error("Failed to find a valid repository for dependency: ", dep.Id) + return + } + candidates = generateRepoCandidates(ref.Registry, ref.Repository) } - aqlQuery := fmt.Sprintf(`{ + var ( + repoName string + resultMap map[string]*servicesUtils.ResultItem + ) + for _, candidate := range candidates { + if candidate.repoKey == "" { + continue + } + storagePath := path.Join(candidate.subpath, chartName, chartVersion) + aqlQuery := fmt.Sprintf(`{ "repo": "%s", "path": "%s" - }`, repoName, versionPath) - resultMap, err := searchOCIArtifactsByAQL(serviceManager, aqlQuery) - if err != nil { - log.Debug("Failed to search OCI artifacts for dependency ", dep.Id, " : ", err) - return + }`, candidate.repoKey, storagePath) + resultMap, err = searchOCIArtifactsByAQL(serviceManager, aqlQuery) + if err != nil { + log.Debug("Failed to search OCI artifacts for dependency ", dep.Id, " : ", err) + return + } + if len(resultMap) == 0 { + continue + } + repoName = candidate.repoKey + break } if len(resultMap) == 0 { log.Debug("Did not find any OCI artifacts for dependency: ", dep.Id) diff --git a/artifactory/commands/helm/layers_test.go b/artifactory/commands/helm/layers_test.go index 9abd9713..5539e669 100644 --- a/artifactory/commands/helm/layers_test.go +++ b/artifactory/commands/helm/layers_test.go @@ -1,9 +1,13 @@ package helm import ( + "fmt" "testing" + "github.com/jfrog/build-info-go/entities" + servicesUtils "github.com/jfrog/jfrog-client-go/artifactory/services/utils" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) // TestParseDependencyID tests the parseDependencyID function @@ -90,3 +94,156 @@ func TestExtractDependencyPathInLayers(t *testing.T) { }) } } + +func TestAddOCILayersForDependency(t *testing.T) { + const ( + chartName = "chart" + chartVersion = "0.1.0" + ) + + tests := []struct { + name string + dependency entities.Dependency + responses map[pushSearchCall][]servicesUtils.ResultItem + expectedSearchCalls []pushSearchCall + expectedDependencies []entities.Dependency + manifestRepo string + manifestPath string + }{ + { + name: "single-segment virtual host subpath resolves using host fallback", + dependency: entities.Dependency{ + Id: chartName + ":" + chartVersion, + Repository: "oci://helm-repo.art.com/team-a", + }, + responses: map[pushSearchCall][]servicesUtils.ResultItem{ + {repo: "team-a", path: "chart/0.1.0"}: nil, + {repo: "helm-repo", path: "team-a/chart/0.1.0"}: { + newOCIArtifact("helm-repo", "team-a/chart/0.1.0", "manifest.json", "manifest-sha"), + newOCIArtifact("helm-repo", "team-a/chart/0.1.0", "sha256__config", "config-sha"), + newOCIArtifact("helm-repo", "team-a/chart/0.1.0", "sha256__layer", "layer-sha"), + }, + }, + expectedSearchCalls: []pushSearchCall{ + {repo: "team-a", path: "chart/0.1.0"}, + {repo: "helm-repo", path: "team-a/chart/0.1.0"}, + }, + expectedDependencies: []entities.Dependency{ + newProcessedLayerDependency("manifest.json", "helm-repo", "manifest-sha"), + newProcessedLayerDependency("sha256__config", "helm-repo", "config-sha"), + newProcessedLayerDependency("sha256__layer", "helm-repo", "layer-sha"), + }, + manifestRepo: "helm-repo", + manifestPath: "team-a/chart/0.1.0", + }, + { + name: "oci dependency with non-root subpath resolves using validated candidate", + dependency: entities.Dependency{ + Id: chartName + ":" + chartVersion, + Repository: "oci://helm-repo.art.com/team-a/charts", + }, + responses: map[pushSearchCall][]servicesUtils.ResultItem{ + {repo: "team-a", path: "charts/chart/0.1.0"}: nil, + {repo: "helm-repo", path: "team-a/charts/chart/0.1.0"}: { + newOCIArtifact("helm-repo", "team-a/charts/chart/0.1.0", "manifest.json", "manifest-sha"), + newOCIArtifact("helm-repo", "team-a/charts/chart/0.1.0", "sha256__config", "config-sha"), + newOCIArtifact("helm-repo", "team-a/charts/chart/0.1.0", "sha256__layer", "layer-sha"), + }, + }, + expectedSearchCalls: []pushSearchCall{ + {repo: "team-a", path: "charts/chart/0.1.0"}, + {repo: "helm-repo", path: "team-a/charts/chart/0.1.0"}, + }, + expectedDependencies: []entities.Dependency{ + newProcessedLayerDependency("manifest.json", "helm-repo", "manifest-sha"), + newProcessedLayerDependency("sha256__config", "helm-repo", "config-sha"), + newProcessedLayerDependency("sha256__layer", "helm-repo", "layer-sha"), + }, + manifestRepo: "helm-repo", + manifestPath: "team-a/charts/chart/0.1.0", + }, + { + name: "root-only oci dependency keeps existing resolution", + dependency: entities.Dependency{ + Id: chartName + ":" + chartVersion, + Repository: "oci://helm-repo.art.com", + }, + responses: map[pushSearchCall][]servicesUtils.ResultItem{ + {repo: "helm-repo", path: "chart/0.1.0"}: { + newOCIArtifact("helm-repo", "chart/0.1.0", "manifest.json", "manifest-sha"), + newOCIArtifact("helm-repo", "chart/0.1.0", "sha256__config", "config-sha"), + newOCIArtifact("helm-repo", "chart/0.1.0", "sha256__layer", "layer-sha"), + }, + }, + expectedSearchCalls: []pushSearchCall{{repo: "helm-repo", path: "chart/0.1.0"}}, + expectedDependencies: []entities.Dependency{ + newProcessedLayerDependency("manifest.json", "helm-repo", "manifest-sha"), + newProcessedLayerDependency("sha256__config", "helm-repo", "config-sha"), + newProcessedLayerDependency("sha256__layer", "helm-repo", "layer-sha"), + }, + manifestRepo: "helm-repo", + manifestPath: "chart/0.1.0", + }, + { + name: "oci dependency without matching candidate returns without adding layers", + dependency: entities.Dependency{ + Id: chartName + ":" + chartVersion, + Repository: "oci://helm-repo.art.com/team-a/charts", + }, + responses: map[pushSearchCall][]servicesUtils.ResultItem{ + {repo: "team-a", path: "charts/chart/0.1.0"}: nil, + {repo: "helm-repo", path: "team-a/charts/chart/0.1.0"}: nil, + }, + expectedSearchCalls: []pushSearchCall{ + {repo: "team-a", path: "charts/chart/0.1.0"}, + {repo: "helm-repo", path: "team-a/charts/chart/0.1.0"}, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + serviceManager := newPushTestServiceManager(t) + serviceManager.searchResults = tt.responses + if tt.manifestRepo != "" { + serviceManager.remoteContents[fmt.Sprintf("%s/%s/manifest.json", tt.manifestRepo, tt.manifestPath)] = createPushManifestJSON("sha256:config", "sha256:layer") + } + + var processed []entities.Dependency + addOCILayersForDependency(tt.dependency, serviceManager, &processed) + + assert.Equal(t, tt.expectedSearchCalls, serviceManager.searchCalls) + assert.Equal(t, tt.expectedDependencies, processed) + }) + } +} + +func TestUpdateClassicHelmDependencyChecksumsLeavesExistingChecksumsUntouched(t *testing.T) { + serviceManager := newPushTestServiceManager(t) + dep := entities.Dependency{ + Id: "classic:1.2.3", + Repository: "https://art.company.com/helm-local", + Checksum: entities.Checksum{ + Md5: "md5", + Sha1: "sha1", + Sha256: "sha256", + }, + } + + var processed []entities.Dependency + updateClassicHelmDependencyChecksums(dep, serviceManager, &processed) + + require.Len(t, processed, 1) + assert.Equal(t, dep, processed[0]) + assert.Empty(t, serviceManager.searchCalls) +} + +func newProcessedLayerDependency(name, repo, sha256 string) entities.Dependency { + return entities.Dependency{ + Id: name, + Repository: repo, + Checksum: entities.Checksum{ + Sha256: sha256, + }, + } +} diff --git a/artifactory/commands/helm/push.go b/artifactory/commands/helm/push.go index 11519e53..518223b2 100644 --- a/artifactory/commands/helm/push.go +++ b/artifactory/commands/helm/push.go @@ -72,11 +72,6 @@ func handlePushCommand(buildInfo *entities.BuildInfo, helmArgs []string, service return saveBuildInfo(buildInfo, buildName, buildNumber, project) } -type repoCandidate struct { - repoKey string - subpath string -} - func resolveOCIPushArtifacts(registryURL, chartName, chartVersion string, sm artifactory.ArtifactoryServicesManager) (repoKey, subpath, storagePath string, resultMap map[string]*servicesUtils.ResultItem, err error) { rawReference := strings.TrimRight(strings.TrimPrefix(registryURL, oci), "/") if !strings.Contains(rawReference, "/") { @@ -115,24 +110,6 @@ func resolveOCIPushArtifacts(registryURL, chartName, chartVersion string, sm art return "", "", "", nil, fmt.Errorf("could not resolve OCI push repository key for %q", registryURL) } -func generateRepoCandidates(registry, repository string) []repoCandidate { - if repository == "" { - return []repoCandidate{{repoKey: extractRepositoryFromHostSubdomain(registry)}} - } - segments := strings.FieldsFunc(repository, func(r rune) bool { - return r == '/' - }) - if len(segments) == 0 { - return nil - } - candidates := []repoCandidate{{repoKey: segments[0], subpath: strings.Join(segments[1:], "/")}} - hostRepoKey := extractRepositoryFromHostSubdomain(registry) - if len(segments) > 1 && hostRepoKey != "" && hostRepoKey != segments[0] { - candidates = append(candidates, repoCandidate{repoKey: hostRepoKey, subpath: repository}) - } - return candidates -} - // searchPushedArtifacts searches for pushed OCI artifacts using a search pattern. func searchPushedArtifacts(serviceManager artifactory.ArtifactoryServicesManager, repoKey, storagePath string) (map[string]*servicesUtils.ResultItem, error) { aqlQuery := fmt.Sprintf(`{ diff --git a/artifactory/commands/helm/push_test.go b/artifactory/commands/helm/push_test.go index 9cffafd0..12c4496c 100644 --- a/artifactory/commands/helm/push_test.go +++ b/artifactory/commands/helm/push_test.go @@ -204,6 +204,21 @@ func TestResolveOCIPushArtifacts(t *testing.T) { expectedPath: chartStoragePath, expectedCalls: []pushSearchCall{{repo: "helm-repo", path: chartStoragePath}}, }, + { + name: "single-segment virtual host subpath falls back to host repo", + registryURL: "oci://helm-repo.art.com/team-a", + responses: map[pushSearchCall][]servicesUtils.ResultItem{ + {repo: "team-a", path: chartStoragePath}: nil, + {repo: "helm-repo", path: "team-a/" + chartStoragePath}: {newOCIArtifact("helm-repo", "team-a/"+chartStoragePath, "manifest.json", "manifest")}, + }, + expectedRepoKey: "helm-repo", + expectedSubpath: "team-a", + expectedPath: "team-a/" + chartStoragePath, + expectedCalls: []pushSearchCall{ + {repo: "team-a", path: chartStoragePath}, + {repo: "helm-repo", path: "team-a/" + chartStoragePath}, + }, + }, { name: "form 4 repo in path with extra subpath", registryURL: "oci://art.company.com/helm-repo/staging/libs", @@ -231,7 +246,7 @@ func TestResolveOCIPushArtifacts(t *testing.T) { }, }, { - name: "chooses real artifact when both multi-label host and first path segment are plausible", + name: "returns path-based match on plausible multi-label host", registryURL: "oci://helm-prod.company.example/team-a/charts", responses: map[pushSearchCall][]servicesUtils.ResultItem{ {repo: "team-a", path: "charts/" + chartStoragePath}: {newOCIArtifact("team-a", "charts/"+chartStoragePath, "manifest.json", "wrong")}, @@ -242,6 +257,19 @@ func TestResolveOCIPushArtifacts(t *testing.T) { expectedPath: "charts/" + chartStoragePath, expectedCalls: []pushSearchCall{{repo: "team-a", path: "charts/" + chartStoragePath}}, }, + { + name: "returns unresolved error when all candidates miss", + registryURL: "oci://helm-repo.art.com/team-a/charts", + responses: map[pushSearchCall][]servicesUtils.ResultItem{ + {repo: "team-a", path: "charts/" + chartStoragePath}: nil, + {repo: "helm-repo", path: "team-a/charts/" + chartStoragePath}: nil, + }, + expectedErrorText: "could not resolve OCI push repository key", + expectedCalls: []pushSearchCall{ + {repo: "team-a", path: "charts/" + chartStoragePath}, + {repo: "helm-repo", path: "team-a/charts/" + chartStoragePath}, + }, + }, { name: "tries host-based candidate without dash in repo key", registryURL: "oci://helmrepo.company.example/team-a/charts", @@ -258,7 +286,7 @@ func TestResolveOCIPushArtifacts(t *testing.T) { }, }, { - name: "returns error when no candidate resolves", + name: "returns unresolved error when generic multi-label host has no match", registryURL: "oci://art.company.com/helm-repo/team-a", responses: map[pushSearchCall][]servicesUtils.ResultItem{}, expectedErrorText: "could not resolve OCI push repository key", diff --git a/artifactory/commands/helm/repository.go b/artifactory/commands/helm/repository.go index b15455da..735c7216 100644 --- a/artifactory/commands/helm/repository.go +++ b/artifactory/commands/helm/repository.go @@ -54,6 +54,29 @@ func extractRepositoryFromHostSubdomain(host string) string { return "" } +type ociRepoCandidate struct { + repoKey string + subpath string +} + +func generateRepoCandidates(registry, repository string) []ociRepoCandidate { + if repository == "" { + return []ociRepoCandidate{{repoKey: extractRepositoryFromHostSubdomain(registry)}} + } + segments := strings.FieldsFunc(repository, func(r rune) bool { + return r == '/' + }) + if len(segments) == 0 { + return nil + } + candidates := []ociRepoCandidate{{repoKey: segments[0], subpath: strings.Join(segments[1:], "/")}} + hostRepoKey := extractRepositoryFromHostSubdomain(registry) + if hostRepoKey != "" && hostRepoKey != segments[0] { + candidates = append(candidates, ociRepoCandidate{repoKey: hostRepoKey, subpath: repository}) + } + return candidates +} + // removeProtocolPrefix removes protocol prefix from URL func removeProtocolPrefix(repository string) string { prefixes := []string{oci, schemeHttp + "://", schemeSecure + "://"} diff --git a/artifactory/commands/helm/repository_test.go b/artifactory/commands/helm/repository_test.go index af7764ca..4ad57aeb 100644 --- a/artifactory/commands/helm/repository_test.go +++ b/artifactory/commands/helm/repository_test.go @@ -194,6 +194,61 @@ func TestIsOCIRepository(t *testing.T) { } } +func TestGenerateOCIRepoCandidates(t *testing.T) { + tests := []struct { + name string + registry string + repository string + expected []ociRepoCandidate + }{ + { + name: "host-only registry falls back to host repo", + registry: "helm-repo.company.example", + repository: "", + expected: []ociRepoCandidate{{repoKey: "helm-repo"}}, + }, + { + name: "generic multi-label host still yields distinct fallback candidate", + registry: "art.company.example", + repository: "helm-repo/staging/libs", + expected: []ociRepoCandidate{ + {repoKey: "helm-repo", subpath: "staging/libs"}, + {repoKey: "art", subpath: "helm-repo/staging/libs"}, + }, + }, + { + name: "virtual host adds host-based fallback candidate", + registry: "helm-repo.company.example", + repository: "team-a/charts", + expected: []ociRepoCandidate{ + {repoKey: "team-a", subpath: "charts"}, + {repoKey: "helm-repo", subpath: "team-a/charts"}, + }, + }, + { + name: "single-segment virtual host subpath adds host-based fallback candidate", + registry: "helm-repo.art.com", + repository: "team-a", + expected: []ociRepoCandidate{ + {repoKey: "team-a", subpath: ""}, + {repoKey: "helm-repo", subpath: "team-a"}, + }, + }, + { + name: "single-segment path repo does not add duplicate host fallback", + registry: "helm-repo.company.example", + repository: "helm-repo", + expected: []ociRepoCandidate{{repoKey: "helm-repo", subpath: ""}}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.expected, generateRepoCandidates(tt.registry, tt.repository)) + }) + } +} + // TestRemoveProtocolPrefix tests the removeProtocolPrefix function func TestRemoveProtocolPrefix(t *testing.T) { tests := []struct { From cb387b2140625e68a252187e47c4ed683fff9e71 Mon Sep 17 00:00:00 2001 From: vjda Date: Tue, 21 Apr 2026 08:43:41 +0200 Subject: [PATCH 3/6] fix(helm): clarify OCI path resolution follow-up Tighten the small follow-up changes requested in PR #425 after the main OCI path fixes were already in place. This commit does not change the core resolution strategy. Instead it improves the behavior and reviewability of the existing hotfix by: - preserving the original manifest retrieval error with `%w` - returning a clearer host-only miss error when the repo key is known but no OCI artifacts are found at the resolved storage path - including the OCI parse error in dependency-side logging - cleaning up a stale comment and a minor test formatting issue The intent is to address the review feedback without widening scope or mixing in unrelated local changes. --- artifactory/commands/helm/layers.go | 2 +- artifactory/commands/helm/push.go | 6 +++--- artifactory/commands/helm/push_test.go | 1 - 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/artifactory/commands/helm/layers.go b/artifactory/commands/helm/layers.go index acd33645..5bcbf92a 100644 --- a/artifactory/commands/helm/layers.go +++ b/artifactory/commands/helm/layers.go @@ -78,7 +78,7 @@ func addOCILayersForDependency(dep entities.Dependency, serviceManager artifacto } else { ref, parseErr := parseOCIReference(registryReference) if parseErr != nil { - log.Error("Failed to find a valid repository for dependency: ", dep.Id) + log.Error("Failed to find a valid repository for dependency: ", dep.Id, " : ", parseErr) return } candidates = generateRepoCandidates(ref.Registry, ref.Repository) diff --git a/artifactory/commands/helm/push.go b/artifactory/commands/helm/push.go index 518223b2..843ca3b4 100644 --- a/artifactory/commands/helm/push.go +++ b/artifactory/commands/helm/push.go @@ -47,7 +47,7 @@ func handlePushCommand(buildInfo *entities.BuildInfo, helmArgs []string, service } artifactManifest, err := getManifest(resultMap, serviceManager, repoKey) if err != nil { - return fmt.Errorf("failed to get manifest") + return fmt.Errorf("failed to get manifest: %w", err) } if artifactManifest == nil { return fmt.Errorf("could not find image manifest in Artifactory") @@ -85,7 +85,7 @@ func resolveOCIPushArtifacts(registryURL, chartName, chartVersion string, sm art return "", "", "", nil, fmt.Errorf("failed to search oci layers for %s : %s: %w", chartName, chartVersion, err) } if len(resultMap) == 0 { - return "", "", "", nil, fmt.Errorf("could not resolve OCI push repository key for %q", registryURL) + return "", "", "", nil, fmt.Errorf("no oci artifacts found for repoKey %q and storagePath %q", repoKey, storagePath) } return repoKey, "", storagePath, resultMap, nil } @@ -144,7 +144,7 @@ func searchPushedArtifacts(serviceManager artifactory.ArtifactoryServicesManager return artifacts, nil } -// updateReaderContents updates the reader contents by writing the specified JSON value to all file paths +// overwriteReaderWithManifestFolder overwrites the reader's backing files with JSON describing the manifest folder result. func overwriteReaderWithManifestFolder(reader *content.ContentReader, repoKey, manifestFolderPath, manifestFolderName string) error { if reader == nil { return fmt.Errorf("reader is nil") diff --git a/artifactory/commands/helm/push_test.go b/artifactory/commands/helm/push_test.go index 12c4496c..b9d3c100 100644 --- a/artifactory/commands/helm/push_test.go +++ b/artifactory/commands/helm/push_test.go @@ -103,7 +103,6 @@ func newOCIArtifact(repo, storagePath, name, sha256 string) servicesUtils.Result Type: "file", Sha256: sha256, } - } func createChartArchive(t *testing.T, chartName, chartVersion string) string { From 9d2eb825630529d5dcdd55a31871336d08653316 Mon Sep 17 00:00:00 2001 From: vjda Date: Thu, 23 Apr 2026 09:40:00 +0200 Subject: [PATCH 4/6] fix(helm): document and trace OCI dependency resolution Add a small follow-up to the OCI dependency subpath work. This commit documents `generateRepoCandidates` with a proper docstring and adds a debug log when the dependency flow resolves the winning OCI repo/subpath candidate. The intent is to improve maintainability and make dependency-side path resolution easier to inspect during review and troubleshooting, without changing the existing functional behavior. --- artifactory/commands/helm/layers.go | 1 + artifactory/commands/helm/repository.go | 7 +++++++ 2 files changed, 8 insertions(+) diff --git a/artifactory/commands/helm/layers.go b/artifactory/commands/helm/layers.go index 5bcbf92a..07b21331 100644 --- a/artifactory/commands/helm/layers.go +++ b/artifactory/commands/helm/layers.go @@ -105,6 +105,7 @@ func addOCILayersForDependency(dep entities.Dependency, serviceManager artifacto continue } repoName = candidate.repoKey + log.Debug("Resolved OCI dependency ", dep.Id, " to repo: ", candidate.repoKey, ", subpath: ", candidate.subpath) break } if len(resultMap) == 0 { diff --git a/artifactory/commands/helm/repository.go b/artifactory/commands/helm/repository.go index 735c7216..b5bdab58 100644 --- a/artifactory/commands/helm/repository.go +++ b/artifactory/commands/helm/repository.go @@ -59,6 +59,13 @@ type ociRepoCandidate struct { subpath string } +// generateRepoCandidates generates plausible Artifactory repo key + subpath +// combinations for an OCI reference. For path-based URLs, it attempts: +// 1. First path segment as repo key (e.g., "team-a" for "team-a/charts") +// 2. Host-derived repo key if different (e.g., "helm-repo" for "helm-repo.art.com") +// +// Candidates are validated by searching for actual OCI artifacts at each location, +// ensuring correctness without relying solely on URL structure heuristics. func generateRepoCandidates(registry, repository string) []ociRepoCandidate { if repository == "" { return []ociRepoCandidate{{repoKey: extractRepositoryFromHostSubdomain(registry)}} From 06c2e59d75aebae69a70416274a9718e87634966 Mon Sep 17 00:00:00 2001 From: vjda Date: Thu, 23 Apr 2026 10:10:29 +0200 Subject: [PATCH 5/6] fix(helm): propagate build property failures Propagate SetProps failures in the OCI push build-properties flow instead of silently ignoring them. This tightens the post-push build-info path without changing the broader OCI resolution logic. The previous helper swallowed serviceManager.SetProps errors, so the command could continue as if build properties had been applied successfully. Return the lower-level SetProps error through addBuildPropertiesOnArtifacts and applyBuildPropertiesOnManifestFolder, then let handlePushCommand surface it with the existing contextual wrapper. Add focused tests for both the helper-level failure path and the end-to-end push command behavior when property application fails. --- artifactory/commands/helm/push.go | 11 ++++--- artifactory/commands/helm/push_test.go | 42 ++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 4 deletions(-) diff --git a/artifactory/commands/helm/push.go b/artifactory/commands/helm/push.go index 843ca3b4..720b3be1 100644 --- a/artifactory/commands/helm/push.go +++ b/artifactory/commands/helm/push.go @@ -220,15 +220,18 @@ func applyBuildPropertiesOnManifestFolder(serviceManager artifactory.Artifactory err = cleanupErr } }() - addBuildPropertiesOnArtifacts(serviceManager, reader, buildProps) - return nil + return addBuildPropertiesOnArtifacts(serviceManager, reader, buildProps) } -func addBuildPropertiesOnArtifacts(serviceManager artifactory.ArtifactoryServicesManager, reader *content.ContentReader, buildProps string) { +func addBuildPropertiesOnArtifacts(serviceManager artifactory.ArtifactoryServicesManager, reader *content.ContentReader, buildProps string) error { propsParams := services.PropsParams{ Reader: reader, Props: buildProps, IsRecursive: true, } - _, _ = serviceManager.SetProps(propsParams) + _, err := serviceManager.SetProps(propsParams) + if err != nil { + return fmt.Errorf("failed to set build properties on artifacts: %w", err) + } + return nil } diff --git a/artifactory/commands/helm/push_test.go b/artifactory/commands/helm/push_test.go index b9d3c100..c4388083 100644 --- a/artifactory/commands/helm/push_test.go +++ b/artifactory/commands/helm/push_test.go @@ -36,6 +36,7 @@ type pushTestServiceManager struct { searchResults map[pushSearchCall][]servicesUtils.ResultItem searchCalls []pushSearchCall propsCalls []pushPropsCall + propsErr error remoteContents map[string]string } @@ -59,6 +60,9 @@ func (m *pushTestServiceManager) SetProps(params services.PropsParams) (int, err item := new(servicesUtils.ResultItem) require.NoError(m.t, params.Reader.NextRecord(item)) m.propsCalls = append(m.propsCalls, pushPropsCall{props: params.Props, item: *item}) + if m.propsErr != nil { + return 0, m.propsErr + } return 1, nil } @@ -346,6 +350,20 @@ func TestNewManifestFolderReader(t *testing.T) { assert.Equal(t, "folder", item.Type) } +func TestApplyBuildPropertiesOnManifestFolderReturnsSetPropsError(t *testing.T) { + serviceManager := newPushTestServiceManager(t) + serviceManager.propsErr = fmt.Errorf("set props failed") + + err := applyBuildPropertiesOnManifestFolder(serviceManager, "helm-repo", "team-a/charts/chart", "0.1.0", "build.name=test") + require.Error(t, err) + assert.ErrorContains(t, err, "failed to set build properties on artifacts") + assert.ErrorContains(t, err, "set props failed") + require.Len(t, serviceManager.propsCalls, 1) + assert.Equal(t, "helm-repo", serviceManager.propsCalls[0].item.Repo) + assert.Equal(t, "team-a/charts/chart", serviceManager.propsCalls[0].item.Path) + assert.Equal(t, "0.1.0", serviceManager.propsCalls[0].item.Name) +} + func TestOverwriteReaderWithManifestFolderUsesRealArtifactoryPath(t *testing.T) { reader := newPushSearchReader(t, []servicesUtils.ResultItem{ newOCIArtifact("old-repo", "ignored", "manifest.json", "manifest"), @@ -470,3 +488,27 @@ func TestHandlePushCommandResolvesOCIPaths(t *testing.T) { }) } } + +func TestHandlePushCommandReturnsBuildPropsError(t *testing.T) { + t.Setenv("JFROG_CLI_HOME_DIR", t.TempDir()) + serviceManager := newPushTestServiceManager(t) + serviceManager.propsErr = fmt.Errorf("set props failed") + serviceManager.searchResults[pushSearchCall{repo: "helm-repo", path: "chart/0.1.0"}] = []servicesUtils.ResultItem{ + newOCIArtifact("helm-repo", "chart/0.1.0", "manifest.json", "manifest-sha"), + newOCIArtifact("helm-repo", "chart/0.1.0", "sha256__config", "config-sha"), + newOCIArtifact("helm-repo", "chart/0.1.0", "sha256__layer", "layer-sha"), + } + chartPath := createChartArchive(t, "chart", "0.1.0") + buildInfo := &entities.BuildInfo{ + Modules: []entities.Module{{Id: "chart:0.1.0", Type: "helm"}}, + BuildAgent: &entities.Agent{Name: "Helm", Version: "test"}, + } + + err := handlePushCommand(buildInfo, []string{chartPath, "oci://helm-repo.art.com"}, serviceManager, "build-name", "42", "proj") + require.Error(t, err) + assert.ErrorContains(t, err, "failed to apply build properties on OCI manifest folder") + assert.ErrorContains(t, err, "failed to set build properties on artifacts") + assert.ErrorContains(t, err, "set props failed") + require.Len(t, serviceManager.propsCalls, 1) + assert.Empty(t, buildInfo.Modules[0].Artifacts) +} From be28c05a58bbe11ae3c5455a5946c88a9f3cc18a Mon Sep 17 00:00:00 2001 From: vjda Date: Thu, 23 Apr 2026 13:21:03 +0200 Subject: [PATCH 6/6] fix(helm): prefer host-based OCI candidates first Prefer the host-derived OCI candidate before the path-based fallback when resolving OCI repository layouts that can be addressed in more than one valid way. This does not claim that host-first is universally more correct. For ambiguous layouts, the candidate order is effectively a policy choice because the current flow stops at the first successful artifact lookup. This change makes that policy explicit. The practical goal here is to align the heuristic with the more common Artifactory host-based OCI layout while still preserving the same fallback behavior when the host-derived candidate does not resolve. The accompanying test updates do two things: - rename cases so they describe the new host-first policy precisely - adjust expected search order and fallback behavior accordingly The result is a clearer contract between the helper, `resolveOCIPushArtifacts`, and the OCI dependency flow, with tests that now document the intended order instead of the previous path-first assumption. --- artifactory/commands/helm/layers_test.go | 16 ++-- artifactory/commands/helm/push_test.go | 83 +++++++++++--------- artifactory/commands/helm/repository.go | 7 +- artifactory/commands/helm/repository_test.go | 12 +-- 4 files changed, 64 insertions(+), 54 deletions(-) diff --git a/artifactory/commands/helm/layers_test.go b/artifactory/commands/helm/layers_test.go index 5539e669..0d941d63 100644 --- a/artifactory/commands/helm/layers_test.go +++ b/artifactory/commands/helm/layers_test.go @@ -111,21 +111,20 @@ func TestAddOCILayersForDependency(t *testing.T) { manifestPath string }{ { - name: "single-segment virtual host subpath resolves using host fallback", + name: "single-segment virtual host subpath resolves on first host-based candidate", dependency: entities.Dependency{ Id: chartName + ":" + chartVersion, Repository: "oci://helm-repo.art.com/team-a", }, responses: map[pushSearchCall][]servicesUtils.ResultItem{ - {repo: "team-a", path: "chart/0.1.0"}: nil, {repo: "helm-repo", path: "team-a/chart/0.1.0"}: { newOCIArtifact("helm-repo", "team-a/chart/0.1.0", "manifest.json", "manifest-sha"), newOCIArtifact("helm-repo", "team-a/chart/0.1.0", "sha256__config", "config-sha"), newOCIArtifact("helm-repo", "team-a/chart/0.1.0", "sha256__layer", "layer-sha"), }, + {repo: "team-a", path: "chart/0.1.0"}: nil, }, expectedSearchCalls: []pushSearchCall{ - {repo: "team-a", path: "chart/0.1.0"}, {repo: "helm-repo", path: "team-a/chart/0.1.0"}, }, expectedDependencies: []entities.Dependency{ @@ -137,21 +136,20 @@ func TestAddOCILayersForDependency(t *testing.T) { manifestPath: "team-a/chart/0.1.0", }, { - name: "oci dependency with non-root subpath resolves using validated candidate", + name: "oci dependency with non-root subpath resolves on first host-based candidate", dependency: entities.Dependency{ Id: chartName + ":" + chartVersion, Repository: "oci://helm-repo.art.com/team-a/charts", }, responses: map[pushSearchCall][]servicesUtils.ResultItem{ - {repo: "team-a", path: "charts/chart/0.1.0"}: nil, {repo: "helm-repo", path: "team-a/charts/chart/0.1.0"}: { newOCIArtifact("helm-repo", "team-a/charts/chart/0.1.0", "manifest.json", "manifest-sha"), newOCIArtifact("helm-repo", "team-a/charts/chart/0.1.0", "sha256__config", "config-sha"), newOCIArtifact("helm-repo", "team-a/charts/chart/0.1.0", "sha256__layer", "layer-sha"), }, + {repo: "team-a", path: "charts/chart/0.1.0"}: nil, }, expectedSearchCalls: []pushSearchCall{ - {repo: "team-a", path: "charts/chart/0.1.0"}, {repo: "helm-repo", path: "team-a/charts/chart/0.1.0"}, }, expectedDependencies: []entities.Dependency{ @@ -185,18 +183,18 @@ func TestAddOCILayersForDependency(t *testing.T) { manifestPath: "chart/0.1.0", }, { - name: "oci dependency without matching candidate returns without adding layers", + name: "oci dependency without matching host-first or path fallback returns without adding layers", dependency: entities.Dependency{ Id: chartName + ":" + chartVersion, Repository: "oci://helm-repo.art.com/team-a/charts", }, responses: map[pushSearchCall][]servicesUtils.ResultItem{ - {repo: "team-a", path: "charts/chart/0.1.0"}: nil, {repo: "helm-repo", path: "team-a/charts/chart/0.1.0"}: nil, + {repo: "team-a", path: "charts/chart/0.1.0"}: nil, }, expectedSearchCalls: []pushSearchCall{ - {repo: "team-a", path: "charts/chart/0.1.0"}, {repo: "helm-repo", path: "team-a/charts/chart/0.1.0"}, + {repo: "team-a", path: "charts/chart/0.1.0"}, }, }, } diff --git a/artifactory/commands/helm/push_test.go b/artifactory/commands/helm/push_test.go index c4388083..6ce4c1ec 100644 --- a/artifactory/commands/helm/push_test.go +++ b/artifactory/commands/helm/push_test.go @@ -171,17 +171,16 @@ func TestResolveOCIPushArtifacts(t *testing.T) { expectedCalls: []pushSearchCall{{repo: "helm-repo", path: chartStoragePath}}, }, { - name: "form 2 virtual host with subpath", + name: "form 2 virtual host with subpath resolves on first host-based candidate", registryURL: "oci://helm-repo.art.com/team-a/charts", responses: map[pushSearchCall][]servicesUtils.ResultItem{ - {repo: "team-a", path: "charts/" + chartStoragePath}: nil, {repo: "helm-repo", path: "team-a/charts/" + chartStoragePath}: {newOCIArtifact("helm-repo", "team-a/charts/"+chartStoragePath, "manifest.json", "manifest")}, + {repo: "team-a", path: "charts/" + chartStoragePath}: nil, }, expectedRepoKey: "helm-repo", expectedSubpath: "team-a/charts", expectedPath: "team-a/charts/" + chartStoragePath, expectedCalls: []pushSearchCall{ - {repo: "team-a", path: "charts/" + chartStoragePath}, {repo: "helm-repo", path: "team-a/charts/" + chartStoragePath}, }, }, @@ -197,105 +196,110 @@ func TestResolveOCIPushArtifacts(t *testing.T) { expectedCalls: []pushSearchCall{{repo: "helm-repo", path: chartStoragePath}}, }, { - name: "form 3 repo in path without extra subpath", + name: "form 3 repo in path without extra subpath falls back after host-first miss", registryURL: "oci://art.company.com/helm-repo", responses: map[pushSearchCall][]servicesUtils.ResultItem{ - {repo: "helm-repo", path: chartStoragePath}: {newOCIArtifact("helm-repo", chartStoragePath, "manifest.json", "manifest")}, + {repo: "art", path: "helm-repo/" + chartStoragePath}: nil, + {repo: "helm-repo", path: chartStoragePath}: {newOCIArtifact("helm-repo", chartStoragePath, "manifest.json", "manifest")}, }, expectedRepoKey: "helm-repo", expectedSubpath: "", expectedPath: chartStoragePath, - expectedCalls: []pushSearchCall{{repo: "helm-repo", path: chartStoragePath}}, + expectedCalls: []pushSearchCall{ + {repo: "art", path: "helm-repo/" + chartStoragePath}, + {repo: "helm-repo", path: chartStoragePath}, + }, }, { - name: "single-segment virtual host subpath falls back to host repo", + name: "single-segment virtual host subpath resolves on first host-based candidate", registryURL: "oci://helm-repo.art.com/team-a", responses: map[pushSearchCall][]servicesUtils.ResultItem{ - {repo: "team-a", path: chartStoragePath}: nil, {repo: "helm-repo", path: "team-a/" + chartStoragePath}: {newOCIArtifact("helm-repo", "team-a/"+chartStoragePath, "manifest.json", "manifest")}, + {repo: "team-a", path: chartStoragePath}: nil, }, expectedRepoKey: "helm-repo", expectedSubpath: "team-a", expectedPath: "team-a/" + chartStoragePath, expectedCalls: []pushSearchCall{ - {repo: "team-a", path: chartStoragePath}, {repo: "helm-repo", path: "team-a/" + chartStoragePath}, }, }, { - name: "form 4 repo in path with extra subpath", + name: "form 4 repo in path with extra subpath falls back after host-first miss", registryURL: "oci://art.company.com/helm-repo/staging/libs", responses: map[pushSearchCall][]servicesUtils.ResultItem{ - {repo: "helm-repo", path: "staging/libs/" + chartStoragePath}: {newOCIArtifact("helm-repo", "staging/libs/"+chartStoragePath, "manifest.json", "manifest")}, + {repo: "art", path: "helm-repo/staging/libs/" + chartStoragePath}: nil, + {repo: "helm-repo", path: "staging/libs/" + chartStoragePath}: {newOCIArtifact("helm-repo", "staging/libs/"+chartStoragePath, "manifest.json", "manifest")}, }, expectedRepoKey: "helm-repo", expectedSubpath: "staging/libs", expectedPath: "staging/libs/" + chartStoragePath, - expectedCalls: []pushSearchCall{{repo: "helm-repo", path: "staging/libs/" + chartStoragePath}}, + expectedCalls: []pushSearchCall{ + {repo: "art", path: "helm-repo/staging/libs/" + chartStoragePath}, + {repo: "helm-repo", path: "staging/libs/" + chartStoragePath}, + }, }, { - name: "tries next candidate when plausible path-first candidate has no artifact", + name: "tries path fallback when host-first candidate has no artifact", registryURL: "oci://helm-repo.art.com/folder/subfolder", responses: map[pushSearchCall][]servicesUtils.ResultItem{ - {repo: "folder", path: "subfolder/" + chartStoragePath}: nil, {repo: "helm-repo", path: "folder/subfolder/" + chartStoragePath}: {newOCIArtifact("helm-repo", "folder/subfolder/"+chartStoragePath, "manifest.json", "manifest")}, + {repo: "folder", path: "subfolder/" + chartStoragePath}: nil, }, expectedRepoKey: "helm-repo", expectedSubpath: "folder/subfolder", expectedPath: "folder/subfolder/" + chartStoragePath, expectedCalls: []pushSearchCall{ - {repo: "folder", path: "subfolder/" + chartStoragePath}, {repo: "helm-repo", path: "folder/subfolder/" + chartStoragePath}, }, }, { - name: "returns path-based match on plausible multi-label host", + name: "returns host-based match first on plausible multi-label host", registryURL: "oci://helm-prod.company.example/team-a/charts", responses: map[pushSearchCall][]servicesUtils.ResultItem{ + {repo: "helm-prod", path: "team-a/charts/" + chartStoragePath}: {newOCIArtifact("helm-prod", "team-a/charts/"+chartStoragePath, "manifest.json", "manifest")}, {repo: "team-a", path: "charts/" + chartStoragePath}: {newOCIArtifact("team-a", "charts/"+chartStoragePath, "manifest.json", "wrong")}, - {repo: "helm-prod", path: "team-a/charts/" + chartStoragePath}: nil, }, - expectedRepoKey: "team-a", - expectedSubpath: "charts", - expectedPath: "charts/" + chartStoragePath, - expectedCalls: []pushSearchCall{{repo: "team-a", path: "charts/" + chartStoragePath}}, + expectedRepoKey: "helm-prod", + expectedSubpath: "team-a/charts", + expectedPath: "team-a/charts/" + chartStoragePath, + expectedCalls: []pushSearchCall{{repo: "helm-prod", path: "team-a/charts/" + chartStoragePath}}, }, { - name: "returns unresolved error when all candidates miss", + name: "returns unresolved error when both host-first and path fallback miss", registryURL: "oci://helm-repo.art.com/team-a/charts", responses: map[pushSearchCall][]servicesUtils.ResultItem{ - {repo: "team-a", path: "charts/" + chartStoragePath}: nil, {repo: "helm-repo", path: "team-a/charts/" + chartStoragePath}: nil, + {repo: "team-a", path: "charts/" + chartStoragePath}: nil, }, expectedErrorText: "could not resolve OCI push repository key", expectedCalls: []pushSearchCall{ - {repo: "team-a", path: "charts/" + chartStoragePath}, {repo: "helm-repo", path: "team-a/charts/" + chartStoragePath}, + {repo: "team-a", path: "charts/" + chartStoragePath}, }, }, { - name: "tries host-based candidate without dash in repo key", + name: "tries host-based candidate without dash in repo key first", registryURL: "oci://helmrepo.company.example/team-a/charts", responses: map[pushSearchCall][]servicesUtils.ResultItem{ - {repo: "team-a", path: "charts/" + chartStoragePath}: nil, {repo: "helmrepo", path: "team-a/charts/" + chartStoragePath}: {newOCIArtifact("helmrepo", "team-a/charts/"+chartStoragePath, "manifest.json", "manifest")}, + {repo: "team-a", path: "charts/" + chartStoragePath}: nil, }, expectedRepoKey: "helmrepo", expectedSubpath: "team-a/charts", expectedPath: "team-a/charts/" + chartStoragePath, expectedCalls: []pushSearchCall{ - {repo: "team-a", path: "charts/" + chartStoragePath}, {repo: "helmrepo", path: "team-a/charts/" + chartStoragePath}, }, }, { - name: "returns unresolved error when generic multi-label host has no match", + name: "returns unresolved error when neither host-first nor path fallback matches", registryURL: "oci://art.company.com/helm-repo/team-a", responses: map[pushSearchCall][]servicesUtils.ResultItem{}, expectedErrorText: "could not resolve OCI push repository key", expectedCalls: []pushSearchCall{ - {repo: "helm-repo", path: "team-a/" + chartStoragePath}, {repo: "art", path: "helm-repo/team-a/" + chartStoragePath}, + {repo: "helm-repo", path: "team-a/" + chartStoragePath}, }, }, } @@ -407,18 +411,17 @@ func TestHandlePushCommandResolvesOCIPaths(t *testing.T) { expectedPropsName: "0.1.0", }, { - name: "path-based push uses real manifest folder with subpath", + name: "host-first push keeps real manifest folder when the host candidate resolves", registryURL: "oci://helm-repo.art.com/team-a/charts", responses: map[pushSearchCall][]servicesUtils.ResultItem{ - {repo: "team-a", path: "charts/chart/0.1.0"}: nil, {repo: "helm-repo", path: "team-a/charts/chart/0.1.0"}: { newOCIArtifact("helm-repo", "team-a/charts/chart/0.1.0", "manifest.json", "manifest-sha"), newOCIArtifact("helm-repo", "team-a/charts/chart/0.1.0", "sha256__config", "config-sha"), newOCIArtifact("helm-repo", "team-a/charts/chart/0.1.0", "sha256__layer", "layer-sha"), }, + {repo: "team-a", path: "charts/chart/0.1.0"}: nil, }, expectedSearches: []pushSearchCall{ - {repo: "team-a", path: "charts/chart/0.1.0"}, {repo: "helm-repo", path: "team-a/charts/chart/0.1.0"}, }, expectedManifest: "helm-repo/team-a/charts/chart/0.1.0/manifest.json", @@ -427,32 +430,40 @@ func TestHandlePushCommandResolvesOCIPaths(t *testing.T) { expectedPropsName: "0.1.0", }, { - name: "form 3 path-based repo without extra subpath keeps chart root paths", + name: "form 3 path-based repo keeps chart root paths after host-first miss", registryURL: "oci://art.company.com/helm-repo", responses: map[pushSearchCall][]servicesUtils.ResultItem{ + {repo: "art", path: "helm-repo/chart/0.1.0"}: nil, {repo: "helm-repo", path: "chart/0.1.0"}: { newOCIArtifact("helm-repo", "chart/0.1.0", "manifest.json", "manifest-sha"), newOCIArtifact("helm-repo", "chart/0.1.0", "sha256__config", "config-sha"), newOCIArtifact("helm-repo", "chart/0.1.0", "sha256__layer", "layer-sha"), }, }, - expectedSearches: []pushSearchCall{{repo: "helm-repo", path: "chart/0.1.0"}}, + expectedSearches: []pushSearchCall{ + {repo: "art", path: "helm-repo/chart/0.1.0"}, + {repo: "helm-repo", path: "chart/0.1.0"}, + }, expectedManifest: "helm-repo/chart/0.1.0/manifest.json", expectedPropsRepo: "helm-repo", expectedPropsPath: "chart", expectedPropsName: "0.1.0", }, { - name: "form 4 path-based repo with extra subpath uses resolved manifest folder", + name: "form 4 path-based repo uses resolved manifest folder after host-first miss", registryURL: "oci://art.company.com/helm-repo/staging/libs", responses: map[pushSearchCall][]servicesUtils.ResultItem{ + {repo: "art", path: "helm-repo/staging/libs/chart/0.1.0"}: nil, {repo: "helm-repo", path: "staging/libs/chart/0.1.0"}: { newOCIArtifact("helm-repo", "staging/libs/chart/0.1.0", "manifest.json", "manifest-sha"), newOCIArtifact("helm-repo", "staging/libs/chart/0.1.0", "sha256__config", "config-sha"), newOCIArtifact("helm-repo", "staging/libs/chart/0.1.0", "sha256__layer", "layer-sha"), }, }, - expectedSearches: []pushSearchCall{{repo: "helm-repo", path: "staging/libs/chart/0.1.0"}}, + expectedSearches: []pushSearchCall{ + {repo: "art", path: "helm-repo/staging/libs/chart/0.1.0"}, + {repo: "helm-repo", path: "staging/libs/chart/0.1.0"}, + }, expectedManifest: "helm-repo/staging/libs/chart/0.1.0/manifest.json", expectedPropsRepo: "helm-repo", expectedPropsPath: "staging/libs/chart", diff --git a/artifactory/commands/helm/repository.go b/artifactory/commands/helm/repository.go index b5bdab58..bf4c74de 100644 --- a/artifactory/commands/helm/repository.go +++ b/artifactory/commands/helm/repository.go @@ -61,8 +61,8 @@ type ociRepoCandidate struct { // generateRepoCandidates generates plausible Artifactory repo key + subpath // combinations for an OCI reference. For path-based URLs, it attempts: -// 1. First path segment as repo key (e.g., "team-a" for "team-a/charts") -// 2. Host-derived repo key if different (e.g., "helm-repo" for "helm-repo.art.com") +// 1. Host-derived repo key if different (e.g., "helm-repo" for "helm-repo.art.com") +// 2. First path segment as repo key (e.g., "team-a" for "team-a/charts") // // Candidates are validated by searching for actual OCI artifacts at each location, // ensuring correctness without relying solely on URL structure heuristics. @@ -76,11 +76,12 @@ func generateRepoCandidates(registry, repository string) []ociRepoCandidate { if len(segments) == 0 { return nil } - candidates := []ociRepoCandidate{{repoKey: segments[0], subpath: strings.Join(segments[1:], "/")}} + var candidates []ociRepoCandidate hostRepoKey := extractRepositoryFromHostSubdomain(registry) if hostRepoKey != "" && hostRepoKey != segments[0] { candidates = append(candidates, ociRepoCandidate{repoKey: hostRepoKey, subpath: repository}) } + candidates = append(candidates, ociRepoCandidate{repoKey: segments[0], subpath: strings.Join(segments[1:], "/")}) return candidates } diff --git a/artifactory/commands/helm/repository_test.go b/artifactory/commands/helm/repository_test.go index 4ad57aeb..f2a68228 100644 --- a/artifactory/commands/helm/repository_test.go +++ b/artifactory/commands/helm/repository_test.go @@ -208,30 +208,30 @@ func TestGenerateOCIRepoCandidates(t *testing.T) { expected: []ociRepoCandidate{{repoKey: "helm-repo"}}, }, { - name: "generic multi-label host still yields distinct fallback candidate", + name: "generic multi-label host yields host-first and path fallback candidates", registry: "art.company.example", repository: "helm-repo/staging/libs", expected: []ociRepoCandidate{ - {repoKey: "helm-repo", subpath: "staging/libs"}, {repoKey: "art", subpath: "helm-repo/staging/libs"}, + {repoKey: "helm-repo", subpath: "staging/libs"}, }, }, { - name: "virtual host adds host-based fallback candidate", + name: "virtual host prefers host-based candidate before path fallback", registry: "helm-repo.company.example", repository: "team-a/charts", expected: []ociRepoCandidate{ - {repoKey: "team-a", subpath: "charts"}, {repoKey: "helm-repo", subpath: "team-a/charts"}, + {repoKey: "team-a", subpath: "charts"}, }, }, { - name: "single-segment virtual host subpath adds host-based fallback candidate", + name: "single-segment virtual host subpath prefers host-based candidate before path fallback", registry: "helm-repo.art.com", repository: "team-a", expected: []ociRepoCandidate{ - {repoKey: "team-a", subpath: ""}, {repoKey: "helm-repo", subpath: "team-a"}, + {repoKey: "team-a", subpath: ""}, }, }, {