Support git url repository #11258

yxxhero · 2022-08-23T12:35:01Z

Continue the PR #6734 work.
related issue: #9461

If applicable:

this PR contains documentation
this PR contains unit tests
this PR has been tested for backwards compatibility

see: #9482

Signed-off-by: yxxhero <aiopsclub@163.com>

Signed-off-by: Jeff Valore <rally25rs@yahoo.com>

Signed-off-by: yxxhero <aiopsclub@163.com>

Signed-off-by: yxxhero <aiopsclub@163.com> add license headers for gitutils Signed-off-by: yxxhero <aiopsclub@163.com>

Signed-off-by: yxxhero <aiopsclub@163.com>

TBBle · 2022-08-23T19:19:01Z

cmd/helm/dependency.go

+The 'repository' can be the https or ssh URL that you would use to clone a git
+repo or add as a git remote, prefixed with 'git:'.
+For example 'git://git@github.com:helm/helm-chart.git' or
+'git://https://github.com/helm/helm-chart.git'


git://https://github.com/helm/helm-chart.git isn't an awesome URL. (I'm not even sure the double-scheme is a valid URL). Also, git://<SCP format> looks a lot like the Git protocol format git://host:port you'd give to the CLI.

What about something like git+XXX:// since then we could simple strip off the git+ part, and as long as git accepts the remains, e.g., https://, ssh://, file://, then we don't need to validate that any harder and are also future-proofed against git changes or locally-defined transport helpers. (I've most-frequently seen this format in Python described as VCS+protocol).

It might make sense to still support git://<SCP-like> and just munge that back into ssh:// syntax before further processing, since the Git transport would be git+git://<hostname>. That mainly benefits people cut-and-pasting from, e.g., GitHub's "Clone" dialog which gives SCP-like syntax for SSH clones.

This is also how NPM Git URLs work in yarn which was cited as the inspiration in #6734, but I didn't see any discussion of this URL syntax difference in that PR.

TBBle · 2022-08-23T19:32:08Z

cmd/helm/dependency.go

+it is recommended to use an SSH git url, and have your git client configured
+with an SSH cert that does not require a password.
+* The helm chart and 'Chart.yaml' must be in the root of the git repo.
+The chart cannot be loaded from a subdirectory.


This seems like a significant limitation for the general case, as Helm charts often live in sub-directories of their associated source project.

Maybe for a later improvement, but (again, leaning on Python) perhaps something like a #subdirectory=X fragment in the URL would allow this.

It might be a place to hang other features like branch name/tag/reference, etc., and that would free-up version to be used for semver-matching, as is the case in the similar file:// support. (With perhaps the same shortcut as OCI support that if version is a literal semver, we treat it as a git tag, i.e. the behaviour you have now, and if it's a SemVer match, we could match against git tags, assuming no branch name/tag/reference was provided in the URL.)

This interaction with version (dropping support for version being a branch-name, so it works like and is validated like version on any other dependency) is the reason I'd consider addressing this here, even if it's just to close off the non-semver version support now, pending further consideration.

TBBle · 2022-08-23T19:52:29Z

internal/fileutil/fileutil.go

+	zr := gzip.NewWriter(buf)
+
+	zr.ModTime = time.Date(1977, time.May, 25, 0, 0, 0, 0, time.UTC)
+	zr.Header.ModTime = time.Date(1977, time.May, 25, 0, 0, 0, 0, time.UTC)


Is there a particular significant to this date? Since the tarball is being created now, why not use the current date? Or date of last commit?

TBBle · 2022-08-23T19:55:38Z

pkg/getter/gitgetter.go

+	// A .helmignore that includes an ignore for .git/ should be included in the git repo itself,
+	// but a lot of people will probably not think about that.
+	// To prevent the git history from bleeding into the charts archive, append/create .helmignore.
+	g.ensureGitDirIgnored(chartTmpDir)


The example .helmignore in the docs includes .git. And if it wasn't included, wouldn't the target repo have the same problem of including the .git directory when running helm package?

Also, does this even work? https://github.com/helm/helm/blob/v3.9.3/pkg/chart/loader/load.go#L55-L57 suggests it will be ignored when this archive is handled.

Would it be better to honour .helmignore CompressDirToTgz perhaps?

TBBle · 2022-08-23T20:02:53Z

internal/resolver/resolver.go

@@ -107,6 +110,27 @@ func (r *Resolver) Resolve(reqs []*chart.Dependency, repoNames map[string]string
 			continue
 		}

+		if strings.HasPrefix(d.Repository, "git://") {


Perhaps all the open-coded strings.HasPrefix(d.Repository, "git://") calls could be moved into a function, e.g., gitutils.IsGit, analog to registry.IsOCI.

TBBle · 2022-08-23T20:08:56Z

pkg/getter/gitgetter.go

+	"helm.sh/helm/v3/internal/fileutil"
+)
+
+// GitGetter is the default HTTP(/S) backend handler


Is this comment correct?

TBBle · 2022-08-23T20:14:14Z

pkg/gitutils/gitutils.go

+		return false, err
+	}
+
+	if err := repo.Get(); err != nil {


Is this doing a full clone just for this check? Could this be minimised into a shallow clone of the given ref, and failure means it doesn't exist? For that matter, if we only care about branches or tags (as is currently the case), the docs imply we can get that without cloning at all.

TBBle · 2022-08-23T20:15:01Z

pkg/gitutils/gitutils.go

+)
+
+func HasGitReference(gitRepo, ref, repoName string) (bool, error) {
+	local, err := os.MkdirTemp("", repoName)


Is there a point in passing/using repoName just for this? The temp directory name doesn't really matter, I'd think?

TBBle · 2022-08-23T20:24:00Z

pkg/downloader/chart_downloader.go

@@ -103,10 +113,17 @@ func (c *ChartDownloader) DownloadTo(ref, version, dest string) (string, *proven
 	}

 	name := filepath.Base(u.Path)
-	if u.Scheme == registry.OCIScheme {
+	if u.Scheme == registry.OCIScheme || u.Scheme == "git" {


This new condition doesn't seem to do anything, because if it's true, then scheme would also be "git", and so the next block will immediately overwrite name anyway. I also don't see how this block would apply to git sources anyway, they don't separate the chart name and version with a colon like OCI sources.

TBBle · 2022-08-23T20:24:37Z

pkg/downloader/manager.go

@@ -594,6 +611,16 @@ func (m *Manager) resolveRepoNames(deps []*chart.Dependency) (map[string]string,
 			reposMap[dd.Name] = dd.Repository
 			continue
 		}
+		// if dep chart is from a git url, assume it is valid for now.
+		// if the repo does not exist then it will later error when we try to fetch branches and tags.
+		// we could check for the repo existence here, but trying to avoid anotehr git request.


nit: Typo "anotehr".

rblaine95 · 2022-10-29T10:08:47Z

Any specific reasons why this PR was closed?

yxxhero · 2022-10-29T11:49:28Z

try to use:

https://github.com/aslafy-z/helm-git

@rblaine95 if you have interesting to complete this PR. please do. Thanks very much.