-
Notifications
You must be signed in to change notification settings - Fork 397
Migrate Project Config service calls to v2. #665
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
Conversation
2e9a822
to
fdaa025
Compare
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.
Sorry for the delay. I needed to confirm with some folks about some additional rename they wanted to do.
@@ -2515,7 +2515,7 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => { | |||
|
|||
describe('getOAuthIdpConfig()', () => { | |||
const providerId = 'oidc.provider'; | |||
const path = handler.path('v2beta1', `/oauthIdpConfigs/${providerId}`, 'project_id'); |
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.
We also want to rename the APIs to be consistent with the SAML counterparts here and elsewhere.
oauthIdpConfigs
-> inboundOauthConfigs
Check the proto file for confirmation.
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.
Seems like oauthIdpConfigs was kept for OnePlatform compliancy.
src/auth/auth-api-request.ts
Outdated
@@ -1644,7 +1644,7 @@ export class TenantAwareAuthRequestHandler extends AbstractAuthRequestHandler { | |||
* @return {AuthResourceUrlBuilder} A new project config resource URL builder instance. | |||
*/ | |||
protected newProjectConfigUrlBuilder(): AuthResourceUrlBuilder { | |||
return new TenantAwareAuthResourceUrlBuilder(this.projectId, 'v2beta1', this.tenantId); | |||
return new TenantAwareAuthResourceUrlBuilder(this.projectId, 'admin/v2', this.tenantId); |
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.
It seems they plan to remove the admin/
prefix as it is not one platform compliant (it will break the API discovery doc).
They are working on it now.
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.
Removed the prefix from the calls.
No description provided.