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

Added check for empty charts in chart-repo #604

Merged
merged 3 commits into from
Mar 7, 2019
Merged

Added check for empty charts in chart-repo #604

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a005c40
Signed-off-by: Himanshu Pandey <[email protected]>
Feb 22, 2019
5355db7
Added Test case for Empty Charts
Mar 1, 2019
895e3d6
Corrected the test case
Mar 1, 2019
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: 4 additions & 0 deletions cmd/chart-repo/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,10 @@ func syncRepo(dbSession datastore.Session, repoName, repoURL string, authorizati
}

charts := chartsFromIndex(index, r)
if len(charts) == 0 {
log.Fatal("No charts are found")
Copy link
Member

Choose a reason for hiding this comment

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

can I suggest simply returning an errors.New("no charts in repository index"), as the caller of this function will log the error with Fatal:

monocular/cmd/chart-repo/sync.go

Line 64 in e19c1b1

if err = syncRepo(dbSession, args[0], args[1], authorizationHeader); err != nil {
.

The issue with using Fatal here is that it will exit the program here, and that should really be the job of the caller of this function. This function should just return a response and not control the flow of the program.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@prydonius Thanks for explaining.

Do you mean to add a test something like this:

type chartHTTPClient struct{}

func (h *chartHTTPClient) Do(req *http.Request) (*http.Response, error){
	w := httptest.NewRecorder()
	// Don't accept trailing slashes
	if strings.HasPrefix(req.URL.Path, "//") {
		w.WriteHeader(500)
	}
	// If subpath repo URL test, check that index.yaml is correctly added to the
	// subpath
	if req.URL.Host == "subpath.test" && req.URL.Path != "/subpath/index.yaml" {
		w.WriteHeader(500)
	}
	w.Write(nil)
	return w.Result(), nil
}

Copy link
Member

Choose a reason for hiding this comment

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

@hpandeycodeit I think we can remove the trailing slashes and subpath parts in that, and just return an empty index. We should also call this something like emptyChartRepoHTTPClient to make the intent more clear.

I'm also not sure if w.Write(nil) will be sufficient though, we might need to create a test index file (like

monocular/cmd/chart-repo/testdata/valid-index.yaml

Line 1 in e19c1b1

entries:
) that has an entries key that is empty.

e.g.:

type emptyChartRepoHTTPClient struct{}

func (h *emptyChartRepoHTTPClient) Do(req *http.Request) (*http.Response, error){
	w := httptest.NewRecorder()

        w.Write([]byte(emptyRepoIndexYAML))
	return w.Result(), nil
}

We would then need a test that is similar to

monocular/cmd/chart-repo/utils_test.go

Line 157 in e19c1b1

func Test_syncURLInvalidity(t *testing.T) {
, but also sets the netClient like

monocular/cmd/chart-repo/utils_test.go

Line 202 in e19c1b1

netClient = &badHTTPClient{}
, and asserts than an error is returned.

Does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@prydonius Yes, it does. I have added the test case. Let me know if it looks okay to you.

:)

return nil
}
err = importCharts(dbSession, charts)
if err != nil {
return err
Expand Down