Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for git urls as dependency repositories #6734

Closed
wants to merge 4 commits into from

Conversation

rally25rs
Copy link

@rally25rs rally25rs commented Oct 22, 2019

What this PR does / why we need it:

This PR adds support for using a git URL as a repository in requirements.yaml.

I thought this would be a nice-to-have feature for organizations that house their helm charts in a git repository and would like to maintain private access to those charts.

The format supported is:

dependencies:
- name: "{dependency name}"
  repository: "git:{git repo url}"
  version: "{git ref name}"

where:

  • {git repo url} is any url type that a git clone ... command would accept (an ssh location, an https url, etc.).
  • {git ref name} is an existing tag or branch name on the repo (a commit sha cannot be used at this time)

For example:

dependencies:
- name: "a"
  repository: "git:https://github.com/rally25rs/helm-test-chart.git"
  version: "master"

(the above rally25rs/helm-test-chart does exist. feel free to use it for testing if you want.)

Special notes for your reviewer:

This is my first time coding in Go, so please feel free to comment on the code and provide any refactoring suggestions or ways to do things better.

Since other package managers like npm/yarn, ruby bundler, etc. support github URLs, I thought this might be a nice to have feature. If this isn't the direction that the helm community wants to take for dependency management, we can reject this PR. I won't be offended :) It was still a fun learning experience to do something with Go, and this only took a day and a half to whip up. I actually made this PR instead of trying to build my own private custom registry in the traditional helm way, because that seemed more difficult and would likely require some process to monitor our github organization for changes, check them out to a central location, run the helm command to build the index.yaml, and push it all to a central server that only our employees have access to. Git already has SSH to secure it, so including our private charts in this way seemed a lot easier.

Thanks for your consideration and for taking the time to review this!

If applicable:

  • this PR contains documentation (I added to cmd/helm/dependency.go but it looks like the rest of the generated docs are no longer in the v3 codebase, and make docs doesn't seem to be a command any more? If there are additional docs to edit please let me know)
  • this PR contains unit tests (I added a few where I could. Also see Manual Verification notes below.)
  • this PR has been tested for backwards compatibility (since no existing registry urls would have started with git: there shouldn't be any problem with existing registries)

Implementation Details

The implementation is a simplified take on what yarn does for git dependencies https://github.com/yarnpkg/yarn/blob/master/src/util/git.js (I'm also a yarn contributor so was familiar with their code base)

What it basically does is, if it find a registry that starts with git: then:

  • run a git ls-remote sub process to get the tag and branch names for the repo.
  • verify that the version is a valid tag or branch name.
  • create a temp directory.
  • run a (shallow) git clone sub process to fetch the repo at the specified branch/tag into the temp dir
  • Treat the cloned git repo similar to a file:///path/to/temp/dir style requirement; use chart.LoadDir to load that directory (which in turn applied the logic for filtering the files through .helmignore) and archives it to charts/
  • Delete the temp dir.

Caveats / Known Limitations

  • Since this spawns git as a child process, git has to be available on the system and in the path.
  • There is currently no handling for forwarding stdin to the git child process, so if git asks for input, helm will likely appear to hang as git tries to prompt for input. It will cause an issue if you are trying to reference a private repo with an SSL cert that needs a password, or with an https url that requires username/password authentication. (yarn package manager, ansible, and other tools have this same issue). I couldn't think of a great way to handle this, since you don't really want to put all of the git command's stdout to the screen; just any prompts it might display, but there isn't really an easy way to differentiate.

Manual Verification

In addition to running make test you can try this out by creating a chart and adding the requirements.yaml file:

dependencies:
- name: "a"
  repository: "git:https://github.com/rally25rs/helm-test-chart.git"
  version: "master"

Then run helm dep update and inspect the contents of the resulting archive file in charts/

$ tar -tvf charts/a-1.0.0.tgz
-rw-r--r--  0 0      0         164 Dec 31  1969 a/Chart.yaml
-rw-r--r--  0 0      0          84 Dec 31  1969 a/values.yaml
-rw-r--r--  0 0      0          73 Dec 31  1969 a/templates/deployment.yaml
-rw-r--r--  0 0      0          17 Dec 31  1969 a/.helmignore
-rw-r--r--  0 0      0         164 Dec 31  1969 a/charts/b/Chart.yaml
-rw-r--r--  0 0      0          16 Dec 31  1969 a/charts/b/values.yaml
-rw-r--r--  0 0      0          73 Dec 31  1969 a/charts/b/templates/deployment.yaml
-rw-r--r--  0 0      0          97 Dec 31  1969 a/charts/b/charts/c/Chart.yaml
-rw-r--r--  0 0      0          16 Dec 31  1969 a/charts/b/charts/c/values.yaml
-rw-r--r--  0 0      0          73 Dec 31  1969 a/charts/b/charts/c/templates/deployment.yaml

Future Possibilities

I intentionally left this functionality simple for now to see how well the helm community would receive it. If this PR is accepted, I can see some room for improvements in future PRs:

  • Handle having a chart in a sub-directory of a git repo (currently Chart.yaml must be in the git repo root)
  • Better handle git command stdin/out - Simply adding GIT_TERMINAL_PROMPT=0 to the env when running the sub commands may help https://github.blog/2015-02-06-git-2-3-has-been-released/#the-credential-subsystem-is-now-friendlier-to-scripting
  • I originally wanted to write the current commit sha for the branch/tag to the requirements.lock file, so that if branches or tags move forward, you would still be locked to the last commit. However the git clone --branch {name} command has no way to shallow checkout a single commit, only a branch or tag (git archive can, but github doesn't support that command). It'd be nice to lock to a specific commit. This could be done with a full git clone (non shallow) then a git checkout but I wanted to avoid the overhead of the full clone.
  • I'm not particularly fond of the conditionals if strings.HasPrefix(d.Repository, "file://") and if strings.HasPrefix(d.Repository, "git:") in a couple places (violates the open-closed principle) and would like to see those refactored out to their own logical structs. I didn't want to do that refactoring as part of this PR because I thought moving the file:// logic around would obscure the diff and make it harder to review this PR. I'd rather make it a separate PR with just refactoring changes, no functional changes.

@helm-bot helm-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 22, 2019
Signed-off-by: Jeff Valore <rally25rs@yahoo.com>
@mattfarina mattfarina added the v3.x Issues and Pull Requests related to the major version v3 label Oct 22, 2019
@mattfarina mattfarina added this to the 3.1.0 milestone Oct 22, 2019
@mattfarina
Copy link
Collaborator

@rally25rs Thanks for the PR. Helm v3.0.0 is in the home stretch. We are in bug fix mode now. We can add features in minor releases so I am adding it to that milestone so we can hopefully get it in there. I just wanted to let you know it might be a little bit before we get to this and before it shows up in a release.

@bacongobbler bacongobbler modified the milestones: 3.1.0, 3.2.0 Jan 30, 2020
@rblaine95
Copy link

This is an amazing feature that I am definitely looking forward to!
With this I won't need to maintain a helm registry or juggle git submodules anymore

@rblaine95
Copy link

I just did a custom build of release-3.1 branch with this commit cherry-picked on it, tested it out with a custom chart with multiple dependencies on git repositories

dependencies:
  - name: repo1
    version: v0.2.0
    repository: git:git@bitbucket.org:company/helm-repo1.git
    alias: repo1
    condition: repo1.enabled
  
  - name: repo2
    version: v0.2.2
    repository: git:git@bitbucket.org:company/repo2.git
    alias: repo2
    condition: repo2.enabled
» h3z version
version.BuildInfo{Version:"v3.1.0-1Z", GitCommit:"cba7c89dc2ec2554550deb191d5d2fea03bcec99", GitTreeState:"clean", GoVersion:"go1.13.8"}

Running h3z dep up successfully pulled the git repositories at the correct tags.

@rblaine95
Copy link

Quick update, helm dep up works, helm dep build breaks with the following error:

Error: no repository definition for git:git@bitbucket.org:company/repo1.git, git:git@bitbucket.org:company/repo2.git. Please add the missing repos via 'helm repo add'
# requirements.lock
dependencies:
- name: repo1
  repository: git:git@bitbucket.org:company/repo1.git
  version: feature/my-test-branch
- name: repo2
  repository: git:git@bitbucket.org:company/repo2.git
  version: v0.2.2
digest: sha256:454becce429bf9b4c608dac23b638b754cf51e2b39a7baa0807ccbea0c995276
generated: "2020-02-19T13:49:53.390806944+02:00"

Copy link
Member

@bacongobbler bacongobbler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please address @rblaine95's comments? Once that's done we can start testing this.

One option that may be more portable than hardcoding this in the resolver would be to implement a getter for the git protocol. That way helm install git://git@github.com:helm/helm/chart.git would work. See https://github.com/helm/helm/tree/master/pkg/getter for more info.

@rally25rs
Copy link
Author

OK I'll take a look. This was originally written against Helm 2, then Helm 3 released right as I was making this PR so I sort of just merged it into Helm 3 without much testing, so I'm not surprised that there are a couple issues.
I'll try to get to this early next week.
Thanks for giving this PR some eyes! 👀

@bacongobbler bacongobbler removed this from the 3.2.0 milestone Mar 2, 2020
Signed-off-by: Jeff Valore <rally25rs@yahoo.com>
Signed-off-by: Jeff Valore <rally25rs@yahoo.com>
@rally25rs
Copy link
Author

@bacongobbler I addressed the dep build error reported by @rblaine95 , and also merged latest master into my branch to bring it up to date.

I looked into refactoring my changes into a getter as suggested, but I wasn't sure exactly how to handle that. It looks like the getters are expected to return a *bytes.Buffer however the way my code was working was to git clone a repo into a temp directory, then use chart.LoadDir to turn it into a Chart object. I wasn't sure if there was a way to turn that Chart back into a *bytes.Buffer. Any guidance is appreciated. (I am @jeff-valore in Kubernetes Slack if you want to discuss outside this issue thread)

@rblaine95
Copy link

Just built a new custom version of 3.1.1 with these commits cherry-picked.
helm dep build works now 👍

@GarbageYard
Copy link

Any idea when can we expect this for general consumption?

@rblaine95
Copy link

rblaine95 commented May 28, 2020

I've distributed a custom build of helm-v3.2.1 with these commits cherry-picked (merge conflicts resolved) in my office and several of my colleagues have started using git registries as dependencies.

This feature has made life quite a bit easier with handling Chart Dependencies without needing to resort to the hellish nightmare that is Git Submodules.

We've started using this feature in production to deploy our software (we recently migrated from self-hosted OKD to AWS EKS) and I'm now driving the use and development of Helm Library Charts to keep our charts DRY.

@jdolitsky
Copy link
Contributor

@rally25rs for some guidance on how to implement this as a Getter, please see #7613

@TBBle
Copy link
Contributor

TBBle commented Sep 16, 2020

I wasn't sure if there was a way to turn that Chart back into a *bytes.Buffer.

Per #7613, it seems, given c *chart.Chart, it's as simple as:

	buf := bytes.NewBuffer(nil)
	err = chartutil.Write(c, buf)
	return buf, err

@junnplus
Copy link

junnplus commented Feb 4, 2021

This is an amazing feature! Any progress?

@rally25rs
Copy link
Author

Unfortunately I haven't had the time to come back and try to resolve the merge issues or anything. I haven't actually needed the feature anymore so haven't been able to take the time out of my usual work day to take care of it.

If anyone wants to take it over and push it across the finish line, please do.

@bacongobbler
Copy link
Member

bacongobbler commented Aug 23, 2021

closing as stale. See #9482.

@yxxhero yxxhero mentioned this pull request Aug 23, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files. v3.x Issues and Pull Requests related to the major version v3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants