Detect if the user to init on Tenant already has a project on OSO (OSIO#1446) #554
Conversation
|
@xcoulon snapshot fabric8-tenant image is available for testing. |
…IO#1446) Add function to list projects on OSO for the user, given his/her OSO token. Check that there's no project, otherwise, clean/remove all user's namespaces. Include option in User service to support custom HTTP transport. Rename vars in controller.Tenant to avoid collision with packages (`user` and `cluster`). Add test on the controller.TenantController.Setup() method using `go-vcr`. Add a function in `openshift` pkg to retrieve the name of the user's projects (and factorize the code to perform the GET request on the Openshift API endpoint). Also: use encoded token in the go-vcr recording for TestResolveServiceAccountToken Fixes openshiftio/openshift.io#1446 Signed-off-by: Xavier Coulon <[email protected]>
xcoulon
force-pushed
the
IssueOSIO1446_detect_OSO_project
branch
from
bb40a68 to
dc160d3
Compare
|
@xcoulon snapshot fabric8-tenant image is available for testing. |
do not use a go routine at first. Create the user's NS and check the result, then use go routines for all other namespaces to create
xcoulon
force-pushed
the
IssueOSIO1446_detect_OSO_project
branch
from
0eb1052 to
a002aca
Compare
|
@xcoulon snapshot fabric8-tenant image is available for testing. |
- do not call the API to list the user's projects - process the ProjectRequest response and return an error if something wrong happened (403, 409, 500) Signed-off-by: Xavier Coulon <[email protected]>
xcoulon
force-pushed
the
IssueOSIO1446_detect_OSO_project
branch
from
5bda3d5 to
44d965a
Compare
|
@xcoulon snapshot fabric8-tenant image is available for testing. |
…st header Added some a custom method on the generated Goa contexts to support `AbsolutURL` while taking into account the optional `X-Forwarded-Path` request headers Signed-off-by: Xavier Coulon <[email protected]>
Signed-off-by: Xavier Coulon <[email protected]>
also, splitted the single, wayyy too long `go-vcr` recording file (yaml) into 1 file per test, to better understand and monitor the requests that are executed. Signed-off-by: Xavier Coulon <[email protected]>
xcoulon
force-pushed
the
IssueOSIO1446_detect_OSO_project
branch
from
a32a9e5 to
c4d7a35
Compare
Signed-off-by: Xavier Coulon <[email protected]>
…ruct Signed-off-by: Xavier Coulon <[email protected]>
|
@xcoulon snapshot fabric8-tenant image is available for testing. |
Signed-off-by: Xavier Coulon <[email protected]>
Signed-off-by: Xavier Coulon <[email protected]>
|
@xcoulon snapshot fabric8-tenant image is available for testing. |
Makefile
Outdated
| @@ -215,6 +215,9 @@ app/controllers.go: $(DESIGNS) $(GOAGEN_BIN) $(VENDOR_DIR) | |||
| $(GOAGEN_BIN) controller -d ${PACKAGE_NAME}/${DESIGN_DIR} -o controller/ --pkg controller --app-pkg app | |||
| $(GOAGEN_BIN) swagger -d ${PACKAGE_NAME}/${DESIGN_DIR} | |||
| $(GOAGEN_BIN) client -d github.com/fabric8-services/fabric8-auth/design --notool --out auth --pkg client | |||
| $(GOAGEN_BIN) client -d github.com/fabric8-services/fabric8-auth/design --notool --out auth --pkg client | |||
There was a problem hiding this comment.
above line is a duplicate.
There was a problem hiding this comment.
nice catch, thanks!
| @@ -0,0 +1,173 @@ | |||
| package errors | |||
There was a problem hiding this comment.
Why create a new one when we already have it in wit?
There was a problem hiding this comment.
because I need some type of errors that are specific to tenant and openshift.
There was a problem hiding this comment.
is there a way to put it in a place that like in https://github.com/fabric8-services and then use it from library? Won't this add up the code duplication? And errors/bgs getting replicated?
There was a problem hiding this comment.
yes, I've been thinking about it for a few packages, including errors and log amongst others. It would probably make sense to have something like fabric8-commons for all those shared pkgs. But that should be part of another PR, IMO
| _, err := executeRequest(context.Background(), | ||
| request{ | ||
| method: "DELETE", | ||
| url: fmt.Sprintf("%s/oapi/v1/projects/%s", strings.TrimSuffix(clusterURL, "/"), name), |
There was a problem hiding this comment.
why can't we use the client APIs to do this? https://github.com/kubernetes/client-go or if it is a OpenShift specific operation then we can also use https://github.com/openshift/client-go
There was a problem hiding this comment.
I'm fine with that. @aslakknutsen, what do you think about it ?
There was a problem hiding this comment.
We could, but Openshift go client has historically been a beast and we only need like 2 simple REST calls
There was a problem hiding this comment.
ah ok, maybe we would wanna consider moving all our api calls to client-go of openshift!
test/recorder/recorder.go
Outdated
| // "cassette_method": cassetteRequest.Method, | ||
| // "http_request_url": httpRequest.URL, | ||
| // "cassette_url": cassetteRequest.URL, | ||
| // }, "Cassette method/url doesn't match with the current request") |
Signed-off-by: Xavier Coulon <[email protected]>
|
@xcoulon snapshot fabric8-tenant image is available for testing. |
|
@xcoulon snapshot fabric8-tenant image is available for testing. |
|
@xcoulon snapshot fabric8-tenant image is available for testing. |
Signed-off-by: Xavier Coulon <[email protected]>
xcoulon
force-pushed
the
IssueOSIO1446_detect_OSO_project
branch
from
28b8cce to
037eb0a
Compare
|
@xcoulon snapshot fabric8-tenant image is available for testing. |
@aslakknutsen could you clarify on the 2 problems you reported ? |
Fixes fabric8-services#592 Signed-off-by: Xavier Coulon <[email protected]>
Signed-off-by: Xavier Coulon <[email protected]>
Signed-off-by: Xavier Coulon <[email protected]>
|
@xcoulon snapshot fabric8-tenant image is available for testing. |
|
@xcoulon CreateTenant is never called: https://github.com/fabric8-services/fabric8-tenant/pull/554/files#diff-dfb0ecdc519ac765cb68432d078cc1d0L104 In the current v we changed it to use Create instead of Save, so the second attempt to provision the same user would fail. As there is no real clean path to when we provision a user in the live v we can get multiple concurrent requests to provision. So to avoid the second request starting the same process, they use the DB as a 'lock', only 1 request can sucessfully INSERT a tenant record. One of the two concurrent requests will pass, the rest will fail. |
|
@xcoulon About Che and Jenkins I'm unsure. It might be related to a local issue when I tried to run it, but all namespaces except Che and Jenkins provisioned just fine. Che and Jenkins failed for some reason. |
|
@xcoulon snapshot fabric8-tenant image is available for testing. |
|
[test] |
Codecov Report
@@ Coverage Diff @@
## master #554 +/- ##
===========================================
+ Coverage 35.37% 50.17% +14.79%
===========================================
Files 31 36 +5
Lines 2086 2326 +240
===========================================
+ Hits 738 1167 +429
+ Misses 1259 1036 -223
- Partials 89 123 +34
Continue to review full report at Codecov.
|
Signed-off-by: Xavier Coulon <[email protected]>
|
@aslakknutsen I addressed the comment about the missing record in the db (#554 (comment)). Please let me know if it's ok now, and if the Che and Jenkins namespaces still cause troubles for you |
|
@xcoulon snapshot fabric8-tenant image is available for testing. |
|
[test] |
|
This seems to be outdated. Closing. |
Add function to list projects on OSO for the user, given his/her OSO token.
Check that there's no project, or a single project named after the user.
Include option in User service to support custom HTTP transport.
Rename vars in controller.Tenant to avoid collision with packages (
userandcluster).Add test on the controller.TenantController.Setup() method using
go-vcr.Add a function in
openshiftpkg to retrieve the name of the user's projects (and factorizethe code to perform the GET request on the Openshift API endpoint).
Also: use encoded token in the go-vcr recording for TestResolveServiceAccountToken
Fixes openshiftio/openshift.io#1446
Signed-off-by: Xavier Coulon [email protected]