Resolve comments

pkg/api/install.go

@@ -34,19 +34,18 @@ func Install(svc Service) http.Handler {
 		cfg := ReleaseConfig{ChartName: req.Chart, Name: req.Name, Namespace: req.Namespace}
 		res, err := svc.Install(r.Context(), cfg, req.Values)
 		if err != nil {
-			respondError(w, "error while installing chart: %v", err)
+			respondInstallError(w, "error while installing chart: %v", err)
 			return
 		}
 		response.Status = res.Status
 		if err := json.NewEncoder(w).Encode(&response); err != nil {
-			logger.Errorf("[Install] error writing response %v", err)
+			respondInstallError(w, "error writing response: %v", err)
-			w.WriteHeader(http.StatusInternalServerError)
 			return
 		}
 	})
 }
 
-func respondError(w http.ResponseWriter, logprefix string, err error) {
+func respondInstallError(w http.ResponseWriter, logprefix string, err error) {
 	response := InstallResponse{Error: err.Error()}
 	w.WriteHeader(http.StatusInternalServerError)
 	if err := json.NewEncoder(w).Encode(&response); err != nil {

pkg/api/list.go

@@ -47,25 +47,27 @@ func List(svc Service) http.Handler {
 		helmReleases, err := svc.List(request.ReleaseStatus)
 
 		if err != nil {
-			logger.Errorf("[List] error while installing chart: %v", err)
+			respondInstallError(w, "error while listing charts: %v", err)
-			response.Error = err.Error()
-			payload, _ := json.Marshal(response)
-			w.WriteHeader(http.StatusInternalServerError)
-			w.Write(payload)
 			return
 		}
 
 		response = ListResponse{"", helmReleases}
 		payload, err := json.Marshal(response)
 		if err != nil {
-			logger.Errorf("[List] error writing response %v", err)
+			respondInstallError(w, "error writing response: %v", err)
-			response.Error = err.Error()
-			payload, _ := json.Marshal(response)
-			w.WriteHeader(http.StatusInternalServerError)
-			w.Write(payload)
 			return
 		}
 
 		w.Write(payload)
 	})
 }
+
+func respondListError(w http.ResponseWriter, logprefix string, err error) {
+	response := ListResponse{Error: err.Error()}
+	w.WriteHeader(http.StatusInternalServerError)
+	if err := json.NewEncoder(w).Encode(&response); err != nil {
+		logger.Errorf("[List] %s %v", logprefix, err)
+		w.WriteHeader(http.StatusInternalServerError)
+		return
+	}
+}

pkg/api/list_api_test.go

@@ -2,14 +2,15 @@ package api_test
 
 import (
 	"fmt"
-	"helm.sh/helm/v3/pkg/chart"
-	"helm.sh/helm/v3/pkg/time"
 	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
 	"strings"
 	"testing"
 
+	"helm.sh/helm/v3/pkg/chart"
+	"helm.sh/helm/v3/pkg/time"
+
 	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
 	"github.com/stretchr/testify/suite"
@@ -65,31 +66,30 @@ func (s *ListTestSuite) TestShouldReturnReleasesWhenSuccessfulAPICall() {
 	layout := "2006-01-02T15:04:05.000Z"
 	str := "2014-11-12T11:45:26.371Z"
 	timeFromStr, _ := time.Parse(layout, str)
-
 	body := `{"release_status":"deployed"}`
 	req, _ := http.NewRequest("POST", fmt.Sprintf("%s/list", s.server.URL), strings.NewReader(body))
-
+	releases := []*release.Release{
-	var releases []*release.Release
+		{
-	releases = append(releases,
+			Name:      "test-release",
-		&release.Release{Name: "test-release",
 			Namespace: "test-namespace",
 			Info:      &release.Info{Status: release.StatusDeployed, LastDeployed: timeFromStr},
 			Version:   1,
 			Chart:     &chart.Chart{Metadata: &chart.Metadata{Name: "test-release", Version: "0.1", AppVersion: "0.1"}},
-		})
+		},
-
+	}
 	s.mockList.On("SetStateMask")
 	s.mockList.On("SetState", action.ListDeployed)
 	s.mockList.On("Run").Return(releases, nil)
 
-	resp, err := http.DefaultClient.Do(req)
+	resp, httpErr := http.DefaultClient.Do(req)
 
 	assert.Equal(s.T(), 200, resp.StatusCode)
 
 	expectedResponse := `{"releases":[{"name":"test-release","namespace":"test-namespace","revision":1,"updated_at":"2014-11-12T11:45:26.371Z","status":"deployed","chart":"test-release-0.1","app_version":"0.1"}]}`
-	respBody, _ := ioutil.ReadAll(resp.Body)
+	respBody, err := ioutil.ReadAll(resp.Body)
-	assert.Equal(s.T(), expectedResponse, string(respBody))
 
+	assert.Equal(s.T(), expectedResponse, string(respBody))
+	require.NoError(s.T(), httpErr)
 	require.NoError(s.T(), err)
 	s.mockList.AssertExpectations(s.T())
 }

pkg/api/service_test.go

@@ -47,13 +47,14 @@ func (s *ServiceTestSuite) SetupTest() {
 }
 
 func (s *ServiceTestSuite) TestInstallShouldReturnErrorOnInvalidChart() {
+	var vals api.ChartValues
 	chartName := "stable/invalid-chart"
 	cfg := api.ReleaseConfig{
 		Name:      "some-component",
 		Namespace: "hermes",
 		ChartName: chartName,
 	}
-	var vals api.ChartValues
+
 	s.chartloader.On("LocateChart", chartName, s.settings).Return("", errors.New("Unable to find chart"))
 
 	res, err := s.svc.Install(s.ctx, cfg, vals)
@@ -67,13 +68,13 @@ func (s *ServiceTestSuite) TestInstallShouldReturnErrorOnInvalidChart() {
 }
 
 func (s *ServiceTestSuite) TestInstallShouldReturnErrorOnLocalChartReference() {
+	var vals api.ChartValues
 	chartName := "./some/local-chart"
 	cfg := api.ReleaseConfig{
 		Name:      "some-component",
 		Namespace: "hermes",
 		ChartName: chartName,
 	}
-	var vals api.ChartValues
 
 	res, err := s.svc.Install(s.ctx, cfg, vals)
 
@@ -86,15 +87,16 @@ func (s *ServiceTestSuite) TestInstallShouldReturnErrorOnLocalChartReference() {
 }
 
 func (s *ServiceTestSuite) TestInstallShouldReturnErrorOnFailedInstallRun() {
+	var release *release.Release
+	var vals map[string]interface{}
 	chartName := "stable/valid-chart"
 	cfg := api.ReleaseConfig{
 		Name:      "some-component",
 		Namespace: "hermes",
 		ChartName: chartName,
 	}
-	var vals map[string]interface{}
+
 	s.chartloader.On("LocateChart", chartName, s.settings).Return("testdata/albatross", nil)
-	var release *release.Release
 	s.installer.On("SetConfig", cfg)
 	s.installer.On("Run", mock.AnythingOfType("*chart.Chart"), vals).Return(release, errors.New("cluster issue"))
 
@@ -108,13 +110,14 @@ func (s *ServiceTestSuite) TestInstallShouldReturnErrorOnFailedInstallRun() {
 }
 
 func (s *ServiceTestSuite) TestInstallShouldReturnResultOnSuccess() {
+	var vals map[string]interface{}
 	chartName := "stable/valid-chart"
 	cfg := api.ReleaseConfig{
 		Name:      "some-component",
 		Namespace: "hermes",
 		ChartName: chartName,
 	}
-	var vals map[string]interface{}
+
 	s.chartloader.On("LocateChart", chartName, s.settings).Return("testdata/albatross", nil)
 	s.installer.On("SetConfig", cfg)
 	release := &release.Release{Name: "some-comp-release", Info: &release.Info{Status: release.StatusDeployed}}
@@ -131,20 +134,21 @@ func (s *ServiceTestSuite) TestInstallShouldReturnResultOnSuccess() {
 }
 
 func (s *ServiceTestSuite) TestUpgradeInstallTrueShouldInstallChart() {
+	var vals map[string]interface{}
 	chartName := "stable/valid-chart"
 	cfg := api.ReleaseConfig{
 		Name:      "some-component",
 		Namespace: "hermes",
 		ChartName: chartName,
 	}
-	var vals map[string]interface{}
+
 	s.chartloader.On("LocateChart", chartName, s.settings).Return("testdata/albatross", nil)
 	s.upgrader.On("GetInstall").Return(true)
 	s.history.On("Run", "some-component").Return([]*release.Release{}, driver.ErrReleaseNotFound)
-
 	s.installer.On("SetConfig", cfg)
 	release := &release.Release{Name: "some-comp-release", Info: &release.Info{Status: release.StatusDeployed}}
 	s.installer.On("Run", mock.AnythingOfType("*chart.Chart"), vals).Return(release, nil)
+
 	res, err := s.svc.Upgrade(s.ctx, cfg, vals)
 
 	t := s.T()
@@ -168,10 +172,10 @@ func (s *ServiceTestSuite) TestUpgradeInstallFalseShouldNotInstallChart() {
 	var vals map[string]interface{}
 	s.chartloader.On("LocateChart", chartName, s.settings).Return("testdata/albatross", nil)
 	s.upgrader.On("GetInstall").Return(false)
-
 	s.upgrader.On("SetConfig", cfg)
 	release := &release.Release{Name: "some-comp-release", Info: &release.Info{Status: release.StatusDeployed}}
 	s.upgrader.On("Run", "some-component", mock.AnythingOfType("*chart.Chart"), vals).Return(release, nil)
+
 	res, err := s.svc.Upgrade(s.ctx, cfg, vals)
 
 	t := s.T()
@@ -199,6 +203,7 @@ func (s *ServiceTestSuite) TestUpgradeShouldReturnErrorOnFailedUpgradeRun() {
 	s.upgrader.On("Run", "some-component", mock.AnythingOfType("*chart.Chart"), vals).Return(release, errors.New("cluster issue"))
 
 	res, err := s.svc.Upgrade(s.ctx, cfg, vals)
+
 	t := s.T()
 	assert.Nil(t, res)
 	assert.EqualError(t, err, "error in upgrading chart: cluster issue")
@@ -221,6 +226,7 @@ func (s *ServiceTestSuite) TestUpgradeShouldReturnResultOnSuccess() {
 	s.upgrader.On("Run", "some-component", mock.AnythingOfType("*chart.Chart"), vals).Return(release, nil)
 
 	res, err := s.svc.Upgrade(s.ctx, cfg, vals)
+
 	t := s.T()
 	assert.NoError(t, err)
 	require.NotNil(t, res)
@@ -230,13 +236,13 @@ func (s *ServiceTestSuite) TestUpgradeShouldReturnResultOnSuccess() {
 }
 
 func (s *ServiceTestSuite) TestUpgradeValidateFailShouldResultFailure() {
+	var vals api.ChartValues
 	chartName := "./some/local-chart"
 	cfg := api.ReleaseConfig{
 		Name:      "some-component",
 		Namespace: "hermes",
 		ChartName: chartName,
 	}
-	var vals api.ChartValues
 
 	res, err := s.svc.Upgrade(s.ctx, cfg, vals)
 
@@ -269,32 +275,26 @@ func (s *ServiceTestSuite) TestUpgradeShouldReturnErrorOnInvalidChart() {
 }
 
 func (s *ServiceTestSuite) TestListShouldReturnErrorOnFailureOfListRun() {
+	var releases []*release.Release
+	releaseStatus := "deployed"
 	s.lister.On("SetState", action.ListDeployed)
 	s.lister.On("SetStateMask")
-
-	var releases []*release.Release
-
 	s.lister.On("Run").Return(releases, errors.New("cluster issue"))
 
-	releaseStatus := "deployed"
 	res, err := s.svc.List(releaseStatus)
 
 	t := s.T()
 	assert.Error(t, err, "cluster issue")
 	assert.Nil(t, res)
-
 	s.lister.AssertExpectations(t)
 }
 
 func (s *ServiceTestSuite) TestListShouldReturnAllReleasesIfNoFilterIsPassed() {
 	layout := "2006-01-02T15:04:05.000Z"
 	str := "2014-11-12T11:45:26.371Z"
-	timeFromStr, _ := time.Parse(layout, str)
+	releaseStatus := ""
-
-	s.lister.On("SetState", action.ListAll)
-	s.lister.On("SetStateMask")
-
 	var releases []*release.Release
+	timeFromStr, _ := time.Parse(layout, str)
 	releases = append(releases,
 		&release.Release{Name: "test-release",
 			Namespace: "test-namespace",
@@ -302,10 +302,11 @@ func (s *ServiceTestSuite) TestListShouldReturnAllReleasesIfNoFilterIsPassed() {
 			Version:   1,
 			Chart:     &chart.Chart{Metadata: &chart.Metadata{Name: "test-release", Version: "0.1", AppVersion: "0.1"}},
 		})
+	s.lister.On("SetState", action.ListAll)
+	s.lister.On("SetStateMask")
 
 	s.lister.On("Run").Return(releases, nil)
 
-	releaseStatus := ""
 	res, err := s.svc.List(releaseStatus)
 
 	t := s.T()
@@ -330,7 +331,7 @@ func (s *ServiceTestSuite) TestListShouldReturnAllReleasesIfNoFilterIsPassed() {
 	assert.Equal(t, release.StatusDeployed, response[0].Status)
 	assert.Equal(t, "test-release-0.1", response[0].Chart)
 	assert.Equal(t, "0.1", response[0].AppVersion)
-
+	assert.Equal(t, response, releases[0])
 	s.lister.AssertExpectations(t)
 }
 
@@ -339,17 +340,16 @@ func (s *ServiceTestSuite) TestListShouldReturnErrorIfInvalidStatusIsPassedAsFil
 	_, err := s.svc.List(releaseStatus)
 
 	t := s.T()
-	assert.Error(t, err, "invalid release status")
+	assert.EqualError(t, err, "invalid release status")
 }
 
 func (s *ServiceTestSuite) TestListShouldReturnDeployedReleasesIfDeployedIsPassedAsFilter() {
+	var releases []*release.Release
+	releaseStatus := "deployed"
 	s.lister.On("SetState", action.ListDeployed)
 	s.lister.On("SetStateMask")
-
-	var releases []*release.Release
 	s.lister.On("Run").Return(releases, nil)
 
-	releaseStatus := "deployed"
 	_, err := s.svc.List(releaseStatus)
 
 	t := s.T()

pkg/api/upgrade.go

@@ -37,14 +37,23 @@ func Upgrade(svc Service) http.Handler {
 		cfg := ReleaseConfig{ChartName: req.Chart, Name: req.Name, Namespace: req.Namespace}
 		res, err := svc.Upgrade(r.Context(), cfg, req.Values)
 		if err != nil {
-			respondError(w, "error while upgrading chart: %v", err)
+			respondUpgradeError(w, "error while upgrading chart: %v", err)
 			return
 		}
 		response.Status = res.Status
 		if err := json.NewEncoder(w).Encode(&response); err != nil {
-			logger.Errorf("[Upgrade] error writing response %v", err)
+			respondUpgradeError(w, "error writing response: %v", err)
-			w.WriteHeader(http.StatusInternalServerError)
 			return
 		}
 	})
 }
+
+func respondUpgradeError(w http.ResponseWriter, logprefix string, err error) {
+	response := UpgradeResponse{Error: err.Error()}
+	w.WriteHeader(http.StatusInternalServerError)
+	if err := json.NewEncoder(w).Encode(&response); err != nil {
+		logger.Errorf("[Upgrade] %s %v", logprefix, err)
+		w.WriteHeader(http.StatusInternalServerError)
+		return
+	}
+}