From ad5da035b39db3a14e75d9f70afae80f3a1609b9 Mon Sep 17 00:00:00 2001 From: Nick Cabatoff Date: Sun, 13 May 2018 13:22:16 -0400 Subject: [PATCH 1/3] Fix #1067 by filtering out chart dirs from manifest loading of changed files. --- cluster/kubernetes/manifests.go | 4 ++++ cluster/kubernetes/resource/load.go | 4 ++-- cluster/manifests.go | 2 ++ daemon/loop.go | 34 +++++++++++++++++++++++++++++ 4 files changed, 42 insertions(+), 2 deletions(-) diff --git a/cluster/kubernetes/manifests.go b/cluster/kubernetes/manifests.go index 3edb7bbc3..e86c435a3 100644 --- a/cluster/kubernetes/manifests.go +++ b/cluster/kubernetes/manifests.go @@ -23,4 +23,8 @@ func (c *Manifests) UpdateDefinition(def []byte, container string, image image.R return updatePodController(def, container, image) } +func (c *Manifests) LooksLikeChart(path string) bool { + return kresource.LooksLikeChart(path) +} + // UpdatePolicies and ServicesWithPolicies in policies.go diff --git a/cluster/kubernetes/resource/load.go b/cluster/kubernetes/resource/load.go index bf23add03..aacd5883b 100644 --- a/cluster/kubernetes/resource/load.go +++ b/cluster/kubernetes/resource/load.go @@ -24,7 +24,7 @@ func Load(base, atLeastOne string, more ...string) (map[string]resource.Resource return errors.Wrapf(err, "walking %q for yamels", path) } - if info.IsDir() && looksLikeChart(path) { + if info.IsDir() && LooksLikeChart(path) { return filepath.SkipDir } @@ -60,7 +60,7 @@ func Load(base, atLeastOne string, more ...string) (map[string]resource.Resource // looksLikeChart returns `true` if the path `dir` (assumed to be a // directory) looks like it contains a Helm chart, rather than // manifest files. -func looksLikeChart(dir string) bool { +func LooksLikeChart(dir string) bool { // These are the two mandatory parts of a chart. If they both // exist, chances are it's a chart. See // https://github.com/kubernetes/helm/blob/master/docs/charts.md#the-chart-file-structure diff --git a/cluster/manifests.go b/cluster/manifests.go index 643bd0852..6485c3895 100644 --- a/cluster/manifests.go +++ b/cluster/manifests.go @@ -32,6 +32,8 @@ type Manifests interface { UpdatePolicies([]byte, policy.Update) ([]byte, error) // ServicesWithPolicies returns all services with their associated policies ServicesWithPolicies(path string) (policy.ResourceMap, error) + // LooksLikeChart returns true if the given directory path appears to be a Helm chart. + LooksLikeChart(path string) bool } // UpdateManifest looks for the manifest for a given service, reads diff --git a/daemon/loop.go b/daemon/loop.go index b752d5ef4..35e6f99a5 100644 --- a/daemon/loop.go +++ b/daemon/loop.go @@ -2,6 +2,8 @@ package daemon import ( "fmt" + "os" + "path/filepath" "strings" "time" @@ -242,6 +244,10 @@ func (d *Daemon) doSync(logger log.Logger) (retErr error) { if err == nil && len(changedFiles) > 0 { // We had some changed files, we're syncing a diff // FIXME(michael): this won't be accurate when a file can have more than one resource + filtered, err := d.filterOutChartDirs(working.Dir(), changedFiles) + if err == nil && len(filtered) > 0 { + changedResources, err = d.Manifests.LoadManifests(working.Dir(), filtered[0], filtered[1:]...) + } changedResources, err = d.Manifests.LoadManifests(working.Dir(), changedFiles[0], changedFiles[1:]...) } cancel() @@ -413,3 +419,31 @@ func isUnknownRevision(err error) bool { (strings.Contains(err.Error(), "unknown revision or path not in the working tree.") || strings.Contains(err.Error(), "bad revision")) } + +func (d *Daemon) filterOutChartDirs(root string, changedFiles []string) ([]string, error) { + var chartdirs = make(map[string]bool) + err := filepath.Walk(root, func(path string, info os.FileInfo, err error) error { + if err != nil { + return errors.Wrapf(err, "walking %q for charts", path) + } + + if info.IsDir() && d.Manifests.LooksLikeChart(path) { + chartdirs[path] = true + return filepath.SkipDir + } + + return nil + }) + if err != nil { + return []string{}, err + } + + var filtered []string + for _, f := range changedFiles { + dirname := filepath.Dir(f) + if !chartdirs[dirname] && !chartdirs[filepath.Dir(dirname)] { + filtered = append(filtered, f) + } + } + return filtered, nil +} From 485ee663f51ef489cd0b59d4a8634e62c56b9b72 Mon Sep 17 00:00:00 2001 From: Nick Cabatoff Date: Tue, 15 May 2018 19:37:28 -0400 Subject: [PATCH 2/3] Hide the chart-ignoring details inside Load. Add tests. --- cluster/kubernetes/manifests.go | 4 -- cluster/kubernetes/resource/load.go | 51 +++++++++++++++++++++++- cluster/kubernetes/resource/load_test.go | 49 +++++++++++++++++++++++ cluster/manifests.go | 2 - daemon/loop.go | 34 ---------------- 5 files changed, 98 insertions(+), 42 deletions(-) diff --git a/cluster/kubernetes/manifests.go b/cluster/kubernetes/manifests.go index e86c435a3..3edb7bbc3 100644 --- a/cluster/kubernetes/manifests.go +++ b/cluster/kubernetes/manifests.go @@ -23,8 +23,4 @@ func (c *Manifests) UpdateDefinition(def []byte, container string, image image.R return updatePodController(def, container, image) } -func (c *Manifests) LooksLikeChart(path string) bool { - return kresource.LooksLikeChart(path) -} - // UpdatePolicies and ServicesWithPolicies in policies.go diff --git a/cluster/kubernetes/resource/load.go b/cluster/kubernetes/resource/load.go index aacd5883b..3b24294eb 100644 --- a/cluster/kubernetes/resource/load.go +++ b/cluster/kubernetes/resource/load.go @@ -18,16 +18,24 @@ import ( func Load(base, atLeastOne string, more ...string) (map[string]resource.Resource, error) { roots := append([]string{atLeastOne}, more...) objs := map[string]resource.Resource{} + charts, err := newChartTracker(base) + if err != nil { + return nil, errors.Wrapf(err, "walking %q for chartdirs", base) + } for _, root := range roots { err := filepath.Walk(root, func(path string, info os.FileInfo, err error) error { if err != nil { return errors.Wrapf(err, "walking %q for yamels", path) } - if info.IsDir() && LooksLikeChart(path) { + if charts.isChart(path) { return filepath.SkipDir } + if charts.inChart(path) { + return nil + } + if !info.IsDir() && filepath.Ext(path) == ".yaml" || filepath.Ext(path) == ".yml" { bytes, err := ioutil.ReadFile(path) if err != nil { @@ -57,10 +65,49 @@ func Load(base, atLeastOne string, more ...string) (map[string]resource.Resource return objs, nil } +type chartTracker map[string]bool + +func newChartTracker(root string) (chartTracker, error) { + var chartdirs = make(map[string]bool) + err := filepath.Walk(root, func(path string, info os.FileInfo, err error) error { + if err != nil { + return errors.Wrapf(err, "walking %q for charts", path) + } + + if info.IsDir() && looksLikeChart(path) { + chartdirs[path] = true + return filepath.SkipDir + } + + return nil + }) + if err != nil { + return nil, err + } + + return chartTracker(chartdirs), nil +} + +func (c chartTracker) isChart(path string) bool { + return c[path] +} + +func (c chartTracker) inChart(path string) bool { + p := path + root := fmt.Sprintf("%c", filepath.Separator) + for p != root { + if c[p] { + return true + } + p = filepath.Dir(p) + } + return false +} + // looksLikeChart returns `true` if the path `dir` (assumed to be a // directory) looks like it contains a Helm chart, rather than // manifest files. -func LooksLikeChart(dir string) bool { +func looksLikeChart(dir string) bool { // These are the two mandatory parts of a chart. If they both // exist, chances are it's a chart. See // https://github.com/kubernetes/helm/blob/master/docs/charts.md#the-chart-file-structure diff --git a/cluster/kubernetes/resource/load_test.go b/cluster/kubernetes/resource/load_test.go index 22ee749fa..a454b9192 100644 --- a/cluster/kubernetes/resource/load_test.go +++ b/cluster/kubernetes/resource/load_test.go @@ -2,6 +2,7 @@ package resource import ( "bytes" + "path/filepath" "reflect" "testing" @@ -140,3 +141,51 @@ func TestLoadSome(t *testing.T) { t.Errorf("expected %d objects from %d files, got result:\n%#v", len(testfiles.ServiceMap(dir)), len(testfiles.Files), objs) } } + +func TestChartTracker(t *testing.T) { + dir, cleanup := testfiles.TempDir(t) + defer cleanup() + if err := testfiles.WriteTestFiles(dir); err != nil { + t.Fatal(err) + } + + ct, err := newChartTracker(dir) + if err != nil { + t.Fatal(err) + } + + noncharts := []string{"garbage", "locked-service-deploy.yaml", + "test", "test/test-service-deploy.yaml"} + for _, f := range noncharts { + fq := filepath.Join(dir, f) + if ct.isChart(fq) { + t.Errorf("%q thought to be a chart", f) + } + if f == "garbage" { + continue + } + if m, err := Load(dir, fq); err != nil || len(m) == 0 { + t.Errorf("Load returned 0 objs, err=%v", err) + } + } + if !ct.isChart(filepath.Join(dir, "charts/nginx")) { + t.Errorf("charts/nginx not recognized as chart") + } + if !ct.inChart(filepath.Join(dir, "charts/nginx/Chart.yaml")) { + t.Errorf("charts/nginx/Chart.yaml not recognized as in chart") + } + + chartfiles := []string{"charts", + "charts/nginx", + "charts/nginx/Chart.yaml", + "charts/nginx/values.yaml", + "charts/nginx/templates/deployment.yaml", + } + for _, f := range chartfiles { + fq := filepath.Join(dir, f) + if m, err := Load(dir, fq); err != nil || len(m) != 0 { + t.Errorf("%q not ignored as a chart should be", f) + } + } + +} diff --git a/cluster/manifests.go b/cluster/manifests.go index 6485c3895..643bd0852 100644 --- a/cluster/manifests.go +++ b/cluster/manifests.go @@ -32,8 +32,6 @@ type Manifests interface { UpdatePolicies([]byte, policy.Update) ([]byte, error) // ServicesWithPolicies returns all services with their associated policies ServicesWithPolicies(path string) (policy.ResourceMap, error) - // LooksLikeChart returns true if the given directory path appears to be a Helm chart. - LooksLikeChart(path string) bool } // UpdateManifest looks for the manifest for a given service, reads diff --git a/daemon/loop.go b/daemon/loop.go index 35e6f99a5..b752d5ef4 100644 --- a/daemon/loop.go +++ b/daemon/loop.go @@ -2,8 +2,6 @@ package daemon import ( "fmt" - "os" - "path/filepath" "strings" "time" @@ -244,10 +242,6 @@ func (d *Daemon) doSync(logger log.Logger) (retErr error) { if err == nil && len(changedFiles) > 0 { // We had some changed files, we're syncing a diff // FIXME(michael): this won't be accurate when a file can have more than one resource - filtered, err := d.filterOutChartDirs(working.Dir(), changedFiles) - if err == nil && len(filtered) > 0 { - changedResources, err = d.Manifests.LoadManifests(working.Dir(), filtered[0], filtered[1:]...) - } changedResources, err = d.Manifests.LoadManifests(working.Dir(), changedFiles[0], changedFiles[1:]...) } cancel() @@ -419,31 +413,3 @@ func isUnknownRevision(err error) bool { (strings.Contains(err.Error(), "unknown revision or path not in the working tree.") || strings.Contains(err.Error(), "bad revision")) } - -func (d *Daemon) filterOutChartDirs(root string, changedFiles []string) ([]string, error) { - var chartdirs = make(map[string]bool) - err := filepath.Walk(root, func(path string, info os.FileInfo, err error) error { - if err != nil { - return errors.Wrapf(err, "walking %q for charts", path) - } - - if info.IsDir() && d.Manifests.LooksLikeChart(path) { - chartdirs[path] = true - return filepath.SkipDir - } - - return nil - }) - if err != nil { - return []string{}, err - } - - var filtered []string - for _, f := range changedFiles { - dirname := filepath.Dir(f) - if !chartdirs[dirname] && !chartdirs[filepath.Dir(dirname)] { - filtered = append(filtered, f) - } - } - return filtered, nil -} From aefd36b2e30cc03fc23c70d40266fe4f6c6ad882 Mon Sep 17 00:00:00 2001 From: Nick Cabatoff Date: Thu, 17 May 2018 19:51:19 -0400 Subject: [PATCH 3/3] Rename methods to be less alike so as to avoid confusion. --- cluster/kubernetes/resource/load.go | 8 ++++---- cluster/kubernetes/resource/load_test.go | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/cluster/kubernetes/resource/load.go b/cluster/kubernetes/resource/load.go index 3b24294eb..46d8c0476 100644 --- a/cluster/kubernetes/resource/load.go +++ b/cluster/kubernetes/resource/load.go @@ -28,11 +28,11 @@ func Load(base, atLeastOne string, more ...string) (map[string]resource.Resource return errors.Wrapf(err, "walking %q for yamels", path) } - if charts.isChart(path) { + if charts.isDirChart(path) { return filepath.SkipDir } - if charts.inChart(path) { + if charts.isPathInChart(path) { return nil } @@ -88,11 +88,11 @@ func newChartTracker(root string) (chartTracker, error) { return chartTracker(chartdirs), nil } -func (c chartTracker) isChart(path string) bool { +func (c chartTracker) isDirChart(path string) bool { return c[path] } -func (c chartTracker) inChart(path string) bool { +func (c chartTracker) isPathInChart(path string) bool { p := path root := fmt.Sprintf("%c", filepath.Separator) for p != root { diff --git a/cluster/kubernetes/resource/load_test.go b/cluster/kubernetes/resource/load_test.go index a454b9192..dfa916ea9 100644 --- a/cluster/kubernetes/resource/load_test.go +++ b/cluster/kubernetes/resource/load_test.go @@ -158,7 +158,7 @@ func TestChartTracker(t *testing.T) { "test", "test/test-service-deploy.yaml"} for _, f := range noncharts { fq := filepath.Join(dir, f) - if ct.isChart(fq) { + if ct.isDirChart(fq) { t.Errorf("%q thought to be a chart", f) } if f == "garbage" { @@ -168,10 +168,10 @@ func TestChartTracker(t *testing.T) { t.Errorf("Load returned 0 objs, err=%v", err) } } - if !ct.isChart(filepath.Join(dir, "charts/nginx")) { + if !ct.isDirChart(filepath.Join(dir, "charts/nginx")) { t.Errorf("charts/nginx not recognized as chart") } - if !ct.inChart(filepath.Join(dir, "charts/nginx/Chart.yaml")) { + if !ct.isPathInChart(filepath.Join(dir, "charts/nginx/Chart.yaml")) { t.Errorf("charts/nginx/Chart.yaml not recognized as in chart") }