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

Add support for authentication execution configuration #241

Merged
merged 1 commit into from
Mar 9, 2020
Merged

Add support for authentication execution configuration #241

merged 1 commit into from
Mar 9, 2020

Conversation

ndrpnt
Copy link
Contributor

@ndrpnt ndrpnt commented Feb 29, 2020

Building on #215, this PR adds support for configuring executions.

I doubt that this is the best implementation as I am a keycloak/terraform newbie, but it seems good enough for my use case.

@mrparkers
Copy link
Contributor

Hi @ndrpnt, thanks for the PR! My only thought here is that it may be more user-friendly to define config as a new attribute on the keycloak_authentication_execution resource instead of having its own resource. What are your thoughts on this?

@mrparkers mrparkers added the question Further information is requested label Mar 4, 2020
@ndrpnt
Copy link
Contributor Author

ndrpnt commented Mar 5, 2020

I could see that being more elegant for the user, but slightly more complex in the code as it doesn't stick to the Keycloak API (one Terraform resource would manage two Keycloak resources). I can give it a try.

@ndrpnt
Copy link
Contributor Author

ndrpnt commented Mar 8, 2020

Hi @mrparkers, getting back to you after trying to merge execution and execution config in a single Terraform resource. It is more complex than I anticipated, and I would rather not waste more time on it, as I don't think this is the best solution.

My unfinished attempt is here. I may be missing something, but I feel like it makes the code more complicated than it needs to be. I ended up in some sort of typecasting/mapping hell (or is it what any provider looks like?) which makes me wonder if I am heading in the right direction.
This is to be compared with the separate resources implementation which was super simple.

Some problems I faced:

  • CRUD operation can be successful on one resource and fail on the other (implies the use of partial state ?)
  • Lifecycle of the resources differ: we should ideally be able to add/update/delete an execution config without affecting the parent execution. This is some logic that we delegate to the Terraform framework when splitting the resources
  • The client offers two separate datastructures (AuthenticationExecution and AuthenticationExecutionConfig) that I have trouble mapping to a single ResourceData struct (maybe using an intermediate datastructure would help?)

What do you think?

@mrparkers
Copy link
Contributor

Thanks for your thoughts! I didn't think my suggestion would bring that many challenges, but I understand that it's difficult to manage this in one terraform resource with the Keycloak API presents them as two separate resources.

I'm fine with merging your PR as-is.

Thanks for the contribution!

@mrparkers mrparkers merged commit e4fa8f2 into keycloak:master Mar 9, 2020
@ndrpnt ndrpnt deleted the add_execution_config branch March 22, 2020 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants