fix(#674): introduce nsUsername used as base-name for namespace names #675

MatousJobanek · 2018-11-01T14:37:15Z

This PR introduces nsUsername in tenant table. It is used as a base-name (eg. johny) for namespace names. When a new tenant is being created, then it checks if there is some tenant or namespace that the same base-name already uses. If there is some, then it appends a number to the name starting with number 2 (eg. johny2) and checks for any conflict again.

… namespace names If there is a tenant or namespace with such a base-name, then it appends number starting with number 2 - eg. johny2

aslakknutsen · 2018-11-01T14:43:42Z

migration/sql-files/006-add-ns-username-column-to-tenant.sql

@@ -0,0 +1,2 @@
+ALTER TABLE tenants
+ADD COLUMN ns_username TEXT;


Maybe add a migration to add the basename of the existing users. Using the namespace.name where type=user should work..

index added: a1914bc
is it fine that it is in the same file? or did you expect another migration file?

Maybe add a migration to add the basename of the existing users. Using the namespace.name where type=user should work.

Do you mean something like this:

UPDATE tenants t SET t.ns_username = (SELECT name FROM namespaces n WHERE t.id = n.tenant_id AND t.type = 'user')

Actually, it's good idea - with this we could easily find those tenants that are not properly provisioned - have no user namespace

Yeah, easy to find anyway, but probably about 20 at this point I think

added the migration command here: 139d178

there was a bug in the SQL command - the correct is:

UPDATE tenants SET ns_base_name = (SELECT name FROM namespaces n WHERE tenants.id = n.tenant_id AND n.type = 'user')

fixed as part of ec19689

codecov · 2018-11-01T14:51:42Z

Codecov Report

Merging #675 into master will increase coverage by 0.34%.
The diff coverage is 58.66%.

@@            Coverage Diff             @@
##           master     #675      +/-   ##
==========================================
+ Coverage   52.34%   52.68%   +0.34%     
==========================================
  Files          28       28              
  Lines        2050     2105      +55     
==========================================
+ Hits         1073     1109      +36     
- Misses        859      878      +19     
  Partials      118      118

Impacted Files	Coverage Δ
tenant/tenant.go	`79.31% <ø> (ø)`	⬆️
openshift/clean_tenant.go	`0% <0%> (ø)`	⬆️
controller/tenant.go	`13.43% <0%> (-0.91%)`	⬇️
openshift/process_template.go	`58.75% <100%> (ø)`	⬆️
environment/template.go	`86.25% <100%> (-0.11%)`	⬇️
migration/migration.go	`36.52% <100%> (+0.55%)`	⬆️
openshift/init_tenant.go	`35% <50%> (-0.65%)`	⬇️
tenant/repository.go	`66.66% <94.87%> (+15.94%)`	⬆️

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 445eb60...ec19689. Read the comment docs.

alexeykazakov · 2018-11-01T14:51:11Z

controller/tenant.go

 	// create openshift config
 	openshiftConfig := openshift.NewConfig(c.config, user.UserData, cluster.User, cluster.Token, cluster.APIURL)
 	tenant := &tenant.Tenant{
 		ID:         ttoken.Subject(),
 		Email:      ttoken.Email(),
 		OSUsername: user.OpenShiftUsername,
+		NsUsername: username,


Can we maybe call it NsNamePrefix or NsBaseName or something like this? The fact that we use a username as a base for that prefix is implementation detail and we can change it in the future. Also it would make it less confusing why this "nsusername" does not always match username.
So, I would like to make it clear across the code that we have clear names for these two concepts here. Username (actual username) and a base-name or prefix which is currently constructed from a username and an index.

I called it ns_username because there in the templates it is used as:

namespace: ${USER_NAME}-che

so, I wanted to make it consitent. Unfortunatelly, the fact that we also have OSUsername that is stored in the table as username make the stuff more complicated.

So, I would like to make it clear across the code that we have clear names for these two concepts here. Username (actual username) and a base-name or prefix which is currently constructed from a username and an index.

The actual username is/was used only for the namespaces - there is no other usage of it (if I'm not mistaken). We only use OSUsername. In other words, the username is only our artificial thingy (implementation detail) that is retrieved from the real OSUsername. Unfortunately, we used this name of the variable also in the templates - which makes sense in case of user namespace.

So, I'll call it NsBaseName and when this PR is merged I'll change also the variables in all templates from ${USER_NAME} to ${NS_BASE_NAME} to make it consistent, if you are fine with it.

Why is naming always so hard? :-/

changed in ec19689

“There are only two hard things in Computer Science: cache invalidation and naming things.”
:)

alexeykazakov · 2018-11-01T18:40:20Z

tenant/repository.go

+	if err != nil {
+		return false
+	}
+	return true


Check if the error is NotFoundError. If yes then return false. Otherwise return the error. Otherwise if there is some problem with the query or DB different than expected Not Found then you will 1) get an infinite loop when constructing an unique name 2) miss opportunity to catch/log some problem with DB.

