Skip to content
This repository has been archived by the owner on Jul 16, 2021. It is now read-only.

enable ability to index new chart data for the same chart version #556

Merged
merged 3 commits into from
Nov 2, 2018
Merged

enable ability to index new chart data for the same chart version #556

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
75af685
enable ability to index new chart data for the same chart version
Nov 1, 2018
5117413
update kubeapps/common dep to pull in fix for tests
Nov 1, 2018
b2f4d0c
add comments
Nov 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Gopkg.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions cmd/chart-repo/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,4 +57,5 @@ type chartFiles struct {
Readme string
Values string
Repo repo
Digest string
}
6 changes: 3 additions & 3 deletions cmd/chart-repo/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,7 @@ 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 {
if err := db.C(chartFilesCollection).Find(bson.M{"_id": chartFilesID, "digest": cv.Digest}).One(&chartFiles{}); err == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Mind adding a comment explaining the logic behind this query?

log.WithFields(log.Fields{"name": name, "version": cv.Version}).Debug("skipping existing files")
return nil
}
Expand Down Expand Up @@ -357,7 +357,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 {
Expand All @@ -369,7 +369,7 @@ 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)
db.C(chartFilesCollection).UpsertId(chartFilesID, chartFiles)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, explaining that if we get to this point is because we either don't have a chart for that version, or we do have it but with different digest.

I personally don't like upserts much in general because its context sometimes can be difficult to understand, also, in some cases it could cause side effects. In any case, a comment like my suggestion above might be enough.

Copy link
Member Author

Choose a reason for hiding this comment

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

in some cases it could cause side effects

I agree in some cases, but I think in this case it should be fine since we are updating the entry with a full struct, and not doing a partial update.


return nil
}
Expand Down
11 changes: 7 additions & 4 deletions cmd/chart-repo/utils_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
})
}

Expand Down
1 change: 1 addition & 0 deletions vendor/github.com/kubeapps/common/datastore/mockstore/mockstore.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.