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

Continue to add support for git urls as dependency repositories #9482

Closed
wants to merge 10 commits into from
Closed

Continue to add support for git urls as dependency repositories #9482

wants to merge 10 commits into from

Conversation

yxxhero
Copy link
Member

@yxxhero yxxhero commented Mar 14, 2021

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

@helm-bot helm-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 14, 2021
Copy link
Contributor

@jdolitsky jdolitsky 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 implement the Getter interface for the git:// protocol? This will allow it to be used for helm install as well.

Here is an example PR for how this was done for oci://: #8843

@yxxhero
Copy link
Member Author

yxxhero commented Mar 16, 2021

ok, I will do this work as soon as I can. Thanks for your reply. @jdolitsky

@yxxhero yxxhero marked this pull request as draft March 16, 2021 11:39
@helm-bot helm-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 19, 2021
@BlueCog
Copy link

BlueCog commented Apr 2, 2021

Hi @yxxhero , is there anything i can help you with?

And one more question: is it also possible to give a contextDir with a chart in the Chart.yaml? For the usecase when the chart is not directly in the root of the Git repository (see example below)

# Chart.yaml
dependencies:
- name: nginx
  version: "1.2.3"
  repository: "ssh://git@git.host:7999/helm-dep/helm-dep.git"
  contextDir: "helm/chart" 

@yxxhero
Copy link
Member Author

yxxhero commented Apr 5, 2021

Hi @yxxhero , is there anything i can help you with?

And one more question: is it also possible to give a contextDir with a chart in the Chart.yaml? For the usecase when the chart is not directly in the root of the Git repository (see example below)

# Chart.yaml
dependencies:
- name: nginx
  version: "1.2.3"
  repository: "ssh://git@git.host:7999/helm-dep/helm-dep.git"
  contextDir: "helm/chart" 

I'm busy recently. I'll try to finish it Thanks very much.

@yxxhero yxxhero marked this pull request as ready for review April 24, 2021 10:12
@yxxhero yxxhero requested a review from jdolitsky April 24, 2021 10:12
@yxxhero
Copy link
Member Author

yxxhero commented Apr 24, 2021

@jdolitsky ping

@yxxhero
Copy link
Member Author

yxxhero commented Apr 26, 2021

@technosophos @mattfarina @bacongobbler @jdolitsky Look forward to your reply. Thanks very much.

@BlueCog
Copy link

BlueCog commented Apr 29, 2021

Hey @yxxhero great work!

From the inline docs i get that it is not possible to use subdirectories (contextdir) with this Git functionality:

* The helm chart and 'Chart.yaml' must be in the root of the git repo.
The chart cannot be loaded from a subdirectory.

Is this a design choice? Or is it functionality what is possible to add? For example we (my team) have more files and directories in our helm-chart-repo's. Files needed for CICD pipelines for example.

So our repo would be:

\chart-cicd\helm-chart
           \cicd-files 

So when referencing the helm chart in the dependencies we would like to reference the subdir helm-chart from the repo chart-cicd

@yxxhero
Copy link
Member Author

yxxhero commented Apr 29, 2021

Hey @yxxhero great work!

From the inline docs i get that it is not possible to use subdirectories (contextdir) with this Git functionality:

* The helm chart and 'Chart.yaml' must be in the root of the git repo.
The chart cannot be loaded from a subdirectory.

Is this a design choice? Or is it functionality what is possible to add? For example we (my team) have more files and directories in our helm-chart-repo's. Files needed for CICD pipelines for example.

So our repo would be:

\chart-cicd\helm-chart
           \cicd-files 

So when referencing the helm chart in the dependencies we would like to reference the subdir helm-chart from the repo chart-cicd

The current design is like this. Of course you can change it and work together to do it. @BlueCog

@yxxhero
Copy link
Member Author

yxxhero commented Apr 29, 2021

I'm going to add contextDir features in my own repo. If it is done. You can test it. Thanks very much. @BlueCog

@yxxhero
Copy link
Member Author

yxxhero commented May 6, 2021

@bacongobbler @jdolitsky Does this feature require design documentation? Thanks very much.

@BlueCog
Copy link

BlueCog commented May 6, 2021

@yxxhero

FYI: i've build a Helm client with your PR and tested the functionality (private and public repo's). It works without any issue.

Greets

@yxxhero yxxhero changed the title Support git url repository Continue to add support for git urls as dependency repositories May 8, 2021
@helm-bot helm-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 2, 2021
@yxxhero
Copy link
Member Author

yxxhero commented Jun 2, 2021

@jdolitsky ping