Good point - I blindly copied the approach from the Exists method that was already there and I didn't realize these cases.
I'll send another PR to fix the Exists method as well.

Done in 39cbe2f

alexeykazakov · 2018-11-01T18:42:12Z

tenant/repository.go

+	if err != nil {
+		return false
+	}
+	return true


Check if the error is NotFoundError. If yes then return false. Otherwise return the error.

done in 39cbe2f

aslakknutsen · 2018-11-02T14:29:32Z

environment/template.go

-		varUserName:              userName,
-		varProjectUser:           user,
-		varProjectRequestingUser: user,
+		varUserName:              nsBaseName,


Depending on how this is used but the naming looks odd.. username != basename

never mind.. {USER_NAME}-che

fix(fabric8-services#674): introduce nsUsername used as base-name for…

6616cc6

… namespace names If there is a tenant or namespace with such a base-name, then it appends number starting with number 2 - eg. johny2

MatousJobanek requested review from aslakknutsen and alexeykazakov November 1, 2018 14:40

aslakknutsen suggested changes Nov 1, 2018

View reviewed changes

add index

a1914bc

MatousJobanek force-pushed the issue-674-ns-username branch from 5f9f190 to a1914bc Compare November 1, 2018 15:07

add migration that sets ns_username in existing tenants

139d178

alexeykazakov suggested changes Nov 1, 2018

View reviewed changes

MatousJobanek added 2 commits November 2, 2018 10:37

check if error returned from DB is ErrRecordNotFound

39cbe2f

renamed username to nsBaseName

ec19689

alexeykazakov approved these changes Nov 2, 2018

View reviewed changes

aslakknutsen reviewed Nov 2, 2018

View reviewed changes

aslakknutsen approved these changes Nov 2, 2018

View reviewed changes

alexeykazakov mentioned this pull request Nov 4, 2018

Sometimes namespace of type 'user' is missing for some users redhat-developer/rh-che#1043

Closed

5 tasks

MatousJobanek merged commit bdeeef1 into fabric8-services:master Nov 5, 2018

MatousJobanek mentioned this pull request Nov 15, 2018

Conflict when two users with same name are registered #674

Closed

MatousJobanek mentioned this pull request Dec 13, 2018

jenkins.http’s server IP address could not be found. openshiftio/openshift.io#4598

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(#674): introduce nsUsername used as base-name for namespace names #675

fix(#674): introduce nsUsername used as base-name for namespace names #675

MatousJobanek commented Nov 1, 2018

aslakknutsen Nov 1, 2018

aslakknutsen Nov 1, 2018

MatousJobanek Nov 1, 2018

MatousJobanek Nov 1, 2018

MatousJobanek Nov 1, 2018

aslakknutsen Nov 1, 2018

MatousJobanek Nov 1, 2018

MatousJobanek Nov 2, 2018

codecov bot commented Nov 1, 2018 •

edited

Loading

alexeykazakov Nov 1, 2018

MatousJobanek Nov 2, 2018

MatousJobanek Nov 2, 2018

alexeykazakov Nov 2, 2018

alexeykazakov Nov 1, 2018

MatousJobanek Nov 2, 2018

MatousJobanek Nov 2, 2018

alexeykazakov Nov 1, 2018

MatousJobanek Nov 2, 2018

aslakknutsen Nov 2, 2018

aslakknutsen Nov 2, 2018

		@@ -0,0 +1,2 @@
		ALTER TABLE tenants
		ADD COLUMN ns_username TEXT;

fix(#674): introduce nsUsername used as base-name for namespace names #675

fix(#674): introduce nsUsername used as base-name for namespace names #675

Conversation

MatousJobanek commented Nov 1, 2018

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

codecov bot commented Nov 1, 2018 • 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

Choose a reason for hiding this comment

codecov bot commented Nov 1, 2018 •

edited

Loading