From 6b6497b051683bcb3f924cb7329a72cb202462c0 Mon Sep 17 00:00:00 2001 From: Chmouel Boudjnah Date: Thu, 29 Nov 2018 13:41:45 +0100 Subject: [PATCH 1/7] Query wit service to check if space exist Creating a serviceFactory of all services and use this from design controllers. --- .gitignore | 1 + Gopkg.lock | 8 +- Gopkg.toml | 6 ++ Makefile | 2 + application/rest/http.go | 33 +++++++ application/rest/url.go | 42 ++++++++ application/services.go | 26 +++++ application/wit/wit.go | 99 +++++++++++++++++++ configuration/configuration.go | 12 ++- controller/pipeline_environments.go | 14 ++- .../pipeline_environments_blackbox_test.go | 6 +- main.go | 6 +- 12 files changed, 246 insertions(+), 9 deletions(-) create mode 100644 application/rest/http.go create mode 100644 application/rest/url.go create mode 100644 application/services.go create mode 100644 application/wit/wit.go diff --git a/.gitignore b/.gitignore index 83ecf7e..0b57d28 100644 --- a/.gitignore +++ b/.gitignore @@ -81,3 +81,4 @@ tool/ migration/sqlbindata.go environment/generated/ auth/client +application/wit/witservice diff --git a/Gopkg.lock b/Gopkg.lock index bbb58aa..2fd28cd 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -86,6 +86,7 @@ "convert", "errors", "goamiddleware", + "goasupport", "goasupport/jsonapi_errors_helpers", "goasupport/status", "gormsupport", @@ -102,6 +103,11 @@ ] revision = "45960af9689499e9f5e5a4ebd142a8189a19ca1e" +[[projects]] + name = "github.com/fabric8-services/fabric8-wit" + packages = ["design"] + revision = "b59d0dc8ae9b2bfea1fb5dc6ff3552c42b735756" + [[projects]] name = "github.com/fsnotify/fsnotify" packages = ["."] @@ -482,6 +488,6 @@ [solve-meta] analyzer-name = "dep" analyzer-version = 1 - inputs-digest = "ac7eebf48cc5244e737fb8a811469b2fe37f4a59553f0a2aee834bc2f90a0c56" + inputs-digest = "a40eabbe2a85347eff475ae29b4dde96eab5c8f704e421dd2c5f8ba0041db2d9" solver-name = "gps-cdcl" solver-version = 1 diff --git a/Gopkg.toml b/Gopkg.toml index 00a9b6a..c9df145 100644 --- a/Gopkg.toml +++ b/Gopkg.toml @@ -29,6 +29,7 @@ required = [ "github.com/fabric8-services/fabric8-common/errors", # needed by goa generator "github.com/fabric8-services/fabric8-common/goasupport/jsonapi_errors_helpers", # needed by goa generator "github.com/fabric8-services/fabric8-common/goasupport/status", # needed by goa generator + "github.com/fabric8-services/fabric8-wit/design", "github.com/goadesign/goa/cors", "github.com/goadesign/goa/encoding/form", "github.com/goadesign/goa/goagen", @@ -57,3 +58,8 @@ ignored = [ [[constraint]] name = "github.com/stretchr/testify" version = "1.2.1" + + +[[constraint]] + name = "github.com/fabric8-services/fabric8-wit" + revision = "b59d0dc8ae9b2bfea1fb5dc6ff3552c42b735756" diff --git a/Makefile b/Makefile index bde2e93..501a877 100644 --- a/Makefile +++ b/Makefile @@ -277,6 +277,7 @@ clean-generated: -rm -rf ./app -rm -rf ./swagger/ -rm -f ./migration/sqlbindata.go + -rm -rf application/wit/witservice CLEAN_TARGETS += clean-vendor .PHONY: clean-vendor @@ -321,6 +322,7 @@ app/controllers.go: $(DESIGNS) $(GOAGEN_BIN) $(VENDOR_DIR) $(GOAGEN_BIN) app -d ${PACKAGE_NAME}/${DESIGN_DIR} $(GOAGEN_BIN) controller -d ${PACKAGE_NAME}/${DESIGN_DIR} -o controller/ --pkg controller --app-pkg ${PACKAGE_NAME}/app $(GOAGEN_BIN) swagger -d ${PACKAGE_NAME}/${DESIGN_DIR} + $(GOAGEN_BIN) client -d github.com/fabric8-services/fabric8-wit/design --notool --pkg witservice -o application/wit $(GOAGEN_BIN) gen -d ${PACKAGE_NAME}/${DESIGN_DIR} --pkg-path=github.com/fabric8-services/fabric8-common/goasupport/status --out app $(GOAGEN_BIN) gen -d ${PACKAGE_NAME}/${DESIGN_DIR} \ --pkg-path=github.com/fabric8-services/fabric8-common/goasupport/jsonapi_errors_helpers --out app diff --git a/application/rest/http.go b/application/rest/http.go new file mode 100644 index 0000000..1398a26 --- /dev/null +++ b/application/rest/http.go @@ -0,0 +1,33 @@ +package rest + +import ( + "context" + "net/http" + + "github.com/goadesign/goa/client" +) + +// Doer is a wrapper interface for goa client Doer +type HttpDoer interface { + client.Doer +} + +// HttpClient defines the Do method of the http client. +type HttpClient interface { + Do(req *http.Request) (*http.Response, error) +} + +// HttpClientDoer implements HttpDoer +type HttpClientDoer struct { + HttpClient HttpClient +} + +// DefaultHttpDoer creates a new HttpDoer with default http client +func DefaultHttpDoer() HttpDoer { + return &HttpClientDoer{HttpClient: http.DefaultClient} +} + +// Do overrides Do method of the default goa client Doer. It's needed for mocking http clients in tests. +func (d *HttpClientDoer) Do(ctx context.Context, req *http.Request) (*http.Response, error) { + return d.HttpClient.Do(req) +} diff --git a/application/rest/url.go b/application/rest/url.go new file mode 100644 index 0000000..056c86a --- /dev/null +++ b/application/rest/url.go @@ -0,0 +1,42 @@ +package rest + +import ( + "bytes" + "fmt" + "io" + "io/ioutil" + "net/http" + "net/url" +) + +// AbsoluteURL prefixes a relative URL with absolute address +func AbsoluteURL(req *http.Request, relative string) string { + scheme := "http" + if req.URL != nil && req.URL.Scheme == "https" { // isHTTPS + scheme = "https" + } + xForwardProto := req.Header.Get("X-Forwarded-Proto") + if xForwardProto != "" { + scheme = xForwardProto + } + return fmt.Sprintf("%s://%s%s", scheme, req.Host, relative) +} + +// AbsoluteURLAsURL returns the result of AbsoluteURL parsed into a URL +// structure and a potential parsing error. +func AbsoluteURLAsURL(req *http.Request, relative string) (*url.URL, error) { + return url.Parse(AbsoluteURL(req, relative)) +} + +// ReadBody reads body from a ReadCloser and returns it as a string +func ReadBody(body io.ReadCloser) string { + buf := new(bytes.Buffer) + buf.ReadFrom(body) + return buf.String() +} + +// CloseResponse reads the body and close the response. To be used to prevent file descriptor leaks. +func CloseResponse(response *http.Response) { + ioutil.ReadAll(response.Body) + response.Body.Close() +} diff --git a/application/services.go b/application/services.go new file mode 100644 index 0000000..35bd4c6 --- /dev/null +++ b/application/services.go @@ -0,0 +1,26 @@ +package application + +import ( + "github.com/fabric8-services/fabric8-build/application/wit" + "github.com/fabric8-services/fabric8-build/configuration" +) + +type ServiceFactory interface { + WITService() wit.WITService +} + +type serviceFactoryImpl struct { + Config *configuration.Config +} + +func (s serviceFactoryImpl) WITService() wit.WITService { + return &wit.WITServiceImpl{ + Config: *s.Config, + } +} + +func NewServiceFactory(config *configuration.Config) ServiceFactory { + return serviceFactoryImpl{ + Config: config, + } +} diff --git a/application/wit/wit.go b/application/wit/wit.go new file mode 100644 index 0000000..fc0181c --- /dev/null +++ b/application/wit/wit.go @@ -0,0 +1,99 @@ +package wit + +import ( + "context" + "net/http" + "net/url" + + "github.com/fabric8-services/fabric8-build/application/rest" + "github.com/fabric8-services/fabric8-build/application/wit/witservice" + "github.com/fabric8-services/fabric8-build/configuration" + "github.com/fabric8-services/fabric8-common/goasupport" + + "github.com/goadesign/goa/uuid" + "github.com/pkg/errors" + "github.com/prometheus/common/log" +) + +type Space struct { + ID uuid.UUID + OwnerID uuid.UUID + Name string + Description string +} + +type WITService interface { + GetSpace(ctx context.Context, spaceID string) (space *Space, e error) +} + +type WITServiceImpl struct { + Config configuration.Config + doer rest.HttpDoer +} + +// GetSpace talks to the WIT service to retrieve a space record for the specified spaceID, then returns space +func (s *WITServiceImpl) GetSpace(ctx context.Context, spaceID string) (space *Space, e error) { + remoteWITService, err := s.createClientWithContextSigner(ctx) + if err != nil { + return nil, err + } + + spaceIDUUID, err := uuid.FromString(spaceID) + if err != nil { + return nil, err + } + + res, err := remoteWITService.ShowSpace(ctx, witservice.ShowSpacePath(spaceIDUUID), nil, nil) + if err != nil { + return nil, err + } + + defer rest.CloseResponse(res) + if res.StatusCode != http.StatusOK { + bodyString := rest.ReadBody(res.Body) + log.Error(ctx, map[string]interface{}{ + "spaceId": spaceID, + "response_status": res.Status, + "response_body": bodyString, + }, "unable to get space from WIT") + return nil, errors.Errorf("unable to get space from WIT. Response status: %s. Response body: %s", res.Status, bodyString) + } + + spaceSingle, err := remoteWITService.DecodeSpaceSingle(res) + if err != nil { + return nil, err + } + + return &Space{ + ID: *spaceSingle.Data.ID, + Name: *spaceSingle.Data.Attributes.Name, + Description: *spaceSingle.Data.Attributes.Description, + OwnerID: *spaceSingle.Data.Relationships.OwnedBy.Data.ID}, nil +} + +// createClientWithContextSigner creates with a signer based on current context +func (s *WITServiceImpl) createClientWithContextSigner(ctx context.Context) (*witservice.Client, error) { + c, err := s.createClient() + if err != nil { + return nil, err + } + + c.SetJWTSigner(goasupport.NewForwardSigner(ctx)) + return c, nil +} + +func (s *WITServiceImpl) createClient() (*witservice.Client, error) { + witURL, e := s.Config.GetWITURL() + if e != nil { + return nil, e + } + u, err := url.Parse(witURL) + if err != nil { + return nil, err + } + + c := witservice.New(s.doer) + c.Host = u.Host + c.Scheme = u.Scheme + return c, nil +} diff --git a/configuration/configuration.go b/configuration/configuration.go index 8706534..5f61e7e 100644 --- a/configuration/configuration.go +++ b/configuration/configuration.go @@ -24,6 +24,10 @@ const ( varMetricsHTTPAddress = "metrics.http.address" varSentryDSN = "sentry.dsn" + // External f8 services + varAuthURL = "auth.url" + varWITURL = "wit.url" + // Postgres varPostgresHost = "postgres.host" varPostgresPort = "postgres.port" @@ -36,8 +40,6 @@ const ( varPostgresConnectionRetrySleep = "postgres.connection.retrysleep" varPostgresConnectionMaxIdle = "postgres.connection.maxidle" varPostgresConnectionMaxOpen = "postgres.connection.maxopen" - - varAuthURL = "auth.url" ) // New creates a configuration reader object using a configurable configuration @@ -272,3 +274,9 @@ func (c *Config) IsDBLogsEnabled() bool { func (c *Config) IsCleanTestDataEnabled() bool { return c.v.GetBool(varCleanTestDataEnabled) } + +// GetWITURL returns the WIT URL where WIT is running +// If AUTH_WIT_URL is not set and Auth in not in Dev Mode then we calculate the URL from the Auth Service URL domain +func (c *Config) GetWITURL() (string, error) { + return c.v.GetString(varWITURL), nil +} diff --git a/controller/pipeline_environments.go b/controller/pipeline_environments.go index 5359233..8f24d69 100644 --- a/controller/pipeline_environments.go +++ b/controller/pipeline_environments.go @@ -17,14 +17,16 @@ import ( // PipelineEnvironmentController implements the PipelineEnvironment resource. type PipelineEnvironmentController struct { *goa.Controller - db application.DB + db application.DB + svcFactory application.ServiceFactory } // NewPipelineEnvironmentController creates a PipelineEnvironment controller. -func NewPipelineEnvironmentController(service *goa.Service, db application.DB) *PipelineEnvironmentController { +func NewPipelineEnvironmentController(service *goa.Service, db application.DB, svcFactory application.ServiceFactory) *PipelineEnvironmentController { return &PipelineEnvironmentController{ Controller: service.NewController("PipelineEnvironmentController"), db: db, + svcFactory: svcFactory, } } @@ -139,7 +141,11 @@ func (c *PipelineEnvironmentController) Show(ctx *app.ShowPipelineEnvironmentsCo } func (c *PipelineEnvironmentController) checkSpaceExist(ctx context.Context, spaceID string) error { - // TODO check if space exists - // TODO check if space owner is the caller + // TODO(chmouel): Make sure we have the rights for that space + // TODO(chmouel): Better error reporting when NOTFound + _, err := c.svcFactory.WITService().GetSpace(ctx, spaceID) + if err != nil { + return errs.Wrapf(err, "failed to get space id: %s from wit", spaceID) + } return nil } diff --git a/controller/pipeline_environments_blackbox_test.go b/controller/pipeline_environments_blackbox_test.go index a1f2cf3..0fca79c 100644 --- a/controller/pipeline_environments_blackbox_test.go +++ b/controller/pipeline_environments_blackbox_test.go @@ -10,6 +10,7 @@ import ( "github.com/fabric8-services/fabric8-build/app" "github.com/fabric8-services/fabric8-build/app/test" + "github.com/fabric8-services/fabric8-build/application" "github.com/fabric8-services/fabric8-build/configuration" "github.com/fabric8-services/fabric8-build/controller" "github.com/fabric8-services/fabric8-build/gormapp" @@ -33,6 +34,8 @@ type PipelineEnvironmentControllerSuite struct { ctrl *controller.PipelineEnvironmentController ctrl2 *controller.PipelineEnvironmentController + + svcfactory *application.ServiceFactory } func TestPipelineEnvironmentController(t *testing.T) { @@ -56,9 +59,10 @@ func (s *PipelineEnvironmentControllerSuite) SetupSuite() { s.ctx = s.svc.Context s.ctx2 = s.svc2.Context + s.svcFactory = application.ServiceFactory + s.ctrl = controller.NewPipelineEnvironmentController(s.svc, s.db) s.ctrl2 = controller.NewPipelineEnvironmentController(s.svc2, s.db) - } // createPipelineEnvironmentCtrlNoErroring we do this one manually cause the one from diff --git a/main.go b/main.go index 26bc1ef..4e59f60 100644 --- a/main.go +++ b/main.go @@ -10,6 +10,7 @@ import ( "time" "github.com/fabric8-services/fabric8-build/app" + "github.com/fabric8-services/fabric8-build/application" "github.com/fabric8-services/fabric8-build/configuration" "github.com/fabric8-services/fabric8-build/controller" "github.com/fabric8-services/fabric8-build/gormapp" @@ -123,6 +124,9 @@ func main() { service.Use(tokenCtxMW) service.Use(token.InjectTokenManager(tokenMgr)) + // Create the service factory + svcFactory := application.NewServiceFactory(config) + // record HTTP request metrics in prometh service.Use( metric.Recorder( @@ -138,7 +142,7 @@ func main() { appDB := gormapp.NewGormDB(db) // Mount the 'pipeline environment map' controller - pipelineEnvCtrl := controller.NewPipelineEnvironmentController(service, appDB) + pipelineEnvCtrl := controller.NewPipelineEnvironmentController(service, appDB, svcFactory) app.MountPipelineEnvironmentsController(service, pipelineEnvCtrl) log.Logger().Infoln("Git Commit SHA: ", app.Commit) From 61558af97c4c2f317587981ead93620998a71cad Mon Sep 17 00:00:00 2001 From: Chmouel Boudjnah Date: Thu, 29 Nov 2018 15:42:56 +0100 Subject: [PATCH 2/7] Deploy wit in deploy-openshift-dev.sh script --- Makefile | 9 +++------ openshift/deploy-openshift-dev.sh | 14 +++++++++++++- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/Makefile b/Makefile index 501a877..4c19569 100644 --- a/Makefile +++ b/Makefile @@ -41,16 +41,13 @@ AUTH_CONTAINER_NAME = auth AUTH_CONTAINER_PORT = 8089 AUTH_CONTAINER_IMAGE = quay.io/openshiftio/fabric8-services-fabric8-auth:latest -AUTH_DB_CONTAINER_NAME = db-auth -AUTH_DB_CONTAINER_IMAGE = $(DB_CONTAINER_IMAGE) - # Env ENV_CONTAINER_NAME = f8env -ENV_CONTAINER_PORT = 8080 ENV_CONTAINER_IMAGE = quay.io/openshiftio/fabric8-services-fabric8-env:latest -ENV_DB_CONTAINER_NAME = db-env -ENV_DB_CONTAINER_IMAGE = $(DB_CONTAINER_IMAGE) +# Wit +WIT_CONTAINER_NAME = f8wit +WIT_CONTAINER_IMAGE = quay.io/openshiftio/fabric8-services-fabric8-wit:latest # By default reduce the amount of log output from tests, set this to debug and GO_TEST_VERBOSITY_FLAG to -v to increase it F8_LOG_LEVEL ?= error diff --git a/openshift/deploy-openshift-dev.sh b/openshift/deploy-openshift-dev.sh index 9bf4682..061333a 100755 --- a/openshift/deploy-openshift-dev.sh +++ b/openshift/deploy-openshift-dev.sh @@ -13,7 +13,7 @@ set -ex function readlinkf() { python -c 'import os,sys;print(os.path.realpath(sys.argv[1]))' $1 ;} cd $(dirname $(readlinkf $0))/../ -eval $(make print-env|egrep '^(REGISTRY_URL|AUTH|DB|ENV).*(IMAGE|PORT|NAME)') +eval $(make print-env|egrep '^(REGISTRY_URL|WIT|AUTH|DB|ENV).*(IMAGE|PORT|NAME)') oc whoami 2>/dev/null >/dev/null || { echo "oc does not seem to be configured properly"; exit 1 ;} @@ -130,6 +130,18 @@ EOF ) deploy_sideservice ${ENV_CONTAINER_NAME} ${ENV_CONTAINER_IMAGE} "${ENV_SERVICE_VARIABLES}" +# WIT +WIT_SERVICE_VARIABLES=$(cat < Date: Thu, 29 Nov 2018 18:02:29 +0100 Subject: [PATCH 3/7] Add gock based tests for wit service --- Gopkg.lock | 7 +- Gopkg.toml | 3 + .../pipeline_environments_blackbox_test.go | 164 ++++++++++++++++-- 3 files changed, 157 insertions(+), 17 deletions(-) diff --git a/Gopkg.lock b/Gopkg.lock index 2fd28cd..4d56463 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -469,6 +469,11 @@ revision = "947dcec5ba9c011838740e680966fd7087a71d0d" version = "v2.2.6" +[[projects]] + name = "gopkg.in/h2non/gock.v1" + packages = ["."] + revision = "master" + [[projects]] name = "gopkg.in/square/go-jose.v2" packages = [ @@ -488,6 +493,6 @@ [solve-meta] analyzer-name = "dep" analyzer-version = 1 - inputs-digest = "a40eabbe2a85347eff475ae29b4dde96eab5c8f704e421dd2c5f8ba0041db2d9" + inputs-digest = "243b6a93f8916459eea58c49d49e110faf1d215f25330727bc50828927f20df1" solver-name = "gps-cdcl" solver-version = 1 diff --git a/Gopkg.toml b/Gopkg.toml index c9df145..b6e0a77 100644 --- a/Gopkg.toml +++ b/Gopkg.toml @@ -59,6 +59,9 @@ ignored = [ name = "github.com/stretchr/testify" version = "1.2.1" +[[constraint]] + name = "gopkg.in/h2non/gock.v1" + revision = "master" [[constraint]] name = "github.com/fabric8-services/fabric8-wit" diff --git a/controller/pipeline_environments_blackbox_test.go b/controller/pipeline_environments_blackbox_test.go index 0fca79c..4bd7c88 100644 --- a/controller/pipeline_environments_blackbox_test.go +++ b/controller/pipeline_environments_blackbox_test.go @@ -6,6 +6,7 @@ import ( "net/http" "net/http/httptest" "net/url" + "os" "testing" "github.com/fabric8-services/fabric8-build/app" @@ -21,8 +22,116 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" + "gopkg.in/h2non/gock.v1" ) +// TODO(chmouel): Templates externalize etc... +var defaultSpaceJson = `{ + "data": { + "attributes": { + "created-at": "2018-11-29T15:50:57.981132Z", + "description": "", + "name": "%s", + "updated-at": "2018-11-29T15:50:57.981132Z", + "version": 0 + }, + "id": "%s", + "links": { + "backlog": { + "meta": { + "totalCount": 0 + }, + "self": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/spaces/fb0c49c4-7682-46cd-a29c-cb2bff83752b/backlog" + }, + "filters": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/filters", + "related": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/spaces/fb0c49c4-7682-46cd-a29c-cb2bff83752b", + "self": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/spaces/fb0c49c4-7682-46cd-a29c-cb2bff83752b", + "workitemlinktypes": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/spacetemplates/f405fa41-a8bb-46db-8800-2dbe13da1418/workitemlinktypes", + "workitemtypes": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/spacetemplates/f405fa41-a8bb-46db-8800-2dbe13da1418/workitemtypes" + }, + "relationships": { + "areas": { + "links": { + "related": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/spaces/fb0c49c4-7682-46cd-a29c-cb2bff83752b/areas" + } + }, + "backlog": { + "links": { + "related": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/spaces/fb0c49c4-7682-46cd-a29c-cb2bff83752b/backlog" + }, + "meta": { + "totalCount": 0 + } + }, + "codebases": { + "links": { + "related": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/spaces/fb0c49c4-7682-46cd-a29c-cb2bff83752b/codebases" + } + }, + "collaborators": { + "links": { + "related": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/spaces/fb0c49c4-7682-46cd-a29c-cb2bff83752b/collaborators" + } + }, + "filters": { + "links": { + "related": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/filters" + } + }, + "iterations": { + "links": { + "related": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/spaces/fb0c49c4-7682-46cd-a29c-cb2bff83752b/iterations" + } + }, + "labels": { + "links": { + "related": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/spaces/fb0c49c4-7682-46cd-a29c-cb2bff83752b/labels" + } + }, + "owned-by": { + "data": { + "id": "df61d335-a359-48eb-898d-fb4916c52937", + "type": "identities" + }, + "links": { + "related": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/users/df61d335-a359-48eb-898d-fb4916c52937" + } + }, + "space-template": { + "data": { + "id": "f405fa41-a8bb-46db-8800-2dbe13da1418", + "type": "spacetemplates" + }, + "links": { + "related": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/spacetemplates/f405fa41-a8bb-46db-8800-2dbe13da1418", + "self": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/spacetemplates/f405fa41-a8bb-46db-8800-2dbe13da1418" + } + }, + "workitemlinktypes": { + "links": { + "related": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/spacetemplates/f405fa41-a8bb-46db-8800-2dbe13da1418/workitemlinktypes" + } + }, + "workitems": { + "links": { + "related": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/spaces/fb0c49c4-7682-46cd-a29c-cb2bff83752b/workitems" + } + }, + "workitemtypegroups": { + "links": { + "related": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/spacetemplates/f405fa41-a8bb-46db-8800-2dbe13da1418/workitemtypegroups" + } + }, + "workitemtypes": { + "links": { + "related": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/spacetemplates/f405fa41-a8bb-46db-8800-2dbe13da1418/workitemtypes" + } + } + }, + "type": "spaces" + } +}` + type PipelineEnvironmentControllerSuite struct { testsuite.DBTestSuite db *gormapp.GormDB @@ -35,7 +144,7 @@ type PipelineEnvironmentControllerSuite struct { ctrl *controller.PipelineEnvironmentController ctrl2 *controller.PipelineEnvironmentController - svcfactory *application.ServiceFactory + svcFactory application.ServiceFactory } func TestPipelineEnvironmentController(t *testing.T) { @@ -46,6 +155,7 @@ func TestPipelineEnvironmentController(t *testing.T) { func (s *PipelineEnvironmentControllerSuite) SetupSuite() { s.DBTestSuite.SetupSuite() + config, err := configuration.New("") s.db = gormapp.NewGormDB(s.DB) @@ -59,16 +169,29 @@ func (s *PipelineEnvironmentControllerSuite) SetupSuite() { s.ctx = s.svc.Context s.ctx2 = s.svc2.Context - s.svcFactory = application.ServiceFactory + s.svcFactory = application.NewServiceFactory(config) + + s.ctrl = controller.NewPipelineEnvironmentController(s.svc, s.db, s.svcFactory) + s.ctrl2 = controller.NewPipelineEnvironmentController(s.svc2, s.db, s.svcFactory) + + os.Setenv("F8_WIT_URL", "http://witservice") + // gock.Observe(gock.DumpRequest) + + defer func() { + gock.OffAll() + }() +} - s.ctrl = controller.NewPipelineEnvironmentController(s.svc, s.db) - s.ctrl2 = controller.NewPipelineEnvironmentController(s.svc2, s.db) +func (s *PipelineEnvironmentControllerSuite) createGockONSpace(spaceID uuid.UUID, spaceName string) { + gock.New("http://witservice"). + Get("/api/spaces/" + spaceID.String()). + Reply(200). + JSON(fmt.Sprintf(defaultSpaceJson, "space1", spaceID)) } // createPipelineEnvironmentCtrlNoErroring we do this one manually cause the one from // goatest one exit on errro without being able to catch -func (s *PipelineEnvironmentControllerSuite) createPipelineEnvironmentCtrlNoErroring() (*app.CreatePipelineEnvironmentsContext, *httptest.ResponseRecorder) { - spaceID := uuid.NewV4() +func (s *PipelineEnvironmentControllerSuite) createPipelineEnvironmentCtrlNoErroring(spaceID uuid.UUID) (*app.CreatePipelineEnvironmentsContext, *httptest.ResponseRecorder) { rw := httptest.NewRecorder() u := &url.URL{ Path: fmt.Sprintf("/api/pipelines/environments/%v", spaceID), @@ -88,34 +211,40 @@ func (s *PipelineEnvironmentControllerSuite) createPipelineEnvironmentCtrlNoErro } func (s *PipelineEnvironmentControllerSuite) TestCreate() { - s.T().Run("ok", func(t *testing.T) { + defer s.T().Run("ok", func(t *testing.T) { + space1ID := uuid.NewV4() + s.createGockONSpace(space1ID, "space1") payload := newPipelineEnvironmentPayload("osio-stage-create", uuid.NewV4()) - _, newEnv := test.CreatePipelineEnvironmentsCreated(t, s.ctx2, s.svc2, s.ctrl2, uuid.NewV4(), payload) + _, newEnv := test.CreatePipelineEnvironmentsCreated(t, s.ctx2, s.svc2, s.ctrl2, space1ID, payload) assert.NotNil(t, newEnv) assert.NotNil(t, newEnv.Data.ID) assert.NotNil(t, newEnv.Data.Environments[0].EnvUUID) // Same pipeline_name but different spaceID is OK + space2ID := uuid.NewV4() + s.createGockONSpace(space2ID, "space2") payload = newPipelineEnvironmentPayload("osio-stage-create", uuid.NewV4()) - _, newEnv = test.CreatePipelineEnvironmentsCreated(t, s.ctx2, s.svc2, s.ctrl2, uuid.NewV4(), payload) + _, newEnv = test.CreatePipelineEnvironmentsCreated(t, s.ctx2, s.svc2, s.ctrl2, space2ID, payload) assert.NotNil(t, newEnv) assert.NotNil(t, newEnv.Data.ID) assert.NotNil(t, newEnv.Data.Environments[0].EnvUUID) }) s.T().Run("fail", func(t *testing.T) { - conflict_space_id := uuid.NewV4() - payload := newPipelineEnvironmentPayload("osio-stage-create-conflict", uuid.NewV4()) + space1ID := uuid.NewV4() - _, newEnv := test.CreatePipelineEnvironmentsCreated(t, s.ctx2, s.svc2, s.ctrl2, conflict_space_id, payload) + s.createGockONSpace(space1ID, "space1") + payload := newPipelineEnvironmentPayload("osio-stage-create-conflict", uuid.NewV4()) + _, newEnv := test.CreatePipelineEnvironmentsCreated(t, s.ctx2, s.svc2, s.ctrl2, space1ID, payload) assert.NotNil(t, newEnv) - response, err := test.CreatePipelineEnvironmentsConflict(t, s.ctx2, s.svc2, s.ctrl2, conflict_space_id, payload) + s.createGockONSpace(space1ID, "space1") + response, err := test.CreatePipelineEnvironmentsConflict(t, s.ctx2, s.svc2, s.ctrl2, space1ID, payload) require.NotNil(t, response.Header().Get("Location")) assert.Regexp(s.T(), ".*data_conflict_error.*", err.Errors) emptyPayload := &app.CreatePipelineEnvironmentsPayload{} - createCtxerr, rw := s.createPipelineEnvironmentCtrlNoErroring() + createCtxerr, rw := s.createPipelineEnvironmentCtrlNoErroring(space1ID) createCtxerr.Payload = emptyPayload jerr := s.ctrl2.Create(createCtxerr) require.Nil(t, jerr) @@ -123,16 +252,19 @@ func (s *PipelineEnvironmentControllerSuite) TestCreate() { }) s.T().Run("unauthorized", func(t *testing.T) { + space1ID := uuid.NewV4() + s.createGockONSpace(space1ID, "space1") + payload := newPipelineEnvironmentPayload("osio-stage", uuid.NewV4()) - _, err := test.CreatePipelineEnvironmentsUnauthorized(t, s.ctx, s.svc, s.ctrl, uuid.NewV4(), payload) + _, err := test.CreatePipelineEnvironmentsUnauthorized(t, s.ctx, s.svc, s.ctrl, space1ID, payload) assert.NotNil(t, err) }) - } func (s *PipelineEnvironmentControllerSuite) TestShow() { s.T().Run("ok", func(t *testing.T) { spaceID := uuid.NewV4() + s.createGockONSpace(spaceID, "space1") payload := newPipelineEnvironmentPayload("osio-stage-show", uuid.NewV4()) _, newEnv := test.CreatePipelineEnvironmentsCreated(t, s.ctx2, s.svc2, s.ctrl2, spaceID, payload) require.NotNil(t, newEnv) From 5dbbe3a164d9b137777748d081e61d86e8a31ee2 Mon Sep 17 00:00:00 2001 From: Chmouel Boudjnah Date: Thu, 29 Nov 2018 21:28:40 +0100 Subject: [PATCH 4/7] Use the generated struct for space json creation data Instead of using our hardcoded json blob, let's use the proper classes there. --- .../pipeline_environments_blackbox_test.go | 152 +++++------------- 1 file changed, 41 insertions(+), 111 deletions(-) diff --git a/controller/pipeline_environments_blackbox_test.go b/controller/pipeline_environments_blackbox_test.go index 4bd7c88..5a396df 100644 --- a/controller/pipeline_environments_blackbox_test.go +++ b/controller/pipeline_environments_blackbox_test.go @@ -2,22 +2,26 @@ package controller_test import ( "context" + "encoding/json" "fmt" "net/http" "net/http/httptest" "net/url" "os" "testing" + "time" "github.com/fabric8-services/fabric8-build/app" "github.com/fabric8-services/fabric8-build/app/test" "github.com/fabric8-services/fabric8-build/application" + "github.com/fabric8-services/fabric8-build/application/wit/witservice" "github.com/fabric8-services/fabric8-build/configuration" "github.com/fabric8-services/fabric8-build/controller" "github.com/fabric8-services/fabric8-build/gormapp" testauth "github.com/fabric8-services/fabric8-common/test/auth" testsuite "github.com/fabric8-services/fabric8-common/test/suite" "github.com/goadesign/goa" + guuid "github.com/goadesign/goa/uuid" uuid "github.com/satori/go.uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -25,113 +29,6 @@ import ( "gopkg.in/h2non/gock.v1" ) -// TODO(chmouel): Templates externalize etc... -var defaultSpaceJson = `{ - "data": { - "attributes": { - "created-at": "2018-11-29T15:50:57.981132Z", - "description": "", - "name": "%s", - "updated-at": "2018-11-29T15:50:57.981132Z", - "version": 0 - }, - "id": "%s", - "links": { - "backlog": { - "meta": { - "totalCount": 0 - }, - "self": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/spaces/fb0c49c4-7682-46cd-a29c-cb2bff83752b/backlog" - }, - "filters": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/filters", - "related": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/spaces/fb0c49c4-7682-46cd-a29c-cb2bff83752b", - "self": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/spaces/fb0c49c4-7682-46cd-a29c-cb2bff83752b", - "workitemlinktypes": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/spacetemplates/f405fa41-a8bb-46db-8800-2dbe13da1418/workitemlinktypes", - "workitemtypes": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/spacetemplates/f405fa41-a8bb-46db-8800-2dbe13da1418/workitemtypes" - }, - "relationships": { - "areas": { - "links": { - "related": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/spaces/fb0c49c4-7682-46cd-a29c-cb2bff83752b/areas" - } - }, - "backlog": { - "links": { - "related": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/spaces/fb0c49c4-7682-46cd-a29c-cb2bff83752b/backlog" - }, - "meta": { - "totalCount": 0 - } - }, - "codebases": { - "links": { - "related": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/spaces/fb0c49c4-7682-46cd-a29c-cb2bff83752b/codebases" - } - }, - "collaborators": { - "links": { - "related": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/spaces/fb0c49c4-7682-46cd-a29c-cb2bff83752b/collaborators" - } - }, - "filters": { - "links": { - "related": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/filters" - } - }, - "iterations": { - "links": { - "related": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/spaces/fb0c49c4-7682-46cd-a29c-cb2bff83752b/iterations" - } - }, - "labels": { - "links": { - "related": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/spaces/fb0c49c4-7682-46cd-a29c-cb2bff83752b/labels" - } - }, - "owned-by": { - "data": { - "id": "df61d335-a359-48eb-898d-fb4916c52937", - "type": "identities" - }, - "links": { - "related": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/users/df61d335-a359-48eb-898d-fb4916c52937" - } - }, - "space-template": { - "data": { - "id": "f405fa41-a8bb-46db-8800-2dbe13da1418", - "type": "spacetemplates" - }, - "links": { - "related": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/spacetemplates/f405fa41-a8bb-46db-8800-2dbe13da1418", - "self": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/spacetemplates/f405fa41-a8bb-46db-8800-2dbe13da1418" - } - }, - "workitemlinktypes": { - "links": { - "related": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/spacetemplates/f405fa41-a8bb-46db-8800-2dbe13da1418/workitemlinktypes" - } - }, - "workitems": { - "links": { - "related": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/spaces/fb0c49c4-7682-46cd-a29c-cb2bff83752b/workitems" - } - }, - "workitemtypegroups": { - "links": { - "related": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/spacetemplates/f405fa41-a8bb-46db-8800-2dbe13da1418/workitemtypegroups" - } - }, - "workitemtypes": { - "links": { - "related": "http://f8wit-fabric8-build.devtools-dev.ext.devshift.net/api/spacetemplates/f405fa41-a8bb-46db-8800-2dbe13da1418/workitemtypes" - } - } - }, - "type": "spaces" - } -}` - type PipelineEnvironmentControllerSuite struct { testsuite.DBTestSuite db *gormapp.GormDB @@ -177,16 +74,49 @@ func (s *PipelineEnvironmentControllerSuite) SetupSuite() { os.Setenv("F8_WIT_URL", "http://witservice") // gock.Observe(gock.DumpRequest) - defer func() { - gock.OffAll() - }() + defer gock.OffAll() +} + +func (s *PipelineEnvironmentControllerSuite) createSpaceJson(spaceName string, spaceID uuid.UUID) string { + // TODO: Test ownership + identityID := guuid.NewV4() + desc := "Description of " + spaceName + version := 0 + spaceTime := time.Now() + _spaceID, _ := guuid.FromString(spaceID.String()) + + wt := witservice.SpaceSingle{ + Data: &witservice.Space{ + ID: &_spaceID, + Attributes: &witservice.SpaceAttributes{ + CreatedAt: &spaceTime, + Description: &desc, + Name: &spaceName, + UpdatedAt: &spaceTime, + Version: &version, + }, + Links: &witservice.GenericLinksForSpace{}, + Type: "spaces", + Relationships: &witservice.SpaceRelationships{ + OwnedBy: &witservice.SpaceOwnedBy{ + Data: &witservice.IdentityRelationData{ + ID: &identityID, + Type: "identities", + }, + }, + }, + }, + } + + b, _ := json.Marshal(wt) + return string(b) } func (s *PipelineEnvironmentControllerSuite) createGockONSpace(spaceID uuid.UUID, spaceName string) { gock.New("http://witservice"). Get("/api/spaces/" + spaceID.String()). Reply(200). - JSON(fmt.Sprintf(defaultSpaceJson, "space1", spaceID)) + JSON(s.createSpaceJson(spaceName, spaceID)) } // createPipelineEnvironmentCtrlNoErroring we do this one manually cause the one from From 99210e2119dda5792f78e2ecb6a2b19bdc1399e1 Mon Sep 17 00:00:00 2001 From: Chmouel Boudjnah Date: Thu, 29 Nov 2018 21:57:30 +0100 Subject: [PATCH 5/7] Test errors when failing to get space Probably need some better detection tho and more use cases, --- controller/pipeline_environments_blackbox_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/controller/pipeline_environments_blackbox_test.go b/controller/pipeline_environments_blackbox_test.go index 5a396df..6dc953b 100644 --- a/controller/pipeline_environments_blackbox_test.go +++ b/controller/pipeline_environments_blackbox_test.go @@ -179,6 +179,15 @@ func (s *PipelineEnvironmentControllerSuite) TestCreate() { jerr := s.ctrl2.Create(createCtxerr) require.Nil(t, jerr) require.Equal(t, 400, rw.Code) + + failSpaceID := uuid.NewV4() + gock.New("http://witservice"). + Get("/api/spaces/" + failSpaceID.String()). + Reply(404) + // TODO(chmouel): better testing + payload = newPipelineEnvironmentPayload("space-not-found", uuid.NewV4()) + test.CreatePipelineEnvironmentsInternalServerError(t, s.ctx2, s.svc2, s.ctrl2, space1ID, payload) + require.NotNil(t, response.Header().Get("Location")) }) s.T().Run("unauthorized", func(t *testing.T) { From f704ac47ba1fe23e3aae036f96e401d77e6216a1 Mon Sep 17 00:00:00 2001 From: Chmouel Boudjnah Date: Fri, 30 Nov 2018 10:55:55 +0100 Subject: [PATCH 6/7] Properly error with 404 when space is not found We were just 500ing everything out, let 404 when there is a space not found (and room for other erorr handling cases) --- application/wit/wit.go | 9 +++++++-- controller/pipeline_environments_blackbox_test.go | 14 ++++++++++++-- design/pipelineenv.go | 1 + 3 files changed, 20 insertions(+), 4 deletions(-) diff --git a/application/wit/wit.go b/application/wit/wit.go index fc0181c..774c0b7 100644 --- a/application/wit/wit.go +++ b/application/wit/wit.go @@ -8,10 +8,11 @@ import ( "github.com/fabric8-services/fabric8-build/application/rest" "github.com/fabric8-services/fabric8-build/application/wit/witservice" "github.com/fabric8-services/fabric8-build/configuration" + commonerr "github.com/fabric8-services/fabric8-common/errors" "github.com/fabric8-services/fabric8-common/goasupport" + "github.com/pkg/errors" "github.com/goadesign/goa/uuid" - "github.com/pkg/errors" "github.com/prometheus/common/log" ) @@ -56,7 +57,11 @@ func (s *WITServiceImpl) GetSpace(ctx context.Context, spaceID string) (space *S "response_status": res.Status, "response_body": bodyString, }, "unable to get space from WIT") - return nil, errors.Errorf("unable to get space from WIT. Response status: %s. Response body: %s", res.Status, bodyString) + if res.StatusCode == 404 { + return nil, commonerr.NewNotFoundErrorFromString("Cannot find space: " + spaceID) + } else { + return nil, errors.Errorf("unable to get space from WIT. Response status: %s. Response body: %s", res.Status, bodyString) + } } spaceSingle, err := remoteWITService.DecodeSpaceSingle(res) diff --git a/controller/pipeline_environments_blackbox_test.go b/controller/pipeline_environments_blackbox_test.go index 6dc953b..ec8b739 100644 --- a/controller/pipeline_environments_blackbox_test.go +++ b/controller/pipeline_environments_blackbox_test.go @@ -184,10 +184,20 @@ func (s *PipelineEnvironmentControllerSuite) TestCreate() { gock.New("http://witservice"). Get("/api/spaces/" + failSpaceID.String()). Reply(404) - // TODO(chmouel): better testing payload = newPipelineEnvironmentPayload("space-not-found", uuid.NewV4()) - test.CreatePipelineEnvironmentsInternalServerError(t, s.ctx2, s.svc2, s.ctrl2, space1ID, payload) + response, err = test.CreatePipelineEnvironmentsNotFound(t, s.ctx2, s.svc2, s.ctrl2, failSpaceID, payload) require.NotNil(t, response.Header().Get("Location")) + assert.Regexp(s.T(), ".*not_found.*", err.Errors) + + failSpaceID = uuid.NewV4() + gock.New("http://witservice"). + Get("/api/spaces/" + failSpaceID.String()). + Reply(422) + payload = newPipelineEnvironmentPayload("space-unkown-error", uuid.NewV4()) + response, err = test.CreatePipelineEnvironmentsInternalServerError(t, s.ctx2, s.svc2, s.ctrl2, failSpaceID, payload) + require.NotNil(t, response.Header().Get("Location")) + assert.Regexp(s.T(), ".*unknown_error.*", err.Errors) + }) s.T().Run("unauthorized", func(t *testing.T) { diff --git a/design/pipelineenv.go b/design/pipelineenv.go index 4136320..ad4e39e 100644 --- a/design/pipelineenv.go +++ b/design/pipelineenv.go @@ -49,6 +49,7 @@ var _ = a.Resource("PipelineEnvironments", func() { a.Response(d.Unauthorized, JSONAPIErrors) a.Response(d.MethodNotAllowed, JSONAPIErrors) a.Response(d.Conflict, JSONAPIErrors) + a.Response(d.NotFound, JSONAPIErrors) }) a.Action("show", func() { From d0952bd257ae865fa8c3922797b2a2d864b05a97 Mon Sep 17 00:00:00 2001 From: Chmouel Boudjnah Date: Fri, 30 Nov 2018 11:18:10 +0100 Subject: [PATCH 7/7] Fix some idomatic golangci warnings --- application/rest/url.go | 4 ++-- controller/pipeline_environments_blackbox_test.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/application/rest/url.go b/application/rest/url.go index 056c86a..e8b9079 100644 --- a/application/rest/url.go +++ b/application/rest/url.go @@ -31,12 +31,12 @@ func AbsoluteURLAsURL(req *http.Request, relative string) (*url.URL, error) { // ReadBody reads body from a ReadCloser and returns it as a string func ReadBody(body io.ReadCloser) string { buf := new(bytes.Buffer) - buf.ReadFrom(body) + _, _ = buf.ReadFrom(body) return buf.String() } // CloseResponse reads the body and close the response. To be used to prevent file descriptor leaks. func CloseResponse(response *http.Response) { - ioutil.ReadAll(response.Body) + _, _ = ioutil.ReadAll(response.Body) response.Body.Close() } diff --git a/controller/pipeline_environments_blackbox_test.go b/controller/pipeline_environments_blackbox_test.go index ec8b739..7952e61 100644 --- a/controller/pipeline_environments_blackbox_test.go +++ b/controller/pipeline_environments_blackbox_test.go @@ -52,7 +52,7 @@ func TestPipelineEnvironmentController(t *testing.T) { func (s *PipelineEnvironmentControllerSuite) SetupSuite() { s.DBTestSuite.SetupSuite() - config, err := configuration.New("") + config, _ := configuration.New("") s.db = gormapp.NewGormDB(s.DB)