validation added for create env #22
Conversation
Codecov Report
@@ Coverage Diff @@
## master #22 +/- ##
==========================================
- Coverage 61.76% 61.53% -0.23%
==========================================
Files 6 6
Lines 306 325 +19
==========================================
+ Hits 189 200 +11
- Misses 104 111 +7
- Partials 13 14 +1
Continue to review full report at Codecov.
|
nurali-techie
changed the title
controller/environment.go
Outdated
| return nil | ||
| } | ||
| } | ||
| return errors.NewInternalErrorFromString(fmt.Sprintf("cluster with URL '%s' not linked with user account", clusterURL)) |
There was a problem hiding this comment.
why do you return an InternalServerError here? Could you return something more explicit, such as Forbidden if the cluster is not linked to the user?
| spaceID := uuid.NewV4() | ||
| payload := newCreateEnvironmentPayload("osio-stage", "stage", "cluster2.com") | ||
|
|
||
| _, err := test.CreateEnvironmentInternalServerError(t, s.ctx, s.svc, s.ctrl, spaceID, payload) |
There was a problem hiding this comment.
see question above about the InternalServerError returned when the cluster is not linked to the user.
| @@ -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 { | |||
There was a problem hiding this comment.
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.
ok, fair enough, if all cases in the this function are covered but your blackbox tests
| @@ -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.
FYI, if can be simplified in the future, once fabric8-services/fabric8-cluster#59 is done
There was a problem hiding this comment.
We can't use /api/clusters with URL as we need clusters who linked to users. For ex, we call /api/clusters with URL and we get response with cluster_details. It just mean that the cluster is a valid cluster and available with cluster_service but it can't guarantee that the cluster is linked to user also.
There was a problem hiding this comment.
@nurali-techie ok, my bad indeed! In that case, would you need a new endpoint such as /api/user/cluster?url=... to get a single response (or 404 if the user is not linked to the cluster with the given URL)?
There was a problem hiding this comment.
kool, so we don't have such a endpoint today.
There was a problem hiding this comment.
right, but if you need it, please open an issue and I'll take care of it
There was a problem hiding this comment.
thanks for taking my change request into account @nurali-techie ;)
|
waiting for this https://gitlab.cee.redhat.com/dtsd/housekeeping/issues/2504 |
Validation added for clusterApiURL.
Fixes: #20