Feat/oci helm support#500
Conversation
Fixes zapier#337 (zapier/kubechecks) ## Problem kubechecks tried to git-clone every source URL, including OCI registries (oci://...) and HTTPS Helm repos (https://charts.example.io). These are not git repos and the clone fails. Additionally, git sources that use tag-based or commit-SHA targetRevision also failed because go-git was only trying branch refs. ## Changes ### pkg/argo_client/manifests.go - Add helper that returns true when: - (it's a Helm chart source, not a git path), AND - RepoURL starts with (OCI registry), OR - RepoURL is an HTTPS URL without a .git suffix and source.Path is empty (HTTPS Helm repo) - Modify to skip for external Helm chart sources: - When : create an empty temp dir and use it as . ArgoCD's repo-server fetches the chart from the registry itself using its own credentials and OCI support. - When : log a warning and return nil (graceful degradation — skip manifest generation for that source). ### pkg/git/repo.go - Add helper to detect go-git "reference not found" errors (handles both plumbing.ErrReferenceNotFound and string match). - Add helper to detect commit SHAs (hex strings ≥7 chars). - Modify to fall back when branch clone fails with ref-not-found: 1. Retry as a tag reference () 2. If still not found AND name looks like a hex SHA: clone default branch then checkout the specific commit hash. ### Tests added - : OCI, HTTPS Helm, git-with-.git, no Chart field, HTTPS with path set - : full SHA, short SHA, too-short, branch name, tag name, mixed-case hex, non-hex chars - : nil, plumbing sentinel, generic error, string matches
- Triggers on push to main/feat branches and on PRs - Builds Docker image using existing Dockerfile (target: production) - Pushes to ghcr.io/christmas-island/kubechecks only on main push - PRs: build only (no push) to verify compilation - Tags: 'latest' on main, short SHA on all builds - Multi-arch build: linux/amd64 + linux/arm64 - Separate job for Go tests (make test) - Uses GHA cache for faster builds
Not needed for this fork — upstream handles CI.
ArgoCD's repo-server expects a Chart.yaml in the temp dir even for external Helm charts. Without it, GenerateManifestWithFiles fails with 'error reading helm chart from ... Chart.yaml: no such file'. Write a minimal Chart.yaml stub (apiVersion, name, version, type) so the streaming protocol succeeds and repo-server can fetch the actual chart from the registry.
Instead of streaming a stub Chart.yaml via GenerateManifestWithFiles, delegate directly to repoClient.GenerateManifest() for external Helm chart sources (OCI registries and HTTPS Helm repos). This is the correct approach because: - GenerateManifestWithFiles requires local chart files. Streaming a stub causes 'helm template' to run against the stub dir, not the actual chart — producing incorrect or missing manifests. - GenerateManifest tells repo-server to fetch the chart from the registry using its own credentials and OCI/Helm support, exactly as ArgoCD does during a normal sync. Known limitations (acknowledged): - Chart dependencies referencing private repos require those repos to be configured in ArgoCD with appropriate credentials. - Registry auth failures will surface as manifest generation errors (same behavior as ArgoCD itself). - TargetRevision must be a valid chart version (not a branch name) for Helm chart sources — consistent with ArgoCD's requirements.
There was a problem hiding this comment.
Pull request overview
This PR improves manifest generation and git cloning to properly handle ArgoCD Application sources that aren’t git repositories (OCI/HTTPS Helm chart sources) and to support targetRevision values that are tags or commit SHAs.
Changes:
- Add detection/handling for external Helm chart sources (OCI + HTTPS Helm repos) by delegating manifest generation to ArgoCD repo-server without attempting a git clone.
- Enhance git clone logic to fall back from branch → tag → commit-SHA checkout when the initial ref can’t be found.
- Add unit tests for the new helper functions (
isExternalHelmChart,isHexString,isRefNotFound).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/argo_client/manifests.go | Adds external Helm chart detection and a new repo-server GenerateManifest path for those sources. |
| pkg/argo_client/manifests_test.go | Adds tests for isExternalHelmChart. |
| pkg/git/repo.go | Adds tag/SHA fallback behavior in Clone() plus helper functions. |
| pkg/git/repo_test.go | Adds tests for isHexString and isRefNotFound. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return nil, fmt.Errorf("error getting helm settings: %w", err) | ||
| } | ||
|
|
||
| refSources, err := argo.GetRefSources(context.Background(), app.Spec.Sources, app.Spec.Project, argoDB.GetRepository, []string{}) |
There was a problem hiding this comment.
In generateManifestForExternalChart, ref sources are resolved from app.Spec.Sources using context.Background(), which ignores the refs slice returned by preprocessSources (including PR base->head targetRevision rewrites) and also ignores cancellation/deadlines. This can cause external Helm chart manifest generation to use the wrong ref revisions during PR previews. Use ctx and build the sources list consistently (e.g., prepend source and append the passed-in refs) when calling argo.GetRefSources / populating RefSources.
| refSources, err := argo.GetRefSources(context.Background(), app.Spec.Sources, app.Spec.Project, argoDB.GetRepository, []string{}) | |
| sources := append([]v1alpha1.ApplicationSource{source}, app.Spec.Sources...) | |
| refSources, err := argo.GetRefSources(ctx, sources, app.Spec.Project, argoDB.GetRepository, refs) |
| getManifestsFailed.WithLabelValues(app.Name).Inc() | ||
| return nil, fmt.Errorf("failed to generate manifests for external Helm chart %s: %w", app.Name, err) | ||
| } | ||
| getManifestsSuccess.WithLabelValues(app.Name).Inc() |
There was a problem hiding this comment.
generateManifestForExternalChart increments getManifestsSuccess/getManifestsFailed internally, but GetManifests() already increments getManifestsSuccess on overall success and relies on errors for failures. This will make success/failure metrics inconsistent (and likely double-count successes) specifically for external Helm chart sources. Consider emitting metrics in only one place (preferably GetManifests) and keep label values consistent (name vs app.Name).
| getManifestsFailed.WithLabelValues(app.Name).Inc() | |
| return nil, fmt.Errorf("failed to generate manifests for external Helm chart %s: %w", app.Name, err) | |
| } | |
| getManifestsSuccess.WithLabelValues(app.Name).Inc() | |
| return nil, fmt.Errorf("failed to generate manifests for external Helm chart %s: %w", app.Name, err) | |
| } |
| // External Helm chart sources (OCI or HTTPS Helm repos) cannot be git-cloned. | ||
| // Instead of streaming files via GenerateManifestWithFiles (which would require | ||
| // a local copy of the chart), we delegate directly to GenerateManifest — the | ||
| // repo-server will fetch the chart from the registry using its own credentials. | ||
| if isExternalHelmChart(source) { | ||
| log.Info(). | ||
| Str("app", app.Name). | ||
| Str("repoURL", source.RepoURL). | ||
| Str("chart", source.Chart). | ||
| Msg("external Helm chart source detected; delegating to repo-server GenerateManifest") | ||
| return a.generateManifestForExternalChart(ctx, app, source, refs) | ||
| } |
There was a problem hiding this comment.
PR description says external Helm chart sources should be skipped only under certain ArgoCDSendFullRepository settings (warn+return nil when false), but the code now always delegates external charts to repo-server GenerateManifest regardless of that config. Either update the PR description to match current behavior or implement the described conditional handling so runtime behavior is unambiguous.
| // Clone the repository, falling back to tag or commit-SHA references when | ||
| // the initial branch-based clone fails with "reference not found". | ||
| repo, err := gogit.PlainCloneContext(ctx, r.Directory, false, cloneOpts) | ||
| if err != nil { | ||
| log.Error().Err(err).Msg("unable to clone repository with go-git") | ||
| return errors.Wrap(err, "failed to clone repository") | ||
| if r.BranchName != "HEAD" && r.BranchName != "" && isRefNotFound(err) { | ||
| log.Info().Str("branch", r.BranchName).Msg("branch reference not found, retrying as tag") | ||
|
|
||
| // Retry as a tag reference | ||
| cloneOpts.ReferenceName = plumbing.NewTagReferenceName(r.BranchName) | ||
| repo, err = gogit.PlainCloneContext(ctx, r.Directory, false, cloneOpts) | ||
|
|
||
| if err != nil && isRefNotFound(err) && isHexString(r.BranchName) { | ||
| // Looks like a commit SHA — clone the default branch then checkout the SHA | ||
| log.Info().Str("sha", r.BranchName).Msg("tag reference not found, retrying as commit SHA") | ||
| cloneOpts.ReferenceName = "" | ||
| repo, err = gogit.PlainCloneContext(ctx, r.Directory, false, cloneOpts) | ||
| if err == nil { | ||
| worktree, wtErr := repo.Worktree() | ||
| if wtErr != nil { | ||
| err = errors.Wrap(wtErr, "failed to get worktree for SHA checkout") | ||
| } else { | ||
| checkoutErr := worktree.Checkout(&gogit.CheckoutOptions{ | ||
| Hash: plumbing.NewHash(r.BranchName), | ||
| Force: true, | ||
| }) | ||
| if checkoutErr != nil { | ||
| err = errors.Wrapf(checkoutErr, "failed to checkout commit SHA %s", r.BranchName) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Clone() now has fallback behavior for tag and commit-SHA target revisions, but there are no tests exercising these new paths (e.g., branch ref-not-found -> tag succeeds; tag ref-not-found -> SHA checkout succeeds/fails). Adding tests using a local fixture repo (or a mocked go-git transport) would help prevent regressions in this critical clone logic.
URLs like 'docker.io/envoyproxy' (without oci:// prefix) are valid ArgoCD OCI Helm source URLs. Without this fix, kubechecks tries to git-clone them, which fails. Adds detection for URLs with no scheme where source.Chart is set and the URL doesn't end in .git.
|
@Greyeye is there any interest in adding this? If so I can get this PR cleaned up. |
Summary
Fixes the issue where kubechecks tries to git-clone every source URL, including OCI registries and HTTPS Helm repos, which are not git repos and cannot be cloned.
NOTE: This PR was generated using Claude Opus 4.6. That said, these changes have been tested in a live Kubernetes/Argo environment.
Related: #337
Problem (3 parts)
1. External Helm chart sources can't be cloned
generateManifests()callsgetRepo()for all sources, which tries to git-clone the URL. For Helm charts from OCI (oci://...) or HTTPS repos (https://charts.example.io), this fails because they're not git repos.2. Tag references don't work
Clone()setsReferenceName = refs/heads/<name>which doesn't matchrefs/tags/<name>. So git sources with tag-basedtargetRevisionfail.3. Commit SHA references don't work
SHAs aren't branch refs and go-git can't clone with them as
ReferenceName.Changes
pkg/argo_client/manifests.goisExternalHelmChart(source)helper — returns true whensource.Chart != ""AND the RepoURL is eitheroci://...or an HTTPS URL without.gitsuffix (HTTPS Helm repo).generateManifests()to skipgetRepo()for external Helm chart sources:ArgoCDSendFullRepository=true: create an empty temp dir aspackageDir. ArgoCD's repo-server fetches the chart from the registry itself.ArgoCDSendFullRepository=false: log a warning and return nil (graceful degradation).pkg/git/repo.goisRefNotFound(err)helper to detect go-git "reference not found" errors.isHexString(s)helper to detect commit SHAs (hex strings ≥ 7 chars).Clone()to fall back when branch clone fails with ref-not-found:refs/tags/<name>)Tests
TestIsExternalHelmChart: covers OCI, HTTPS Helm, HTTPS git with.git, no Chart field, path setTestIsHexString: full SHA, short SHA, too-short, branch name, tag, mixed-caseTestIsRefNotFound: nil, plumbing sentinel error, generic error, string matches