Skip to content
This repository has been archived by the owner on Mar 11, 2021. It is now read-only.

Reverse lookup tenant by cluster/namespace (openshiftio/openshiftio#1389) #476

Merged

Conversation

xcoulon
Copy link
Contributor

@xcoulon xcoulon commented Nov 22, 2017

Added an endpoint for GET /api/tenants?cluster_url=x&namespace=y
The response will be a list of tenants with a single entry, or 404
if none was found.
The request MUST be sent using the SA token.

Fixes openshiftio/openshiftio#1389

Signed-off-by: Xavier Coulon xcoulon@redhat.com

…389)

Added an endpoint for `GET /api/tenants?cluster_url=x&namespace=y`
The response will be a list of tenants with a single entry, or `404`
if none was found.
The request MUST be sent using the SA token.

Fixes openshiftio/openshiftio#1389

Signed-off-by: Xavier Coulon <xcoulon@redhat.com>
@fabric8cd
Copy link
Contributor

@xcoulon snapshot fabric8-tenant image is available for testing. docker pull docker.io/fabric8/fabric8-tenant:SNAPSHOT-PR-476-1

},
// skipping the paging links for now
Meta: &app.TenantListMeta{
TotalCount: 1,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there only can be one result, why bother with a list at all?

Copy link
Contributor

@kwk kwk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM apart from one comment.

func (s *TenantServiceTestSuite) TestLookupTenantByNamespace() {
s.T().Run("ok", func(t *testing.T) {
// given
fxt := testfixture.NewTestFixture(s.T(), s.DB, testfixture.Tenants(1), testfixture.Namespaces(1))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically fxt := testfixture.NewTestFixture(s.T(), s.DB, testfixture.Namespaces(1)) is enough due to the dependency on Tenants right?

Btw. I would import the testfixture as tf for more readability.

Also, you must not pass s.T() but t here and in all other places as well. Failing an outer test from the inside is really hard to debug.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah right, I had changed a few t / s.T() as I remembered you already warned me about this bug, but I guess I forgot a few places :/ And yes, fxt := testfixture.NewTestFixture(s.T(), s.DB, testfixture.Namespaces(1)) should be enough, but I wanted to be explicit on the fact that we need a Tenant because it's what we're actually searching in this test.

Signed-off-by: Xavier Coulon <xcoulon@redhat.com>
@@ -37,7 +37,7 @@ func makeNamespaces(fxt *TestFixture) error {
tenantService := tenant.NewDBService(fxt.db)
for i := range fxt.Namespaces {
fxt.Namespaces[i] = &tenant.Namespace{
Type: tenant.TypeUser,
Type: tenant.TypeChe,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what the default type should be here but I trust you.

Copy link
Contributor

@kwk kwk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGMT

@fabric8cd
Copy link
Contributor

@xcoulon snapshot fabric8-tenant image is available for testing. docker pull docker.io/fabric8/fabric8-tenant:SNAPSHOT-PR-476-2

@xcoulon
Copy link
Contributor Author

xcoulon commented Nov 22, 2017

[test]

Copy link
Contributor

@alexeykazakov alexeykazakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't test this PR! But the changes look good to me overall.

@xcoulon
Copy link
Contributor Author

xcoulon commented Nov 22, 2017

thanks for the reviews, @kwk and @alexeykazakov !

@fabric8cd
Copy link
Contributor

@xcoulon snapshot fabric8-tenant image is available for testing. docker pull docker.io/fabric8/fabric8-tenant:SNAPSHOT-PR-476-3

@fabric8cd
Copy link
Contributor

@xcoulon snapshot fabric8-tenant image is available for testing. docker pull docker.io/fabric8/fabric8-tenant:SNAPSHOT-PR-476-4

@xcoulon xcoulon merged commit 839c30c into fabric8-services:master Nov 23, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants