Skip to content
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

feat: Adding support for login theme on saml_client resource #590

Merged

Conversation

diffusedlight
Copy link
Contributor

I've been using the provider for a little while and noticed the SAML Client doesn't support setting the login theme via TF. This should enable that. If there's anything that needs tweaking let me know.

@diffusedlight diffusedlight changed the title feat: Adding support for login them on saml_client resource feat: Adding support for login theme on saml_client resource Sep 8, 2021
Copy link
Owner

@mrparkers mrparkers left a comment

Choose a reason for hiding this comment

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

Hey @cw1o, thanks for the PR. This looks good, but we're missing calls to data.Get and data.Set, which is responsible for retrieving the value from your HCL configuration and persisting it back to state, respectively.

You'll want to modify these two functions to add these lines of code:

  • mapToSamlClientFromData - we should call data.Get within this function to help build the keycloak.SamlClient to send off to Keycloak
  • mapToDataFromSamlClient - we should call data.Set within this function to persist the value from Keycloak back to Terraform state.

@diffusedlight
Copy link
Contributor Author

I've updated both of them functions now, tests look okay. That 11.0.3 one might need rerunning don't know what happened there.

Copy link
Owner

@mrparkers mrparkers left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the PR!

@mrparkers mrparkers merged commit 0128742 into mrparkers:master Sep 13, 2021
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