feat(#729): initialize tenant when GET call is received and don't wait for POST #742

MatousJobanek · 2019-01-29T14:42:16Z

tenant initializes namespaces (and creates DB records) not only for POST call but also for GET call that is received with a user token.
checks for the existence of all namespaces - in other words - when one namespace is missing for a particular tenant, then it initializes it

Fixes: #729

…xtensible and replace oc commands (fabric8-services#673)

…ot present (fabric8-services#733)

…fabric8-services#738)

…mplates) (fabric8-services#739)

* chore: make warn log shorter

…ed and don't wait for POST

MatousJobanek · 2019-01-29T16:13:37Z

[test]

codecov · 2019-01-29T16:25:42Z

Codecov Report

Merging #742 into master will decrease coverage by 0.44%.
The diff coverage is 52.32%.

@@            Coverage Diff             @@
##           master     #742      +/-   ##
==========================================
- Coverage   75.24%   74.79%   -0.45%     
==========================================
  Files          37       37              
  Lines        3013     3031      +18     
==========================================
  Hits         2267     2267              
- Misses        561      574      +13     
- Partials      185      190       +5

Impacted Files	Coverage Δ
openshift/types.go	`92.18% <100%> (ø)`	⬆️
openshift/service.go	`72.63% <100%> (ø)`	⬆️
cluster/service.go	`79.38% <100%> (ø)`	⬆️
controller/tenant.go	`68.29% <48.75%> (-3.64%)`	⬇️
update/update.go	`80.37% <0%> (-1.87%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c2ea27...a326890. Read the comment docs.

xcoulon · 2019-02-06T10:09:27Z

controller/tenant.go

+func (c *TenantController) getOrInitTenant(ctx context.Context, user *auth.User) (*tenant.Tenant, error) {
+	var dbTenant *tenant.Tenant
+	var err error
+	tenantRepository := c.tenantService.NewTenantRepository(user.ID)


why do you pass the user.ID in the tenant repository constructor? shouldn't it be user-agnostic (hence, pass the user.ID as an arg when calling the methods. eg: repository.Exists(user.ID))

Because the TenantRepository is specific for one tenant. In one request I'm dealing with one tenant and this helps me to minimize the number of parameters passed to other methods/objects - I don't have to pass DB service along with the tenant id because it is already contained within the repository: https://github.com/fabric8-services/fabric8-tenant/blob/develop/tenant/repository.go#L138

yes, that's what I understood, but it feels unusual to construct the repository with the tenant associated to it. My point is that it does not follow our pattern for services and repositories.

For me it makes sense - it represents a repository for one tenant entity comparing to the DBService that represents a service for all other cases (not related to one tenant).
But I'm open to any proposal ;-).
we already had some discussions about it:
#673 (comment)
#732 (comment)

I have a big problem with these namings so I would welcome any help :-)

controller/tenant_parallel_test.go

xcoulon · 2019-02-06T10:13:04Z

controller/tenant.go

@@ -246,7 +190,24 @@ func (c *TenantController) Show(ctx *app.ShowTenantContext) error {
 		return jsonapi.JSONErrorResponse(ctx, err)
 	}

-	return ctx.OK(&app.TenantSingle{Data: convertTenant(ctx, tenant, namespaces, c.clusterService.GetCluster)})
+	// check if any environment type is missing - should be provisioned
+	missing, _ := filterMissingAndExisting(namespaces)


do you have a test for this filterMissingAndExisting func?

only indirect tests - not direct
see:

when no namespace exists
TestShowTenantOKWhenNoTenantExists
TestSetupTenantOKWhenNoTenantExists

when some namespaces exist
TestShowTenantWhenSomeNamespacesAreMissing
TestSetupTenantOKWhenAlreadyExists

when all namespace exist
TestSetupConflictFailure
TestShowTenant

in https://github.com/fabric8-services/fabric8-tenant/pull/742/files#diff-f9e0701950f6288288104f0f34160817

xcoulon · 2019-02-06T10:14:26Z

controller/tenant.go

+}
+
+func (c *TenantController) createNamespaces(ctx context.Context, envTypes []environment.Type, user *auth.User, dbTenant *tenant.Tenant) error {
+	clusterNsMapping, err := c.clusterService.GetUserClusterForType(ctx, user)


why is this method called GetUserClusterForType? what is the type here?

https://github.com/fabric8-services/fabric8-tenant/blob/develop/cluster/service.go#L34
I could maybe change it to GetUserClusterForEnvType

but you only pass the context and user as args here, so why ForType or ForEnvType ?

it returns a function ForType that returns cluster for the given environment type. To point here is that it was a preparation for a multi-cluster environment (when one env type can be associated with a different cluster), that's why such a mapping. In this case, it just returns the same cluster (the user cluster) for all env types. This function is then used in a generic way.

controller/tenant.go

MatousJobanek · 2019-03-06T08:20:07Z

Closing this issue as it is not relevant anymore. In addition, as there is a Get call before the first Post call, then it would be too early to start provisioning for the first Get call

MatousJobanek and others added 10 commits January 22, 2019 09:46

chore(fabric8-services#678): refactor core functionality to be more e…

5db658b

…xtensible and replace oc commands (fabric8-services#673)

fix: when checks for presence of namespaces then 403 treat as it is n…

c10fdca

…ot present (fabric8-services#733)

fix: use creation token also for cheking if ns was created (fabric8-s…

8e657d9

…ervices#734)

fix: handle conflicts for resource quotas correctly (fabric8-service…

da299a7

…s#735)

fix: wait until PVC is removed or set to terminating state (fabric8-s…

9aaa7d0

…ervices#736)

fix: just try to wait until is removed and don't fail when is not (fa…

94f6c55

…bric8-services#737)

fix: reset result when either 404 or 409 is ignored (during deletion) (…

8e221ab

…fabric8-services#738)

fix: clean should delete all resources (not only those that are in te…

6a2111a

…mplates) (fabric8-services#739)

chore: use callbacks as one nested function chain (fabric8-services#740)

e839d84

* chore: make warn log shorter

feat(fabric8-services#729): initialize tenant when GET call is receiv…

92d2489

…ed and don't wait for POST

alien-ike added the work-in-progress label Jan 29, 2019

MatousJobanek changed the title ~~WIP feat(#729): initialize tenant when GET call is received and don't wait for POST~~ feat(#729): initialize tenant when GET call is received and don't wait for POST Feb 5, 2019

alien-ike removed the work-in-progress label Feb 5, 2019

MatousJobanek requested review from xcoulon, alexeykazakov, nurali-techie and dipak-pawar February 6, 2019 08:39

xcoulon reviewed Feb 6, 2019

View reviewed changes

MatousJobanek changed the base branch from develop to master February 6, 2019 14:40

MatousJobanek added 3 commits February 6, 2019 14:43

Merge branch 'master' into issue-729-lazy-init

9bd6a3c

rename ForType to ForEnvType

afcf8e3

fix parallel tests

a326890

MatousJobanek closed this Mar 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(#729): initialize tenant when GET call is received and don't wait for POST #742

feat(#729): initialize tenant when GET call is received and don't wait for POST #742

MatousJobanek commented Jan 29, 2019 •

edited

Loading

MatousJobanek commented Jan 29, 2019

codecov bot commented Jan 29, 2019 •

edited

Loading

xcoulon Feb 6, 2019

MatousJobanek Feb 6, 2019

xcoulon Feb 6, 2019

MatousJobanek Feb 6, 2019

xcoulon Feb 6, 2019

MatousJobanek Feb 6, 2019

xcoulon Feb 6, 2019

MatousJobanek Feb 6, 2019

xcoulon Feb 6, 2019

MatousJobanek Feb 6, 2019

MatousJobanek commented Mar 6, 2019

feat(#729): initialize tenant when GET call is received and don't wait for POST #742

feat(#729): initialize tenant when GET call is received and don't wait for POST #742

Conversation

MatousJobanek commented Jan 29, 2019 • edited Loading

MatousJobanek commented Jan 29, 2019

codecov bot commented Jan 29, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatousJobanek commented Mar 6, 2019

MatousJobanek commented Jan 29, 2019 •

edited

Loading

codecov bot commented Jan 29, 2019 •

edited

Loading