-
Notifications
You must be signed in to change notification settings - Fork 4
validation added for create env #22
Changes from all commits
45bcb90
8f58d0a
6a4b7ef
4156698
98e2b20
e1d9217
a15f2d7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,10 @@ | ||
package controller | ||
|
||
import ( | ||
"context" | ||
"fmt" | ||
|
||
clusterclient "github.com/fabric8-services/fabric8-cluster-client/service" | ||
"github.com/fabric8-services/fabric8-common/auth" | ||
"github.com/fabric8-services/fabric8-common/errors" | ||
"github.com/fabric8-services/fabric8-common/httpsupport" | ||
|
@@ -18,15 +22,17 @@ const ( | |
|
||
type EnvironmentController struct { | ||
*goa.Controller | ||
db application.DB | ||
authService auth.AuthService | ||
db application.DB | ||
authService auth.AuthService | ||
clusterService clusterclient.Service | ||
} | ||
|
||
func NewEnvironmentController(service *goa.Service, db application.DB, authService auth.AuthService) *EnvironmentController { | ||
func NewEnvironmentController(service *goa.Service, db application.DB, authService auth.AuthService, clusterService clusterclient.Service) *EnvironmentController { | ||
return &EnvironmentController{ | ||
Controller: service.NewController("EnvironmentController"), | ||
db: db, | ||
authService: authService, | ||
Controller: service.NewController("EnvironmentController"), | ||
db: db, | ||
authService: authService, | ||
clusterService: clusterService, | ||
} | ||
} | ||
|
||
|
@@ -65,6 +71,11 @@ func (c *EnvironmentController) Create(ctx *app.CreateEnvironmentContext) error | |
return app.JSONErrorResponse(ctx, err) | ||
} | ||
|
||
err = c.checkClustersUser(ctx, *reqEnv.Attributes.ClusterURL) | ||
if err != nil { | ||
return app.JSONErrorResponse(ctx, err) | ||
} | ||
|
||
var env *environment.Environment | ||
err = application.Transactional(c.db, func(appl application.Application) error { | ||
newEnv := environment.Environment{ | ||
|
@@ -78,8 +89,8 @@ func (c *EnvironmentController) Create(ctx *app.CreateEnvironmentContext) error | |
env, err = appl.Environments().Create(ctx, &newEnv) | ||
if err != nil { | ||
log.Error(ctx, map[string]interface{}{"err": err}, | ||
"failed to create environment: %s", newEnv.Name) | ||
return errs.Wrapf(err, "failed to create environment: %s", newEnv.Name) | ||
"failed to create environment: %s", *newEnv.Name) | ||
return errs.Wrapf(err, "failed to create environment: %s", *newEnv.Name) | ||
} | ||
return nil | ||
}) | ||
|
@@ -132,3 +143,17 @@ func (c *EnvironmentController) Show(ctx *app.ShowEnvironmentContext) error { | |
} | ||
return ctx.OK(res) | ||
} | ||
|
||
func (c *EnvironmentController) checkClustersUser(ctx context.Context, clusterURL string) error { | ||
clusters, err := c.clusterService.UserClusters(ctx) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI, if can be simplified in the future, once fabric8-services/fabric8-cluster#59 is done There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can't use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nurali-techie ok, my bad indeed! In that case, would you need a new endpoint such as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. kool, so we don't have such a endpoint today. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. right, but if you need it, please open an issue and I'll take care of it |
||
if err != nil { | ||
return err | ||
} | ||
clusterURL = httpsupport.RemoveTrailingSlashFromURL(clusterURL) | ||
for _, cluster := range clusters.Data { | ||
if httpsupport.RemoveTrailingSlashFromURL(cluster.APIURL) == clusterURL { | ||
return nil | ||
} | ||
} | ||
return errors.NewForbiddenError(fmt.Sprintf("cluster with URL '%s' not linked with user account", clusterURL)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you add a test for this func?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this link test is added to test this function only. If you observe blackbox_tests, it is a way to test all different combination. This new test is added such that only
checkClustersUser
function is tested rest are assumed to be working. Do you still see need to add test, which will have less pre-setup but everything else same as from part of blackbox test ??There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, fair enough, if all cases in the this function are covered but your blackbox tests