Skip to content

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

Merged
merged 3 commits into from
Dec 12, 2019
Merged

Conversation

nrsim
Copy link
Contributor

@nrsim nrsim commented Oct 4, 2019

No description provided.

@nrsim nrsim force-pushed the projectconfig-version branch from 2e9a822 to fdaa025 Compare October 4, 2019 20:32
@nrsim nrsim requested a review from bojeil-google October 4, 2019 20:35
@nrsim nrsim assigned nrsim and bojeil-google and unassigned nrsim Oct 4, 2019
Copy link
Contributor

@bojeil-google bojeil-google left a 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');
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@bojeil-google bojeil-google assigned nrsim and unassigned bojeil-google Oct 9, 2019
@@ -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);
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@nrsim nrsim assigned bojeil-google and unassigned nrsim Dec 12, 2019
@nrsim nrsim merged commit a9249a6 into master Dec 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants