Skip to content

Commit

Permalink
Update removeMissingCharts to remove from a namespaced repo only. (#1538
Browse files Browse the repository at this point in the history 
)

* Update removeMissingCharts to remove from a namespaced repo only.

* Skip pg test if no pg

* Ensure insertFiles is called with chart ID, not just chart name.
  • Loading branch information
@absoludity
absoludity authored Feb 26, 2020
1 parent 98f34a9 commit e0371c2
Show file tree
Hide file tree
Showing 7 changed files with 224 additions and 74 deletions.
6 changes: 3 additions & 3 deletions cmd/asset-syncer/mongodb_utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ func newMongoDBManager(config datastore.Config) assetManager {
// These steps are processed in this way to ensure relevant chart data is
// imported into the database as fast as possible. E.g. we want all icons for
// charts before fetching readmes for each chart and version pair.
func (m *mongodbAssetManager) Sync(repo models.RepoInternal, charts []models.Chart) error {
func (m *mongodbAssetManager) Sync(repo models.Repo, charts []models.Chart) error {
return m.importCharts(charts)
}

Expand Down Expand Up @@ -132,10 +132,10 @@ func (m *mongodbAssetManager) filesExist(chartFilesID, digest string) bool {
return err == nil
}

func (m *mongodbAssetManager) insertFiles(chartFilesID string, files models.ChartFiles) error {
func (m *mongodbAssetManager) insertFiles(chartId string, files models.ChartFiles) error {
db, closer := m.DBSession.DB()
defer closer()
_, err := db.C(chartFilesCollection).UpsertId(chartFilesID, files)
_, err := db.C(chartFilesCollection).UpsertId(files.ID, files)
return err
}

Expand Down
233 changes: 189 additions & 44 deletions cmd/asset-syncer/postgresql_db_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,65 +110,88 @@ func TestImportCharts(t *testing.T) {

func TestInsertFiles(t *testing.T) {
dbutilstest.SkipIfNoPostgres(t)
const namespace = "my-namespace"
const (
namespace = "my-namespace"
chartId = "my-chart-id"
filesId = chartId + "-1.0"
)

testCases := []struct {
name string
existingFiles []models.ChartFiles
chartFiles models.ChartFiles
filesInserted int
fileCount int
}{
{
name: "it inserts new chart files",
chartFiles: models.ChartFiles{ID: "repo/chart-1.8", Readme: "A Readme", Repo: &models.Repo{Namespace: namespace}},
filesInserted: 1,
name: "it inserts new chart files",
chartFiles: models.ChartFiles{ID: filesId, Readme: "A Readme", Repo: &models.Repo{Namespace: namespace}},
fileCount: 1,
},
{
name: "it updates existing chart files",
existingFiles: []models.ChartFiles{
models.ChartFiles{ID: filesId, Readme: "A Readme", Repo: &models.Repo{Namespace: namespace}},
},
chartFiles: models.ChartFiles{ID: filesId, Readme: "A New Readme", Repo: &models.Repo{Namespace: namespace}},
fileCount: 1,
},
{
name: "it imports the same repo name and chart version in different namespaces",
existingFiles: []models.ChartFiles{
models.ChartFiles{ID: "repo/chart-1.8", Readme: "A different Readme", Repo: &models.Repo{Namespace: "another-namespace"}},
models.ChartFiles{ID: filesId, Readme: "A different Readme", Repo: &models.Repo{Namespace: "another-namespace"}},
},
chartFiles: models.ChartFiles{ID: "repo/chart-1.8", Readme: "A Readme", Repo: &models.Repo{Namespace: namespace}},
filesInserted: 2,
chartFiles: models.ChartFiles{ID: filesId, Readme: "A Readme", Repo: &models.Repo{Namespace: namespace}},
fileCount: 2,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
pam, cleanup := getInitializedManager(t)
defer cleanup()
for _, files := range tc.existingFiles {
_, err := pam.EnsureRepoExists(files.Repo.Namespace, files.Repo.Name)
if err != nil {
t.Fatalf("%+v", err)
}
err = pam.insertFiles("some-id", files)
if err != nil {
t.Fatalf("%+v", err)
}
}
_, err := pam.EnsureRepoExists(tc.chartFiles.Repo.Namespace, tc.chartFiles.Repo.Name)
if err != nil {
t.Fatalf("%+v", err)
}
ensureFilesExist(t, pam, chartId, tc.existingFiles)

ensureChartExists(t, pam, models.Chart{ID: chartId, Repo: tc.chartFiles.Repo})

err = pam.insertFiles("some-id", tc.chartFiles)
err := pam.insertFiles(chartId, tc.chartFiles)
if err != nil {
t.Errorf("%+v", err)
}

if got, want := countTable(t, pam, dbutils.ChartFilesTable), tc.filesInserted; got != want {
if got, want := countTable(t, pam, dbutils.ChartFilesTable), tc.fileCount; got != want {
t.Errorf("got: %d, want: %d", got, want)
}
})
}
}

func ensureChartExists(t *testing.T, pam *postgresAssetManager, chart models.Chart) {
_, err := pam.EnsureRepoExists(chart.Repo.Namespace, chart.Repo.Name)
if err != nil {
t.Fatalf("%+v", err)
}
err = pam.importCharts([]models.Chart{chart}, *chart.Repo)
if err != nil {
t.Fatalf("%+v", err)
}
}

func ensureFilesExist(t *testing.T, pam *postgresAssetManager, chartId string, files []models.ChartFiles) {
for _, f := range files {
ensureChartExists(t, pam, models.Chart{ID: chartId, Repo: f.Repo})
err := pam.insertFiles(chartId, f)
if err != nil {
t.Fatalf("%+v", err)
}
}
}

func TestDelete(t *testing.T) {
dbutilstest.SkipIfNoPostgres(t)
const (
repoNamespace = "my-namespace"
repoName = "my-repo"
chartId = repoName + "/my-chart"
)
repoToDelete := models.Repo{Namespace: repoNamespace, Name: repoName}
otherRepo := models.Repo{Namespace: "other-namespace", Name: repoName}
Expand All @@ -184,7 +207,7 @@ func TestDelete(t *testing.T) {
{
name: "it deletes the repo, chart and files",
existingFiles: []models.ChartFiles{
models.ChartFiles{ID: "repo/chart-1.8", Readme: "A Readme", Repo: &repoToDelete},
models.ChartFiles{ID: chartId + "-1.8", Readme: "A Readme", Repo: &repoToDelete},
},
repo: repoToDelete,
expectedRepos: 0,
Expand All @@ -194,8 +217,8 @@ func TestDelete(t *testing.T) {
{
name: "it deletes the repo, chart and files from that namespace only",
existingFiles: []models.ChartFiles{
models.ChartFiles{ID: "repo/chart-1.8", Readme: "A Readme", Repo: &repoToDelete},
models.ChartFiles{ID: "repo/chart-1.8", Readme: "A Readme", Repo: &otherRepo},
models.ChartFiles{ID: chartId + "-1.8", Readme: "A Readme", Repo: &repoToDelete},
models.ChartFiles{ID: chartId + "-1.8", Readme: "A Readme", Repo: &otherRepo},
},
repo: repoToDelete,
expectedRepos: 1,
Expand All @@ -208,23 +231,7 @@ func TestDelete(t *testing.T) {
t.Run(tc.name, func(t *testing.T) {
pam, cleanup := getInitializedManager(t)
defer cleanup()
for _, files := range tc.existingFiles {
// Ensure the repo and chart exists before creating the files.
_, err := pam.EnsureRepoExists(files.Repo.Namespace, files.Repo.Name)
if err != nil {
t.Fatalf("%+v", err)
}
err = pam.importCharts([]models.Chart{
models.Chart{Repo: files.Repo},
}, *files.Repo)
if err != nil {
t.Fatalf("%+v", err)
}
err = pam.insertFiles("some-id", files)
if err != nil {
t.Fatalf("%+v", err)
}
}
ensureFilesExist(t, pam, chartId, tc.existingFiles)

err := pam.Delete(repoToDelete)
if err != nil {
Expand All @@ -243,3 +250,141 @@ func TestDelete(t *testing.T) {
})
}
}

func TestRemoveMissingCharts(t *testing.T) {
dbutilstest.SkipIfNoPostgres(t)
const (
repoName = "my-repo"
)
repo := models.Repo{Name: repoName, Namespace: "my-namespace"}
repoOtherNameSameNamespace := models.Repo{Name: "other-repo", Namespace: "my-namespace"}
repoSameNameOtherNamespace := models.Repo{Name: repoName, Namespace: "other-namespace"}

testCases := []struct {
name string
// existingFiles maps a chartId to a slice of files for different
// versions of that chart.
existingFiles map[string][]models.ChartFiles
remainingCharts []models.Chart
expectedCharts int
expectedFiles int
}{
{
name: "it removes missing charts and files",
existingFiles: map[string][]models.ChartFiles{
"my-chart": {
models.ChartFiles{ID: "my-chart-1", Readme: "A Readme", Repo: &repo},
},
"other-chart": {
models.ChartFiles{ID: "other-chart-1", Readme: "A Readme", Repo: &repo},
models.ChartFiles{ID: "other-chart-2", Readme: "A Readme", Repo: &repo},
models.ChartFiles{ID: "other-chart-3", Readme: "A Readme", Repo: &repo},
},
},
remainingCharts: []models.Chart{
models.Chart{ID: "my-chart"},
},
expectedCharts: 1,
expectedFiles: 1,
},
{
name: "it leaves two charts while removing one",
existingFiles: map[string][]models.ChartFiles{
"my-chart": {
models.ChartFiles{ID: "my-chart-1", Readme: "A Readme", Repo: &repo},
},
"other-chart": {
models.ChartFiles{ID: "other-chart-1", Readme: "A Readme", Repo: &repo},
models.ChartFiles{ID: "other-chart-2", Readme: "A Readme", Repo: &repo},
models.ChartFiles{ID: "other-chart-3", Readme: "A Readme", Repo: &repo},
},
"third-chart": {
models.ChartFiles{ID: "third-chart-1", Readme: "A Readme", Repo: &repo},
models.ChartFiles{ID: "third-chart-2", Readme: "A Readme", Repo: &repo},
models.ChartFiles{ID: "third-chart-3", Readme: "A Readme", Repo: &repo},
},
},
remainingCharts: []models.Chart{
models.Chart{ID: "my-chart"},
models.Chart{ID: "other-chart"},
},
expectedCharts: 2,
expectedFiles: 4,
},
{
name: "it leaves the same chart in a different repo of same namespace",
// None of the charts or files are removed because my-chart and third-chart
// are the only charts in the specific repo.
existingFiles: map[string][]models.ChartFiles{
"my-chart": {
models.ChartFiles{ID: "my-chart-1", Readme: "A Readme", Repo: &repo},
},
"other-chart": {
models.ChartFiles{ID: "other-chart-1", Readme: "A Readme", Repo: &repoOtherNameSameNamespace},
models.ChartFiles{ID: "other-chart-2", Readme: "A Readme", Repo: &repoOtherNameSameNamespace},
models.ChartFiles{ID: "other-chart-3", Readme: "A Readme", Repo: &repoOtherNameSameNamespace},
},
"third-chart": {
models.ChartFiles{ID: "third-chart-1", Readme: "A Readme", Repo: &repo},
models.ChartFiles{ID: "third-chart-2", Readme: "A Readme", Repo: &repo},
models.ChartFiles{ID: "third-chart-3", Readme: "A Readme", Repo: &repo},
},
},
remainingCharts: []models.Chart{
models.Chart{ID: "my-chart"},
models.Chart{ID: "third-chart"},
},
expectedCharts: 3,
expectedFiles: 7,
},
{
name: "it leaves the same chart in a repo in a different",
// None of the charts or files are removed because my-chart and third-chart
// are the only charts in the specific repo.
existingFiles: map[string][]models.ChartFiles{
"my-chart": {
models.ChartFiles{ID: "my-chart-1", Readme: "A Readme", Repo: &repo},
},
"other-chart": {
models.ChartFiles{ID: "other-chart-1", Readme: "A Readme", Repo: &repoSameNameOtherNamespace},
models.ChartFiles{ID: "other-chart-2", Readme: "A Readme", Repo: &repoSameNameOtherNamespace},
models.ChartFiles{ID: "other-chart-3", Readme: "A Readme", Repo: &repoSameNameOtherNamespace},
},
"third-chart": {
models.ChartFiles{ID: "third-chart-1", Readme: "A Readme", Repo: &repo},
models.ChartFiles{ID: "third-chart-2", Readme: "A Readme", Repo: &repo},
models.ChartFiles{ID: "third-chart-3", Readme: "A Readme", Repo: &repo},
},
},
remainingCharts: []models.Chart{
models.Chart{ID: "my-chart"},
models.Chart{ID: "third-chart"},
},
expectedCharts: 3,
expectedFiles: 7,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
pam, cleanup := getInitializedManager(t)
defer cleanup()
for chartId, files := range tc.existingFiles {
ensureFilesExist(t, pam, chartId, files)
}

err := pam.removeMissingCharts(repo, tc.remainingCharts)
if err != nil {
t.Fatalf("%+v", err)
}

if got, want := countTable(t, pam, dbutils.ChartTable), tc.expectedCharts; got != want {
t.Errorf("got: %d, want: %d", got, want)
}
if got, want := countTable(t, pam, dbutils.ChartFilesTable), tc.expectedFiles; got != want {
t.Errorf("got: %d, want: %d", got, want)
}
})
}

}
20 changes: 10 additions & 10 deletions cmd/asset-syncer/postgresql_utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func newPGManager(config datastore.Config) (assetManager, error) {
// These steps are processed in this way to ensure relevant chart data is
// imported into the database as fast as possible. E.g. we want all icons for
// charts before fetching readmes for each chart and version pair.
func (m *postgresAssetManager) Sync(repo models.RepoInternal, charts []models.Chart) error {
func (m *postgresAssetManager) Sync(repo models.Repo, charts []models.Chart) error {
m.InitTables()

// Ensure the repo exists so FK constraints will be met.
Expand All @@ -59,13 +59,13 @@ func (m *postgresAssetManager) Sync(repo models.RepoInternal, charts []models.Ch
return err
}

err = m.importCharts(charts, models.Repo{Namespace: repo.Namespace, Name: repo.Name})
err = m.importCharts(charts, repo)
if err != nil {
return err
}

// Remove charts no longer existing in index
return m.removeMissingCharts(charts)
return m.removeMissingCharts(repo, charts)
}

func (m *postgresAssetManager) RepoAlreadyProcessed(repoName, repoChecksum string) bool {
Expand Down Expand Up @@ -110,13 +110,13 @@ func (m *postgresAssetManager) importCharts(charts []models.Chart, repo models.R
return nil
}

func (m *postgresAssetManager) removeMissingCharts(charts []models.Chart) error {
func (m *postgresAssetManager) removeMissingCharts(repo models.Repo, charts []models.Chart) error {
var chartIDs []string
for _, chart := range charts {
chartIDs = append(chartIDs, fmt.Sprintf("'%s'", chart.ID))
}
chartIDsString := strings.Join(chartIDs, ", ")
rows, err := m.DB.Query(fmt.Sprintf("DELETE FROM %s WHERE info ->> 'ID' NOT IN (%s) AND info -> 'repo' ->> 'name' = $1", dbutils.ChartTable, chartIDsString), charts[0].Repo.Name)
rows, err := m.DB.Query(fmt.Sprintf("DELETE FROM %s WHERE chart_id NOT IN (%s) AND repo_name = $1 AND repo_namespace = $2", dbutils.ChartTable, chartIDsString), repo.Name, repo.Namespace)
if rows != nil {
defer rows.Close()
}
Expand Down Expand Up @@ -156,16 +156,16 @@ func (m *postgresAssetManager) filesExist(chartFilesID, digest string) bool {
return err == nil && hasEntries
}

func (m *postgresAssetManager) insertFiles(chartFilesID string, files models.ChartFiles) error {
func (m *postgresAssetManager) insertFiles(chartId string, files models.ChartFiles) error {
if files.Repo == nil {
return fmt.Errorf("unable to insert file without repo: %q", files.ID)
}
query := fmt.Sprintf(`INSERT INTO %s (repo_name, repo_namespace, chart_files_ID, info)
VALUES ($1, $2, $3, $4)
query := fmt.Sprintf(`INSERT INTO %s (chart_id, repo_name, repo_namespace, chart_files_ID, info)
VALUES ($1, $2, $3, $4, $5)
ON CONFLICT (repo_namespace, chart_files_ID)
DO UPDATE SET info = $4
DO UPDATE SET info = $5
`, dbutils.ChartFilesTable)
rows, err := m.DB.Query(query, files.Repo.Name, files.Repo.Namespace, chartFilesID, files)
rows, err := m.DB.Query(query, chartId, files.Repo.Name, files.Repo.Namespace, files.ID, files)
if rows != nil {
defer rows.Close()
}
Expand Down
Loading

0 comments on commit e0371c2

Please sign in to comment.