From c6e84e14c7c23e306783c9dcedbea8bd2a089c2d Mon Sep 17 00:00:00 2001 From: Anirudh M Date: Tue, 30 Jun 2020 20:45:41 +0530 Subject: [PATCH] Add additional tests and fix golangci-lint --- cmd/service/service.go | 13 ++------- pkg/api/install_api_test.go | 7 +++-- pkg/api/list.go | 7 +++-- pkg/api/list_api_test.go | 27 +++++++++--------- pkg/api/lister.go | 4 +-- pkg/api/ping/pingcontract.go | 4 +-- pkg/api/ping/pinghandler.go | 4 +-- pkg/api/service.go | 8 +----- pkg/api/service_test.go | 53 +++++++++++++++++++++++++++++++----- pkg/api/upgrade.go | 6 ++-- pkg/api/upgrade_api_test.go | 1 + 11 files changed, 82 insertions(+), 52 deletions(-) diff --git a/cmd/service/service.go b/cmd/service/service.go index 355c498f0..d0764e2a5 100644 --- a/cmd/service/service.go +++ b/cmd/service/service.go @@ -22,21 +22,14 @@ func startServer() { //TODO: use gorilla mux and add middleware to write content type and other headers app := servercontext.App() logger.Setup("debug") -<<<<<<< HEAD + + actionList := action.NewList(app.ActionConfig) actionInstall := action.NewInstall(app.ActionConfig) actionUpgrade := action.NewUpgrade(app.ActionConfig) actionHistory := action.NewHistory(app.ActionConfig) - service := api.NewService(app.Config, new(action.ChartPathOptions), api.NewInstaller(actionInstall), api.NewUpgrader(actionUpgrade), api.NewHistory(actionHistory)) - -======= - - service := api.NewService(app.Config, - new(action.ChartPathOptions), - api.NewInstall(action.NewInstall(app.ActionConfig)), - api.NewList(action.NewList(app.ActionConfig))) + service := api.NewService(app.Config, new(action.ChartPathOptions), api.NewList(actionList), api.NewInstall(actionInstall), api.NewUpgrader(actionUpgrade), api.NewHistory(actionHistory)) ->>>>>>> helm-service-refactor router.Handle("/ping", ping.Handler()) router.Handle("/list", api.List(service)) router.Handle("/install", api.Install(service)) diff --git a/pkg/api/install_api_test.go b/pkg/api/install_api_test.go index 213f41e2f..f5a15fdcd 100644 --- a/pkg/api/install_api_test.go +++ b/pkg/api/install_api_test.go @@ -13,6 +13,7 @@ import ( "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" "gotest.tools/assert" + "helm.sh/helm/v3/pkg/api" "helm.sh/helm/v3/pkg/api/logger" "helm.sh/helm/v3/pkg/cli" @@ -23,9 +24,9 @@ type InstallerTestSuite struct { suite.Suite recorder *httptest.ResponseRecorder server *httptest.Server - mockInstall *mockInstall + mockInstall *mockInstall mockChartLoader *mockChartLoader - mockList *mockList + mockList *mockList appConfig *cli.EnvSettings } @@ -42,7 +43,7 @@ func (s *InstallerTestSuite) SetupTest() { RepositoryConfig: "./testdata/helm", PluginsDirectory: "./testdata/helm/plugin", } - service := api.NewService(s.appConfig, s.mockChartLoader,nil, s.mockInstall, nil, nil) + service := api.NewService(s.appConfig, s.mockChartLoader, nil, s.mockInstall, nil, nil) handler := api.Install(service) s.server = httptest.NewServer(handler) } diff --git a/pkg/api/list.go b/pkg/api/list.go index 8f15225cd..133c706a4 100644 --- a/pkg/api/list.go +++ b/pkg/api/list.go @@ -2,12 +2,13 @@ package api import ( "encoding/json" - "helm.sh/helm/v3/pkg/api/logger" "net/http" + + "helm.sh/helm/v3/pkg/api/logger" ) type ListRequest struct { - NameSpace string `json:"namespace"` + NameSpace string `json:"namespace"` ReleaseStatus string `json:"release_status"` } @@ -63,4 +64,4 @@ func List(svc Service) http.Handler { w.WriteHeader(http.StatusOK) w.Write(payload) }) -} \ No newline at end of file +} diff --git a/pkg/api/list_api_test.go b/pkg/api/list_api_test.go index b09aac14c..6ff86424f 100644 --- a/pkg/api/list_api_test.go +++ b/pkg/api/list_api_test.go @@ -2,30 +2,30 @@ package api_test import ( "fmt" + "io/ioutil" + "net/http" + "net/http/httptest" + "strings" + "testing" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" "gotest.tools/assert" + "helm.sh/helm/v3/pkg/action" "helm.sh/helm/v3/pkg/api" "helm.sh/helm/v3/pkg/api/logger" "helm.sh/helm/v3/pkg/cli" "helm.sh/helm/v3/pkg/release" - "io/ioutil" - "net/http" - "net/http/httptest" - "strings" - "testing" ) type ListTestSuite struct { suite.Suite - recorder *httptest.ResponseRecorder - server *httptest.Server - mockInstall *mockInstall - mockChartLoader *mockChartLoader - mockList *mockList - appConfig *cli.EnvSettings + recorder *httptest.ResponseRecorder + server *httptest.Server + mockList *mockList + appConfig *cli.EnvSettings } type mockList struct{ mock.Mock } @@ -67,7 +67,7 @@ func (s *ListTestSuite) TestShouldReturnReleasesWhenSuccessfulAPICall() { releases = append(releases, &release.Release{Name: "test-release", Namespace: "test-namespace", - Info: &release.Info{Status: release.StatusDeployed}}) + Info: &release.Info{Status: release.StatusDeployed}}) s.mockList.On("SetStateMask") s.mockList.On("SetState", action.ListDeployed) @@ -89,7 +89,6 @@ func (s *ListTestSuite) TestShouldReturnBadRequestErrorIfItHasInvalidCharacter() body := `{"request_id":"test-request-id""""}` req, _ := http.NewRequest("POST", fmt.Sprintf("%s/list", s.server.URL), strings.NewReader(body)) - resp, err := http.DefaultClient.Do(req) assert.Equal(s.T(), 400, resp.StatusCode) @@ -106,4 +105,4 @@ func (s *ListTestSuite) TearDownTest() { func TestListAPI(t *testing.T) { suite.Run(t, new(ListTestSuite)) -} \ No newline at end of file +} diff --git a/pkg/api/lister.go b/pkg/api/lister.go index 3e65eea27..b9d652a87 100644 --- a/pkg/api/lister.go +++ b/pkg/api/lister.go @@ -15,10 +15,10 @@ type lister interface { SetState(state action.ListStates) } -func NewList(action *action.List) *list{ +func NewList(action *action.List) *list { return &list{action} } -func (l *list) SetState(state action.ListStates){ +func (l *list) SetState(state action.ListStates) { l.StateMask = state } diff --git a/pkg/api/ping/pingcontract.go b/pkg/api/ping/pingcontract.go index 4dc780c63..937731b72 100644 --- a/pkg/api/ping/pingcontract.go +++ b/pkg/api/ping/pingcontract.go @@ -1,10 +1,10 @@ package ping -type PingReq struct { +type Req struct { RequestID string } -type PingResponse struct { +type Response struct { Status bool Data string } diff --git a/pkg/api/ping/pinghandler.go b/pkg/api/ping/pinghandler.go index d6c0a3740..2d4cf5f3a 100644 --- a/pkg/api/ping/pinghandler.go +++ b/pkg/api/ping/pinghandler.go @@ -12,7 +12,7 @@ func Handler() http.Handler { res.Header().Set("Content-Type", "application/json") defer req.Body.Close() - var request PingReq + var request Req decoder := json.NewDecoder(req.Body) decoder.UseNumber() @@ -23,7 +23,7 @@ func Handler() http.Handler { request.RequestID = req.Header.Get("Request-Id") - response := PingResponse{Status: true, Data: "pong"} + response := Response{Status: true, Data: "pong"} payload, err := json.Marshal(response) if err != nil { fmt.Println("error parsing response") diff --git a/pkg/api/service.go b/pkg/api/service.go index e830f7c0d..ba2dea7e7 100644 --- a/pkg/api/service.go +++ b/pkg/api/service.go @@ -9,16 +9,13 @@ import ( "helm.sh/helm/v3/pkg/action" - "helm.sh/helm/v3/pkg/api/logger" "helm.sh/helm/v3/pkg/chart" "helm.sh/helm/v3/pkg/chart/loader" "helm.sh/helm/v3/pkg/cli" "helm.sh/helm/v3/pkg/storage/driver" - _ "k8s.io/client-go/plugin/pkg/client/auth" ) - type upgrader interface { SetConfig(ReleaseConfig) GetInstall() bool @@ -58,7 +55,6 @@ func (s Service) getValues(vals ChartValues) (ChartValues, error) { return vals, nil } - func (s Service) Install(ctx context.Context, cfg ReleaseConfig, values ChartValues) (*ReleaseResult, error) { if err := s.validate(cfg, values); err != nil { return nil, fmt.Errorf("error request validation: %v", err) @@ -110,7 +106,6 @@ func (s Service) loadChart(chartName string) (*chart.Chart, error) { return requestedChart, nil } - func (s Service) installChart(icfg ReleaseConfig, ch *chart.Chart, vals ChartValues) (*ReleaseResult, error) { s.Installer.SetConfig(icfg) release, err := s.Installer.Run(ch, vals) @@ -124,7 +119,6 @@ func (s Service) installChart(icfg ReleaseConfig, ch *chart.Chart, vals ChartVal return result, nil } - func (s Service) upgradeRelease(ucfg ReleaseConfig, ch *chart.Chart, vals ChartValues) (*ReleaseResult, error) { s.upgrader.SetConfig(ucfg) release, err := s.upgrader.Run(ucfg.Name, ch, vals) @@ -175,5 +169,5 @@ func (s Service) List(releaseStatus string) ([]Release, error) { } func NewService(settings *cli.EnvSettings, cl chartloader, l lister, i Installer, u upgrader, h history) Service { - return Service{settings,cl, l,i, u, h} + return Service{settings, cl, l, i, u, h} } diff --git a/pkg/api/service_test.go b/pkg/api/service_test.go index ae3c7f4b4..6fee786df 100644 --- a/pkg/api/service_test.go +++ b/pkg/api/service_test.go @@ -3,14 +3,16 @@ package api_test import ( "context" "errors" + "testing" + "helm.sh/helm/v3/pkg/action" "helm.sh/helm/v3/pkg/api" - "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" + "helm.sh/helm/v3/pkg/api/logger" "helm.sh/helm/v3/pkg/chart" "helm.sh/helm/v3/pkg/cli" @@ -25,7 +27,7 @@ type ServiceTestSuite struct { history *mockHistory installer *mockInstall chartloader *mockChartLoader - lister *mockList + lister *mockList svc api.Service settings *cli.EnvSettings } @@ -147,7 +149,10 @@ func (s *ServiceTestSuite) TestUpgradeInstallTrueShouldInstallChart() { assert.NoError(t, err) require.NotNil(t, res) assert.Equal(t, res.Status, "deployed") + s.upgrader.AssertNotCalled(t, "Run") s.chartloader.AssertExpectations(t) + s.upgrader.AssertExpectations(t) + s.history.AssertExpectations(t) s.installer.AssertExpectations(t) } @@ -222,6 +227,44 @@ func (s *ServiceTestSuite) TestUpgradeShouldReturnResultOnSuccess() { s.upgrader.AssertExpectations(t) } +func (s *ServiceTestSuite) TestUpgradeValidateFailShouldResultFailure() { + 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) + + t := s.T() + assert.Nil(t, res) + assert.EqualError(t, err, "error request validation: cannot refer local chart") + s.chartloader.AssertNotCalled(t, "LocateChart") + s.upgrader.AssertNotCalled(t, "SetConfig") + s.upgrader.AssertNotCalled(t, "Run") +} + +func (s *ServiceTestSuite) TestUpgradeShouldReturnErrorOnInvalidChart() { + 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.Upgrade(s.ctx, cfg, vals) + + t := s.T() + assert.Nil(t, res) + assert.EqualError(t, err, "error in locating chart: Unable to find chart") + s.chartloader.AssertExpectations(t) + s.upgrader.AssertNotCalled(t, "SetConfig") + s.upgrader.AssertNotCalled(t, "Run") +} func (s *ServiceTestSuite) TestListShouldReturnErrorOnFailureOfListRun() { s.lister.On("SetState", action.ListDeployed) @@ -249,7 +292,7 @@ func (s *ServiceTestSuite) TestListShouldReturnAllReleasesIfNoFilterIsPassed() { releases = append(releases, &release.Release{Name: "test-release", Namespace: "test-namespace", - Info: &release.Info{Status: release.StatusDeployed}}) + Info: &release.Info{Status: release.StatusDeployed}}) s.lister.On("Run").Return(releases, nil) @@ -293,7 +336,6 @@ func (s *ServiceTestSuite) TestListShouldReturnDeployedReleasesIfDeployedIsPasse s.lister.AssertExpectations(t) } - type mockInstall struct{ mock.Mock } func (m *mockInstall) SetConfig(cfg api.ReleaseConfig) { @@ -305,7 +347,6 @@ func (m *mockInstall) Run(c *chart.Chart, vals map[string]interface{}) (*release return args.Get(0).(*release.Release), args.Error(1) } - type mockChartLoader struct{ mock.Mock } func (m *mockChartLoader) LocateChart(name string, settings *cli.EnvSettings) (string, error) { @@ -313,7 +354,6 @@ func (m *mockChartLoader) LocateChart(name string, settings *cli.EnvSettings) (s return args.String(0), args.Error(1) } - type mockUpgrader struct{ mock.Mock } func (m *mockUpgrader) Run(name string, chart *chart.Chart, vals map[string]interface{}) (*release.Release, error) { @@ -330,7 +370,6 @@ func (m *mockUpgrader) GetInstall() bool { return args.Get(0).(bool) } - type mockHistory struct{ mock.Mock } func (m *mockHistory) Run(name string) ([]*release.Release, error) { diff --git a/pkg/api/upgrade.go b/pkg/api/upgrade.go index 4c39b2fa5..3ab0c8b05 100644 --- a/pkg/api/upgrade.go +++ b/pkg/api/upgrade.go @@ -2,6 +2,7 @@ package api import ( "encoding/json" + "io" "net/http" "helm.sh/helm/v3/pkg/api/logger" @@ -25,8 +26,9 @@ func Upgrade(svc Service) http.Handler { defer r.Body.Close() var req UpgradeRequest - if err := json.NewDecoder(r.Body).Decode(&req); err != nil { - logger.Errorf("[Upgrade] error decoding request: %v", err) + + if err := json.NewDecoder(r.Body).Decode(&req); err == io.EOF || err != nil { + logger.Errorf("[Upgrade] error decoding request: %v", err.Error()) w.WriteHeader(http.StatusBadRequest) return } diff --git a/pkg/api/upgrade_api_test.go b/pkg/api/upgrade_api_test.go index 8194b6b15..64c542e5c 100644 --- a/pkg/api/upgrade_api_test.go +++ b/pkg/api/upgrade_api_test.go @@ -13,6 +13,7 @@ import ( "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" "gotest.tools/assert" + "helm.sh/helm/v3/pkg/api" "helm.sh/helm/v3/pkg/api/logger" "helm.sh/helm/v3/pkg/cli"