Bug 1803941: close helm request on error #4088

akashshinde · 2020-01-28T12:53:29Z

This PR fixes: https://issues.redhat.com/browse/ODC-2871

pedjak

we should have some tests that could catch regressions later. Also the commit message could be improved to describe what we are fixing.

akashshinde · 2020-01-29T08:15:41Z

@pedjak updated the commit message

akashshinde · 2020-01-31T08:34:48Z

cc @benjaminapetersen

akashshinde · 2020-01-31T08:34:59Z

/test analyze

pedjak

please add tests that could catch later regressions.

akashshinde · 2020-02-04T11:16:01Z

@pedjak we would need to add unit tests to test handlers.

pedjak · 2020-02-07T18:02:47Z

akashshinde#3 demonstrates how to add unit tests

pedjak · 2020-02-10T11:06:22Z

pkg/helm/handlers/handlers.go

@@ -20,6 +21,14 @@ var (
 type HelmHandlers struct {
 	ApiServerHost string
 	Transport     http.RoundTripper
+
+	// helm action configurator
+	GetActionConfigurations func(string, string, string, *http.RoundTripper) *action.Configuration


there is no need to make these functions public, see akashshinde#3

pedjak · 2020-02-10T11:08:00Z

pkg/server/server.go

@@ -302,8 +303,12 @@ func (s *Server) HTTPHandler() http.Handler {

 	// Helm Endpoints
 	helmConfig := &handlers.HelmHandlers{
-		ApiServerHost: s.KubeAPIServerURL,
-		Transport:     s.K8sClient.Transport,
+		ApiServerHost:           s.KubeAPIServerURL,


use a constructor, as demonstrated in akashshinde#3 - server does not need to know anything about actions package.

pedjak · 2020-02-10T11:09:22Z

pkg/helm/handlers/handler_test.go

+		request := httptest.NewRequest("", "/foo", strings.NewReader("{}"))
+		response := httptest.NewRecorder()
+
+		fakeHandlers.HandleHelmList(&auth.User{}, response, request)


I see here no assertions.. how do we know if the test passes/fails?

pedjak · 2020-02-10T11:12:10Z

pkg/helm/handlers/handlers.go

@@ -43,7 +43,7 @@ func (h *HelmHandlers) HandleHelmRenderManifests(user *auth.User, w http.Respons
 	conf := h.GetActionConfigurations(h.ApiServerHost, req.Namespace, user.Token, &h.Transport)
 	resp, err := h.RenderManifests(req.Name, req.ChartUrl, req.Values, conf)
 	if err != nil {
-		serverutils.SendResponse(w, http.StatusBadGateway, serverutils.ApiError{fmt.Sprintf("Failed to render manifests: %v", err)})
+		serverutils.SendResponse(w, http.StatusBadRequest, serverutils.ApiError{fmt.Sprintf("Failed to render manifests: %v", err)})


why are we changing contract here? The ticket is about fixing a bug, we should not change the contract. If really needed, we should open another ticket for that.

pkg/helm/handlers/handler_test.go

pedjak · 2020-02-10T11:15:47Z

pkg/helm/handlers/handler_test.go

+					t.Error("Failed to install release")
+				}
+				if bytes.Compare(response.Body.Bytes(), []byte(tt.expectedMsg)) != 0 {
+					t.Error("response body not matching")


I would display here what was expected and what was received - it would help to spot the issue faster.

pedjak · 2020-02-10T11:16:44Z

pkg/helm/fake/fake_actions.go

@@ -17,6 +18,10 @@ func InstallChart(ns string, name string, url string, values map[string]interfac
 	}, nil
 }

+func InstallChartFailed(ns string, name string, url string, values map[string]interface{}, conf *action.Configuration) (*release.Release, error) {


I would move these functions close to the tests, i.e. into the very same test file.

pedjak · 2020-02-13T10:44:34Z

pkg/helm/fake/fake_actions.go

@@ -0,0 +1,30 @@
+package fake


you have introduced here a package that could be accessed and used from the production code as well, and IMHO we should avoid that. Thus, I propose that we move the content of this file in handler_test.go. This allows us as well to make all functions private.

pedjak · 2020-02-13T10:47:48Z

pkg/helm/handlers/handlers.go

@@ -16,10 +18,29 @@ var (
 	plog = capnslog.NewPackageLogger("github.com/openshift/console", "helm")
 )

+func NewHelmHandler(apiUrl string, transport http.RoundTripper) *HelmHandlers {


the package name is handler, thus no need to repeat that in this constructor, I would say NewContext, because this structure represent now a context.

pedjak · 2020-02-13T10:48:54Z

pkg/helm/handlers/handlers.go

@@ -16,10 +18,29 @@ var (
 	plog = capnslog.NewPackageLogger("github.com/openshift/console", "helm")
 )

+func NewHelmHandler(apiUrl string, transport http.RoundTripper) *HelmHandlers {
+	return &HelmHandlers{


let's give a better name to this structure, it is a kind of context, thus let's call it helmContext - the structure does not need to public anymore.

pedjak · 2020-02-13T10:49:35Z

pkg/server/server.go

-		ApiServerHost: s.KubeAPIServerURL,
-		Transport:     s.K8sClient.Transport,
-	}
+	helmConfig := handlers.NewHelmHandler(s.KubeAPIServerURL, s.K8sClient.Transport)


let's rename it to helmContext

pedjak · 2020-02-13T18:53:05Z

/assign benjaminapetersen

pedjak · 2020-02-14T11:21:17Z

/lgtm

pedjak · 2020-02-14T11:50:16Z

/test analyze

pedjak · 2020-02-18T09:38:50Z

/lgtm

jhadvig · 2020-02-18T09:43:51Z

/lgtm

benjaminapetersen · 2020-02-18T15:39:28Z

pkg/server/server.go

-	handle("/api/helm/template", authHandlerWithUser(helmConfig.HandleHelmRenderManifests))
-	handle("/api/helm/releases", authHandlerWithUser(helmConfig.HandleHelmList))
-	handle("/api/helm/release", authHandlerWithUser(helmConfig.HandleHelmInstall))
+	helmHandlers := helmHandlers.New(s.KubeAPIServerURL, s.K8sClient.Transport)


You're redefining here, no?

// camel case the var, don't camelcase the package. helmHandlers := helmhandlers.New(s.KubeAPIServerURL, s.K8sClient.Transport)

If this is confusing, you can do:

// make the package even more clear (convention is not to camel case the import) helmhandlerspkg "github.com/openshift/console/pkg/helm/handlers" // and this isn't such a subtle difference helmHandlers := helmhandlerspkg.New(s.KubeAPIServerURL, s.K8sClient.Transport)

benjaminapetersen · 2020-02-18T15:50:37Z

pkg/helm/handlers/handler_test.go

+
+import (
+	"errors"
+	"helm.sh/helm/v3/pkg/action"


Imports order, puts yours together:

import( // std lib "errors" "strings" .... // deps "helm.sh/helm/v3/pkg/action" "helm.sh/helm/v3/pkg/release" // operator "github.com/openshift/console/pkg/auth" ... // yours, if any )

pkg/server/server.go

Code refactored a bit so that unit tests could be written.

benjaminapetersen · 2020-02-18T17:00:46Z

test-backend.sh

@@ -20,7 +20,7 @@ export GOFLAGS="-mod=vendor"
 # Invoke ./cover for HTML output
 COVER=${COVER:-"-cover"}

-TESTABLE="pkg/auth pkg/proxy pkg/server"
+TESTABLE="pkg/auth pkg/proxy pkg/server pkg/helm/actions pkg/helm/handlers"


@pedjak want to open a follow where we change this to cmd pkg?

benjaminapetersen

/lgtm

openshift-ci-robot · 2020-02-18T17:04:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akashshinde, benjaminapetersen, jhadvig, pedjak

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [benjaminapetersen]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2020-02-18T17:42:53Z

/retest