From 57349ff3d439eb7b8b378d3a17462f6dcca228fc Mon Sep 17 00:00:00 2001 From: Adnan Abdulhussein Date: Fri, 2 Nov 2018 13:24:30 -0700 Subject: [PATCH] enable ability to index new chart data for the same chart version (#556) * enable ability to index new chart data for the same chart version Previously, chart-repo skipped indexing any chart versions previous indexed based on the repo, name and version. However, if a user modifies a chart version without bumping a new version, the changes are not taken into account. This changes chart-repo to skip based on the above AND the digest of the chart package so that packages with a new digest are correctly updated. See also https://github.com/kubeapps/kubeapps/issues/659 fixes #555 Signed-off-by: Adnan Abdulhussein * update kubeapps/common dep to pull in fix for tests Signed-off-by: Adnan Abdulhussein * add comments Signed-off-by: Adnan Abdulhussein --- Gopkg.lock | 4 ++-- cmd/chart-repo/types.go | 1 + cmd/chart-repo/utils.go | 10 +++++++--- cmd/chart-repo/utils_test.go | 11 +++++++---- .../kubeapps/common/datastore/mockstore/mockstore.go | 1 + 5 files changed, 18 insertions(+), 9 deletions(-) diff --git a/Gopkg.lock b/Gopkg.lock index cf4fa72b4..fd2360537 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -135,7 +135,7 @@ [[projects]] branch = "master" - digest = "1:f15cda2356071e51ea44c35e3ca522d3db3f6b539a1bf8eab686095f985416fe" + digest = "1:7373ba404b1bbaacf6c8a4c76a0e97531594873d63eac216d74b0a60efe677ee" name = "github.com/kubeapps/common" packages = [ "datastore", @@ -143,7 +143,7 @@ "response", ] pruneopts = "UT" - revision = "24f6ea203bc21cc1bb3352fbdca9d319dd34b8b0" + revision = "0bcfd8cf936e58615487aa8c13199891372285c9" [[projects]] digest = "1:ff5ebae34cfbf047d505ee150de27e60570e8c394b3b8fdbb720ff6ac71985fc" diff --git a/cmd/chart-repo/types.go b/cmd/chart-repo/types.go index 05b164dca..289d05ce2 100644 --- a/cmd/chart-repo/types.go +++ b/cmd/chart-repo/types.go @@ -57,4 +57,5 @@ type chartFiles struct { Readme string Values string Repo repo + Digest string } diff --git a/cmd/chart-repo/utils.go b/cmd/chart-repo/utils.go index d808240dd..a79f24977 100644 --- a/cmd/chart-repo/utils.go +++ b/cmd/chart-repo/utils.go @@ -315,7 +315,9 @@ func fetchAndImportFiles(dbSession datastore.Session, name string, r repo, cv ch chartFilesID := fmt.Sprintf("%s/%s-%s", r.Name, name, cv.Version) db, closer := dbSession.DB() defer closer() - if err := db.C(chartFilesCollection).FindId(chartFilesID).One(&chartFiles{}); err == nil { + + // Check if we already have indexed files for this chart version and digest + if err := db.C(chartFilesCollection).Find(bson.M{"_id": chartFilesID, "digest": cv.Digest}).One(&chartFiles{}); err == nil { log.WithFields(log.Fields{"name": name, "version": cv.Version}).Debug("skipping existing files") return nil } @@ -357,7 +359,7 @@ func fetchAndImportFiles(dbSession datastore.Session, name string, r repo, cv ch return err } - chartFiles := chartFiles{ID: chartFilesID, Repo: r} + chartFiles := chartFiles{ID: chartFilesID, Repo: r, Digest: cv.Digest} if v, ok := files[readmeFileName]; ok { chartFiles.Readme = v } else { @@ -369,7 +371,9 @@ func fetchAndImportFiles(dbSession datastore.Session, name string, r repo, cv ch log.WithFields(log.Fields{"name": name, "version": cv.Version}).Info("values.yaml not found") } - db.C(chartFilesCollection).Insert(chartFiles) + // inserts the chart files if not already indexed, or updates the existing + // entry if digest has changed + db.C(chartFilesCollection).UpsertId(chartFilesID, chartFiles) return nil } diff --git a/cmd/chart-repo/utils_test.go b/cmd/chart-repo/utils_test.go index 24f095dbb..e530969f7 100644 --- a/cmd/chart-repo/utils_test.go +++ b/cmd/chart-repo/utils_test.go @@ -346,7 +346,8 @@ func Test_fetchAndImportFiles(t *testing.T) { netClient = &goodTarballClient{c: charts[0], skipValues: true, skipReadme: true} m := mock.Mock{} m.On("One", mock.Anything).Return(errors.New("return an error when checking if files already exists to force fetching")) - m.On("Insert", chartFiles{fmt.Sprintf("%s/%s-%s", charts[0].Repo.Name, charts[0].Name, cv.Version), "", "", charts[0].Repo}) + chartFilesID := fmt.Sprintf("%s/%s-%s", charts[0].Repo.Name, charts[0].Name, cv.Version) + m.On("UpsertId", chartFilesID, chartFiles{chartFilesID, "", "", charts[0].Repo, cv.Digest}) dbSession := mockstore.NewMockSession(&m) err := fetchAndImportFiles(dbSession, charts[0].Name, charts[0].Repo, cv) assert.NoErr(t, err) @@ -357,7 +358,8 @@ func Test_fetchAndImportFiles(t *testing.T) { netClient = &authenticatedTarballClient{c: charts[0]} m := mock.Mock{} m.On("One", mock.Anything).Return(errors.New("return an error when checking if files already exists to force fetching")) - m.On("Insert", chartFiles{fmt.Sprintf("%s/%s-%s", charts[0].Repo.Name, charts[0].Name, cv.Version), testChartReadme, testChartValues, charts[0].Repo}) + chartFilesID := fmt.Sprintf("%s/%s-%s", charts[0].Repo.Name, charts[0].Name, cv.Version) + m.On("UpsertId", chartFilesID, chartFiles{chartFilesID, testChartReadme, testChartValues, charts[0].Repo, cv.Digest}) dbSession := mockstore.NewMockSession(&m) err := fetchAndImportFiles(dbSession, charts[0].Name, charts[0].Repo, cv) assert.NoErr(t, err) @@ -368,7 +370,8 @@ func Test_fetchAndImportFiles(t *testing.T) { netClient = &goodTarballClient{c: charts[0]} m := mock.Mock{} m.On("One", mock.Anything).Return(errors.New("return an error when checking if files already exists to force fetching")) - m.On("Insert", chartFiles{fmt.Sprintf("%s/%s-%s", charts[0].Repo.Name, charts[0].Name, cv.Version), testChartReadme, testChartValues, charts[0].Repo}) + chartFilesID := fmt.Sprintf("%s/%s-%s", charts[0].Repo.Name, charts[0].Name, cv.Version) + m.On("UpsertId", chartFilesID, chartFiles{chartFilesID, testChartReadme, testChartValues, charts[0].Repo, cv.Digest}) dbSession := mockstore.NewMockSession(&m) err := fetchAndImportFiles(dbSession, charts[0].Name, charts[0].Repo, cv) assert.NoErr(t, err) @@ -382,7 +385,7 @@ func Test_fetchAndImportFiles(t *testing.T) { dbSession := mockstore.NewMockSession(&m) err := fetchAndImportFiles(dbSession, charts[0].Name, charts[0].Repo, cv) assert.NoErr(t, err) - m.AssertNotCalled(t, "Insert", mock.Anything) + m.AssertNotCalled(t, "UpsertId", mock.Anything, mock.Anything) }) } diff --git a/vendor/github.com/kubeapps/common/datastore/mockstore/mockstore.go b/vendor/github.com/kubeapps/common/datastore/mockstore/mockstore.go index 38dc2a8cd..1f9d71c6f 100644 --- a/vendor/github.com/kubeapps/common/datastore/mockstore/mockstore.go +++ b/vendor/github.com/kubeapps/common/datastore/mockstore/mockstore.go @@ -52,6 +52,7 @@ func (c mockCollection) Upsert(selector interface{}, update interface{}) (*mgo.C } func (c mockCollection) UpsertId(selector interface{}, update interface{}) (*mgo.ChangeInfo, error) { + c.Called(selector, update) return nil, nil }