fix(helm): fix failing helm dep up on subchar requirements

Closes #3742

Signed-off-by: Sebastien Gandon <sgandon@talend.com>

pkg/downloader/manager.go

@@ -216,42 +216,58 @@ func (m *Manager) downloadAll(deps []*chartutil.Dependency) error {
 	fmt.Fprintf(m.Out, "Saving %d charts\n", len(deps))
 	var saveError error
 	for _, dep := range deps {
+		var isSubChart bool
 		if strings.HasPrefix(dep.Repository, "file://") {
-			if m.Debug {
+			//check if file repo path does belong to the current chart "/charts" folder
-				fmt.Fprintf(m.Out, "Archiving %s from repo %s\n", dep.Name, dep.Repository)
+			isSubChart, err = resolver.IsLocalSubChart(dep.Repository, m.ChartPath)
-			}
-			ver, err := tarFromLocalDir(m.ChartPath, dep.Name, dep.Repository, dep.Version)
 			if err != nil {
 				saveError = err
-				break
+			} //else everythig is fine.
-			}
-			dep.Version = ver
-			continue
 		}
 
-		fmt.Fprintf(m.Out, "Downloading %s from repo %s\n", dep.Name, dep.Repository)
+		//Only download or package dependencies that are not sub chart in the "/charts" folder
+		if !isSubChart {
+			if strings.HasPrefix(dep.Repository, "file://") {
+				if m.Debug {
+					fmt.Fprintf(m.Out, "Archiving %s from repo %s\n", dep.Name, dep.Repository)
+				}
+				ver, err := tarFromLocalDir(m.ChartPath, dep.Name, dep.Repository, dep.Version)
+				if err != nil {
+					saveError = err
+					break
+				}
+				dep.Version = ver
+				continue
+			}
 
-		// Any failure to resolve/download a chart should fail:
+			fmt.Fprintf(m.Out, "Downloading %s from repo %s\n", dep.Name, dep.Repository)
-		// https://github.com/kubernetes/helm/issues/1439
-		churl, username, password, err := findChartURL(dep.Name, dep.Version, dep.Repository, repos)
-		if err != nil {
-			saveError = fmt.Errorf("could not find %s: %s", churl, err)
-			break
-		}
 
-		dl := ChartDownloader{
+			// Any failure to resolve/download a chart should fail:
-			Out:      m.Out,
+			// https://github.com/kubernetes/helm/issues/1439
-			Verify:   m.Verify,
+			churl, username, password, err := findChartURL(dep.Name, dep.Version, dep.Repository, repos)
-			Keyring:  m.Keyring,
+			if err != nil {
-			HelmHome: m.HelmHome,
+				saveError = fmt.Errorf("could not find %s: %s", churl, err)
-			Getters:  m.Getters,
+				break
-			Username: username,
+			}
-			Password: password,
+
-		}
+			dl := ChartDownloader{
+				Out:      m.Out,
+				Verify:   m.Verify,
+				Keyring:  m.Keyring,
+				HelmHome: m.HelmHome,
+				Getters:  m.Getters,
+				Username: username,
+				Password: password,
+			}
 
-		if _, _, err := dl.DownloadTo(churl, "", destPath); err != nil {
+			if _, _, err := dl.DownloadTo(churl, "", destPath); err != nil {
-			saveError = fmt.Errorf("could not download %s: %s", churl, err)
+				saveError = fmt.Errorf("could not download %s: %s", churl, err)
-			break
+				break
+			}
+		} else {
+			if m.Debug {
+				fmt.Fprintf(m.Out, "did not package local chart %s: %s\n", dep.Name, dep.Repository)
+			}
 		}
 	}
 

pkg/downloader/manager_test.go

@@ -17,6 +17,9 @@ package downloader
 
 import (
 	"bytes"
+	"io/ioutil"
+	"os"
+	"path/filepath"
 	"reflect"
 	"strings"
 	"testing"
@@ -95,6 +98,7 @@ func TestFindChartURL(t *testing.T) {
 
 func TestGetRepoNames(t *testing.T) {
 	b := bytes.NewBuffer(nil)
+
 	m := &Manager{
 		Out:      b,
 		HelmHome: helmpath.Home("testdata/helmhome"),
@@ -181,3 +185,60 @@ func TestGetRepoNames(t *testing.T) {
 		}
 	}
 }
+
+func TestDownloadAllSubChartsOnly(t *testing.T) {
+	b := bytes.NewBuffer(nil)
+
+	//create a chart dir
+	tmp, err := ioutil.TempDir("", "helm-chart")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.RemoveAll(tmp)
+
+	m := &Manager{
+		Out:       b,
+		HelmHome:  helmpath.Home("testdata/helmhome"),
+		ChartPath: tmp,
+	}
+	//create the /charts subdir
+	chartsFolder := filepath.Join(tmp, "charts")
+	if err != nil {
+		t.Fatal(err)
+	}
+	os.Mkdir(chartsFolder, 0755)
+
+	tests := []struct {
+		name   string
+		req    []*chartutil.Dependency
+		expect map[string]string
+		err    bool
+	}{
+		{
+			name: "sub char relative no dot",
+			req: []*chartutil.Dependency{
+				{Name: "foo", Repository: "file://charts/foo"},
+			},
+		},
+		{
+			name: "sub chart relative with dot",
+			req: []*chartutil.Dependency{
+				{Name: "foo", Repository: "file://./charts/foo"},
+			},
+		},
+		{
+			name: "sub chart absolute",
+			req: []*chartutil.Dependency{
+				{Name: "foo", Repository: "file://" + filepath.Join(chartsFolder, "foo")},
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		//we only check the downloadAll does not return any error.
+		err := m.downloadAll(tt.req)
+		if err != nil {
+			t.Fatal(err)
+		}
+	}
+}

pkg/resolver/resolver.go

@@ -128,8 +128,24 @@ func HashReq(req *chartutil.Requirements) (string, error) {
 }
 
 // GetLocalPath generates absolute local path when use
-// "file://" in repository of requirements
+// "file://" in repository of requirements and check if it exists
 func GetLocalPath(repo string, chartpath string) (string, error) {
+	localPath, err := GenerateLocalPath(repo, chartpath)
+	if err != nil {
+		return "", err
+	}
+	if _, err := os.Stat(localPath); os.IsNotExist(err) {
+		return "", fmt.Errorf("directory %s not found", localPath)
+	} else if err != nil {
+		return "", err
+	}
+
+	return localPath, nil
+}
+
+// GenerateLocalPath generates absolute local path when use
+// "file://" in repository of requirements
+func GenerateLocalPath(repo string, chartpath string) (string, error) {
 	var depPath string
 	var err error
 	p := strings.TrimPrefix(repo, "file://")
@@ -143,11 +159,25 @@ func GetLocalPath(repo string, chartpath string) (string, error) {
 		depPath = filepath.Join(chartpath, p)
 	}
 
-	if _, err = os.Stat(depPath); os.IsNotExist(err) {
+	return depPath, nil
-		return "", fmt.Errorf("directory %s not found", depPath)
+}
-	} else if err != nil {
+
-		return "", err
+//IsLocalSubChart checks if file repo path does belong to the chartpath "/charts" folder
+func IsLocalSubChart(repo string, chartpath string) (bool, error) {
+	if !strings.HasPrefix(repo, "file://") {
+		return false, nil
 	}
+	var isSubChart bool
 
-	return depPath, nil
+	destPath := filepath.Join(chartpath, "charts")
+	origPath, err := GenerateLocalPath(repo, chartpath)
+	if err != nil {
+		return false, err
+	}
+	relPath, err := filepath.Rel(destPath, origPath)
+	if err != nil {
+		return false, err
+	}
+	isSubChart = !strings.HasPrefix(relPath, ".")
+	return isSubChart, nil
 }

pkg/resolver/resolver_test.go

@@ -169,3 +169,58 @@ func TestHashReq(t *testing.T) {
 		t.Errorf("Expected %q !=  %q", expect, h)
 	}
 }
+
+func TestIsSubChart(t *testing.T) {
+	tests := []struct {
+		name      string
+		repo      string
+		chartPath string
+		res       bool
+	}{
+		{
+			name:      "direct subchart",
+			chartPath: "/foo",
+			repo:      "file://charts/bar",
+			res:       true,
+		},
+		{
+			name:      "direct subchart with a dot",
+			chartPath: "/foo",
+			repo:      "file://./charts/bar",
+			res:       true,
+		},
+		{
+			name:      "absolute subchart ",
+			chartPath: "/foo",
+			repo:      "file:///foo/charts/bar",
+			res:       true,
+		},
+		{
+			name:      "absolute not subchart ",
+			chartPath: "/foo",
+			repo:      "file:///bar/charts/bar",
+			res:       false,
+		},
+		{
+			name:      "relative not subchart ",
+			chartPath: "/foo",
+			repo:      "file:///./bar",
+			res:       false,
+		},
+		{
+			name:      "not file:// ",
+			chartPath: "/foo",
+			repo:      "XXX",
+			res:       false,
+		},
+	}
+	for _, tt := range tests {
+		result, err := IsLocalSubChart(tt.repo, tt.chartPath)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if result != tt.res {
+			t.Errorf("Expected %v !=  %v", tt.res, result)
+		}
+	}
+}