pkg/getter/gitgetter.go Outdated Show resolved Hide resolved
pkg/gitutil/gitutil.go Outdated Show resolved Hide resolved
internal/resolver/resolver.go Outdated Show resolved Hide resolved
internal/resolver/resolver.go Outdated Show resolved Hide resolved
internal/resolver/resolver.go Outdated Show resolved Hide resolved
pkg/downloader/chart_downloader.go Outdated Show resolved Hide resolved
pkg/downloader/chart_downloader.go Outdated Show resolved Hide resolved
pkg/downloader/chart_downloader.go Outdated Show resolved Hide resolved
pkg/downloader/manager.go Outdated Show resolved Hide resolved
pkg/downloader/manager.go Outdated Show resolved Hide resolved
pkg/getter/gitgetter.go Outdated Show resolved Hide resolved
cmd/helm/dependency.go Show resolved Hide resolved
cmd/helm/dependency.go Outdated Show resolved Hide resolved
@yxxhero yxxhero requested a review from jdolitsky June 5, 2021 10:32
@rblaine95
Copy link

I built the last commit from this PR and am getting the following error when trying to helmy dep up

» helmy version
version.BuildInfo{Version:"v3.6.1-yxx-1+unreleased", GitCommit:"eef224055ea6457c5a628a76815b0de5b0efe2c1", GitTreeState:"clean", GoVersion:"go1.16.5"}

» helmy dep up
Hang tight while we grab the latest from your chart repositories...
[... trim ...]
Update Complete. ⎈Happy Helming!⎈
Saving 2 charts
Downloading helm-example from repo git://git@bitbucket.org:example/helm-example.git
Save error occurred:  could not download git://git@bitbucket.org:example/helm-example.git: mkdir /tmp/helm1319646873: not a directory
Deleting newly downloaded charts, restoring pre-update state
Error: could not download git://git@bitbucket.org:example/helm-example.git: mkdir /tmp/helm1319646873: not a directory

Another issue I'm getting is, if the name of the dependency doesn't match the name of the repo, I get this error:

dependencies:
  - name: library
    version: 1.2.3
    repository: git://git@bitbucket.org:example/helm-library.git
» helmy dep up
Hang tight while we grab the latest from your chart repositories...
[... trim ...]
Update Complete. ⎈Happy Helming!⎈
Error: The name of dependency git://git@bitbucket.org:example/helm-library.git should be helm-library

In this case, the name of the chart is library but the repo is named helm-library.
The name field in the Chart.yaml is also library.

@yxxhero
Copy link
Member Author

yxxhero commented Jun 23, 2021

I built the last commit from this PR and am getting the following error when trying to helmy dep up

» helmy version
version.BuildInfo{Version:"v3.6.1-yxx-1+unreleased", GitCommit:"eef224055ea6457c5a628a76815b0de5b0efe2c1", GitTreeState:"clean", GoVersion:"go1.16.5"}

» helmy dep up
Hang tight while we grab the latest from your chart repositories...
[... trim ...]
Update Complete. ⎈Happy Helming!⎈
Saving 2 charts
Downloading helm-example from repo git://git@bitbucket.org:example/helm-example.git
Save error occurred:  could not download git://git@bitbucket.org:example/helm-example.git: mkdir /tmp/helm1319646873: not a directory
Deleting newly downloaded charts, restoring pre-update state
Error: could not download git://git@bitbucket.org:example/helm-example.git: mkdir /tmp/helm1319646873: not a directory

Another issue I'm getting is, if the name of the dependency doesn't match the name of the repo, I get this error:

dependencies:
  - name: library
    version: 1.2.3
    repository: git://git@bitbucket.org:example/helm-library.git
» helmy dep up
Hang tight while we grab the latest from your chart repositories...
[... trim ...]
Update Complete. ⎈Happy Helming!⎈
Error: The name of dependency git://git@bitbucket.org:example/helm-library.git should be helm-library

In this case, the name of the chart is library but the repo is named helm-library.
The name field in the Chart.yaml is also library.

  1. mkdir /tmp/helm1319646873: not a directory fix. I used os.CreateTemp by mistake.
  2. you shuld change like below:
dependencies:
  - name: helm-library
    version: 1.2.3
    repository: git://git@bitbucket.org:example/helm-library.git

@rblaine95
Copy link

rblaine95 commented Jun 24, 2021

Great, rebuilt the latest commit (416efce) and changed the name of the dependency to helm-library to match the name of the repo.

» helmy version
version.BuildInfo{Version:"v3.6.1-yxx-1+unreleased", GitCommit:"416efce3c2859d182ceaa6f739018bd46ab13259", GitTreeState:"clean", GoVersion:"go1.16.5"}

» helmy dep up
Hang tight while we grab the latest from your chart repositories...
[... trim ...]
Update Complete. ⎈Happy Helming!⎈
Saving 1 charts
Downloading helm-library from repo git://git@bitbucket.org:example/helm-library.git
Deleting outdated charts

One question, does the name of the dependency have to match the name of the repo?
I ask this because, in my org, we use the helm- prefix to distinguish between applications and helm charts that deploy said applications (or libraries).
We named our internal library chart libs, but the repo helm-libs.
Just like scala-libs or jenkins-libs

I've noticed in umbrella/master charts if the name/version of a dependency doesn't match the name/version of the downloaded chart, helm values don't propagate downwards.

I'll craft a reproducible example and post it here a little later, maybe this issue is outside of the scope of this PR.

Edit: I'll see what I can do regarding the example, I may be mistaken due to having used a build of the old git PR and possible issues in that version.

@yxxhero
Copy link
Member Author

yxxhero commented Jun 24, 2021

Great, rebuilt the latest commit (416efce) and changed the name of the dependency to helm-library to match the name of the repo.

» helmy version
version.BuildInfo{Version:"v3.6.1-yxx-1+unreleased", GitCommit:"416efce3c2859d182ceaa6f739018bd46ab13259", GitTreeState:"clean", GoVersion:"go1.16.5"}

» helmy dep up
Hang tight while we grab the latest from your chart repositories...
[... trim ...]
Update Complete. ⎈Happy Helming!⎈
Saving 1 charts
Downloading helm-library from repo git://git@bitbucket.org:example/helm-library.git
Deleting outdated charts

One question, does the name of the dependency have to match the name of the repo?
I ask this because, in my org, we use the helm- prefix to distinguish between applications and helm charts that deploy said applications (or libraries).
We named our internal library chart libs, but the repo helm-libs.
Just like scala-libs or jenkins-libs

I've noticed in umbrella/master charts if the name/version of a dependency doesn't match the name/version of the downloaded chart, helm values don't propagate downwards.

I'll craft a reproducible example and post it here a little later, maybe this issue is outside of the scope of this PR.

@rblaine95 I will find a way to remove this restriction. wait for your example. I will use your example to test it.

@helm-bot helm-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jul 24, 2021
@yxxhero
Copy link
Member Author

yxxhero commented Jul 31, 2021

@jdolitsky @technosophos ping

@rblaine95
Copy link

Any news regarding this PR?
We're very excited to use this feature 🚀

@yxxhero
Copy link
Member Author

yxxhero commented Oct 7, 2021

The PR is already available. I have written HIP and will actively promote the merger of PR. @rblaine95 @BlueCog @pmahoney-raise
Thank you for your attention.

@rblaine95
Copy link

Setting version to a non-semantic version (for example: a branch name like feat/refactor-ingress) fails:

» helm version
version.BuildInfo{Version:"v3.7+unreleased", GitCommit:"edeeaabb294ea99aecd6753823010d2508c0fd02", GitTreeState:"clean", GoVersion:"go1.17.2"}

» HELM_EXPERIMENTAL_GIT=1 helm dep up
Hang tight while we grab the latest from your chart repositories...
[...]
Update Complete. ⎈Happy Helming!⎈
Error: dependency "zenlibs" has an invalid version/constraint format: improper constraint: feat/refactor-ingress
dependencies:
  - name: zenlibs
    version: feat/refactor-ingress
    repository: git://git@bitbucket.org:zenaptix/helm-zenlibs.git

@yxxhero
Copy link
Member Author

yxxhero commented Oct 18, 2021

@rblaine95 This is the current default behavior. Do you have any idea?

@rblaine95
Copy link

I thought this PR supports branch names and tags?

@yxxhero
Copy link
Member Author

yxxhero commented Oct 18, 2021

I'll consider your suggestion. @rblaine95 Thanks very much.

@rblaine95
Copy link

rblaine95 commented Oct 18, 2021

I'll consider your suggestion. @rblaine95 Thanks very much.

It says here

When using a 'git://' repository, the 'version' must be a valid semantic tag or branch
name for the git repo. For example 'master'.

Edit:
internal/resolver/resolver.go:L127-130

if !found {
  return nil, fmt.Errorf(`dependency %q is missing git branch or tag: %s.
When using a "git://" type repository, the "version" should be a valid branch or tag name`, d.Name, d.Version)
}

pkg/getter/gitgetter.go:L67-69

if version == "" {
  return nil, fmt.Errorf("the version must be a valid tag or branch name for the git repo, not nil")
}

It looks like your PR already does support branch name, but there's something that checks the version constraint before the git checks which causes it to bomb out.

@yxxhero
Copy link
Member Author

yxxhero commented Oct 18, 2021

@rblaine95 cool, you are right.

@yxxhero
Copy link
Member Author

yxxhero commented Jan 22, 2022

@jdolitsky @technosophos

@yxxhero
Copy link
Member Author

yxxhero commented Mar 28, 2022

@jdolitsky @technosophos ping

yxxhero and others added 10 commits May 21, 2022 20:21
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>
Signed-off-by: yxxhero <aiopsclub@163.com>
Signed-off-by: yxxhero <aiopsclub@163.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>
Signed-off-by: yxxhero <aiopsclub@163.com>
@yxxhero yxxhero closed this by deleting the head repository Aug 23, 2022
@jackivanov
Copy link

In the tendency to GitOps it's sad to see this PR being closed. The maintains don't seem to appreciate the community effort at all.

@yxxhero
Copy link
Member Author

yxxhero commented Aug 23, 2022

@jdolitsky oh. sorry. It's my fault. I will create a PR. Thanks very much.

@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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants