Added check for empty charts in chart-repo #604

hpandeycodeit · 2019-02-22T05:31:55Z

Added check for empty charts : Issue #603

Added check for empty charts

prydonius

Thanks so much for picking this up!

It would also be great if we could add a test for this. I think we'd just need to add an HTTP client or extend the existing one (

monocular/cmd/chart-repo/utils_test.go

Lines 58 to 74 in e19c1b1

    
           type goodHTTPClient struct{} 
        
           func (h *goodHTTPClient) 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([]byte(validRepoIndexYAML)) 
        
           	return w.Result(), nil 
        
           }

) to return an empty index that we load from testdata/

monocular/cmd/chart-repo/utils_test.go

Lines 45 to 46 in e19c1b1

    
           var validRepoIndexYAMLBytes, _ = ioutil.ReadFile("testdata/valid-index.yaml") 
        
           var validRepoIndexYAML = string(validRepoIndexYAMLBytes)

prydonius · 2019-02-22T22:11:00Z

cmd/chart-repo/utils.go

@@ -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")


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.

@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 }

@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?

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

:)

Signed-off-by: Himanshu Pandey <[email protected]>

prydonius

Looking good, just a few comments. Thanks!

prydonius · 2019-03-01T17:37:36Z

cmd/chart-repo/utils.go

@@ -98,6 +98,10 @@ func syncRepo(dbSession datastore.Session, repoName, repoURL string, authorizati
 	}

 	charts := chartsFromIndex(index, r)
+	if len(charts) == 0 {
+		errors.New("no charts in repository index")


We'll need to return the error so that the caller can exit appropriately

prydonius · 2019-03-01T17:38:23Z

cmd/chart-repo/utils_test.go

@@ -73,6 +73,24 @@ func (h *goodHTTPClient) Do(req *http.Request) (*http.Response, error) {
 	return w.Result(), nil
 }

+type chartHTTPClient struct{}


I guess we can remove this one and the Do below now that we have the emptyChartRepoHTTPClient?

Looks like I forgot to remove this one. :)
Removed this one now.

prydonius · 2019-03-01T17:39:21Z

cmd/chart-repo/utils_test.go

+
+func Test_emptyChartRepo(t *testing.T) {
+	netClient = &badHTTPClient{} 
+	_, err := fetchRepoIndex(repo{URL: "https://my.examplerepo.com"})


we actually need to call syncRepo, as this is where the error for an empty repo is found

prydonius · 2019-03-01T17:39:32Z

cmd/chart-repo/utils_test.go

+
+
+func Test_emptyChartRepo(t *testing.T) {
+	netClient = &badHTTPClient{} 


Suggested change

netClient = &badHTTPClient{}

netClient = &emptyChartRepoHTTPClient{}

prydonius

this is great! 2 more things before we can merge this:

run gofmt on your changes: https://golang.org/cmd/gofmt/
sign-off commits - see https://github.com/helm/monocular/pull/604/checks

Signed-off-by: Himanshu Pandey <[email protected]>

prydonius · 2019-03-07T18:30:33Z

Thanks @hpandeycodeit!

* Signed-off-by: Himanshu Pandey <[email protected]> Added check for empty charts * Added Test case for Empty Charts Signed-off-by: Himanshu Pandey <[email protected]> * Corrected the test case Signed-off-by: Himanshu Pandey <[email protected]>

hpandeycodeit · 2019-04-16T06:10:25Z

Thank you @prydonius!

Signed-off-by: Himanshu Pandey <[email protected]>

a005c40

Added check for empty charts

helm-bot added the size/XS label Feb 22, 2019

prydonius reviewed Feb 22, 2019

View reviewed changes

helm-bot added size/M and removed size/XS labels Mar 1, 2019

hpandeycodeit force-pushed the master branch from de90907 to 3fd65a6 Compare March 1, 2019 08:26

helm-bot added size/M and removed size/M labels Mar 1, 2019

Added Test case for Empty Charts

5355db7

Signed-off-by: Himanshu Pandey <[email protected]>

hpandeycodeit force-pushed the master branch from 3fd65a6 to 5355db7 Compare March 1, 2019 08:29

helm-bot added size/M and removed size/M labels Mar 1, 2019

prydonius reviewed Mar 1, 2019

View reviewed changes

helm-bot added size/S and removed size/M labels Mar 1, 2019

hpandeycodeit force-pushed the master branch from 32a5fa1 to 0dc11ed Compare March 1, 2019 19:00

helm-bot added size/S and removed size/S labels Mar 1, 2019

hpandeycodeit force-pushed the master branch from 0dc11ed to 7632de3 Compare March 1, 2019 19:03

helm-bot added size/S and removed size/S labels Mar 1, 2019

prydonius approved these changes Mar 2, 2019

View reviewed changes

Corrected the test case

895e3d6

Signed-off-by: Himanshu Pandey <[email protected]>

hpandeycodeit force-pushed the master branch from 7632de3 to 895e3d6 Compare March 2, 2019 00:12

helm-bot added size/S and removed size/S labels Mar 2, 2019

prydonius merged commit a5df290 into helm:master Mar 7, 2019

FengyunPan2 mentioned this pull request May 27, 2019

Don't return error when repos have no chart #633

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added check for empty charts in chart-repo #604

Added check for empty charts in chart-repo #604

hpandeycodeit commented Feb 22, 2019

prydonius left a comment

prydonius Feb 22, 2019

hpandeycodeit Feb 22, 2019

prydonius Feb 25, 2019

hpandeycodeit Mar 1, 2019

prydonius left a comment

prydonius Mar 1, 2019

hpandeycodeit Mar 1, 2019

prydonius Mar 1, 2019

hpandeycodeit Mar 1, 2019

prydonius Mar 1, 2019

hpandeycodeit Mar 1, 2019

prydonius Mar 1, 2019

hpandeycodeit Mar 1, 2019

prydonius left a comment

prydonius commented Mar 7, 2019

hpandeycodeit commented Apr 16, 2019

	type goodHTTPClient struct{}

	func (h goodHTTPClient) 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([]byte(validRepoIndexYAML))
	return w.Result(), nil
	}

	var validRepoIndexYAMLBytes, _ = ioutil.ReadFile("testdata/valid-index.yaml")
	var validRepoIndexYAML = string(validRepoIndexYAMLBytes)



		func Test_emptyChartRepo(t *testing.T) {
		netClient = &badHTTPClient{}

	netClient = &badHTTPClient{}
	netClient = &emptyChartRepoHTTPClient{}

Added check for empty charts in chart-repo #604

Added check for empty charts in chart-repo #604

Conversation

hpandeycodeit commented Feb 22, 2019

prydonius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prydonius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prydonius left a comment

Choose a reason for hiding this comment

prydonius commented Mar 7, 2019

hpandeycodeit commented Apr 16, 2019