-
Notifications
You must be signed in to change notification settings - Fork 375
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Defines tenant management API on AuthRequestHandler. #559
Conversation
…est with no type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good. I just pointed out a few nits.
src/auth/auth-api-request.ts
Outdated
* @param {TenantOptions} tenantOptions The properties to set on the new tenant to be created. | ||
* @return {Promise<TenantServerResponse>} A promise that resolves with the newly created tenant object. | ||
*/ | ||
public createTenant(tenantOptions: TenantOptions): Promise<object> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set return type to Promise<TenantServerResponse>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/auth/auth-api-request.ts
Outdated
} catch (e) { | ||
return Promise.reject(e); | ||
} | ||
return this.invokeRequestHandler(this.tenantMgmtResourceBuilder, CREATE_TENANT, request) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: You can move this inside the try block. Then request
can be a const
. Here and other places where this pattern is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/auth/auth-api-request.ts
Outdated
* @param {TenantOptions} tenantOptions The properties to update on the existing tenant. | ||
* @return {Promise<TenantServerResponse>} A promise that resolves with the modified tenant object. | ||
*/ | ||
public updateTenant(tenantId: string, tenantOptions: TenantOptions): Promise<object> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set the more specific return type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
describe('updateTenant', () => { | ||
const path = '/v2beta1/projects/project_id/tenants/tenant_id'; | ||
const method = 'PATCH'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be call this patchMethod
? That will make the code that uses it more readable:
expect(stub).to.have.been.calledOnce.and.calledWith(callParams(expectedPath, patchMethod, expectedRequest));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
describe('createTenant', () => { | ||
const path = '/v2beta1/projects/project_id/tenants'; | ||
const method = 'POST'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Perhaps call it postMethod
for clarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Defines tenant management API on AuthRequestHandler.