Skip to content

Add class for SAML provider config.#419

Merged
micahstairs merged 5 commits intoidp-configfrom
micahstairs-saml
May 18, 2020
Merged

Add class for SAML provider config.#419
micahstairs merged 5 commits intoidp-configfrom
micahstairs-saml

Conversation

@micahstairs
Copy link
Contributor

Adds a class for the SAML provider config.

Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

Looks pretty good. Just need to remove some of the nested types from the public API surface.

Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

Looks good. Please fix the compilation failure.

@micahstairs micahstairs changed the base branch from micahstairs-provider-cleanup to idp-config May 18, 2020 19:09
Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

I was under the impression that we can deserialize the response into an instance of SamlProviderConfigResponse, and then realized we can't really do that. I've proposed a couple of options to clean things up a bit. Let me know how you feel about them.

@hiranya911 hiranya911 assigned micahstairs and unassigned hiranya911 May 18, 2020
Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM 👍

@hiranya911 hiranya911 assigned micahstairs and unassigned hiranya911 May 18, 2020
@micahstairs micahstairs merged commit 51fdc91 into idp-config May 18, 2020
micahstairs added a commit that referenced this pull request Jun 12, 2020
Adds a class for the SAML provider config.
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.

3 participants