search

mayope / keycloakmigration

Manage your Keycloak configuration with code.
https://mayope.net
MIT License
108 stars 22 forks source link

ProtocolMappers for Clients and ClientScopes #31

Closed JanUnld closed 3 years ago

JanUnld commented 3 years ago

Hey, first of all thanks for the work you already put into this tool 🚀❤️. It totally catched my attention after a collegue shared it with me. I immediatly tried it out and it turned out to work very well for us. During the integration refactoring I'm currently progressing with we also stumbeled across an additional feature the community might benefit from:

ProtocolMapper Actions for ClientScopes

Anyways, I tried to use some free time to put this PR together. To summarize what's included:

I'm totally new to Kotlin and Java, it's actually the first time I touched Kotlin at all, but it honestly was a breeze. If there's anything I can further contribute or if the PR is totally "of the board", let me know 😉

Cheers ✌️

klg71 commented 3 years ago

Hey Jan, thanks for contributing ❤️. I skimmed over the pullrequest and maybe we can make it work without having an abstract class. My suggestion would be, that we leave it as separate commands addClientMapper and addClientScopeMapper. We can then reuse the functionality of adding a mapper for the second case. I favor composition over inheritance and I would prefer to have a lot of simple actions that do one thing over complex actions.

Maybe there is something I am missing here why we can't make it work like that or if this doesn't resonate with your style of code at all we could schedule a call for this because code reviews only in texts are always hard in my eyes. Best Regards Lukas

JanUnld commented 3 years ago

Hey Lukas 🙂 thanks for the feedback!

Sounds totally reasonable to me, I would try to adjust my impls accordingly. I definitely want to comply with your design thinking here! So to shortly summarize what I understood and would do out of you feedback:

Do we want to consider updateClient*Mapper and updateClientScope*Mapper actions as well? 🤔😄

Re: Abstraction & Composition

If you prefer we can actually also schedule a call for a "deeper dive" but I just wanted to give you a short response on the abstraction topic. I actually totally agree to you "composition over abstraction" preference, even tho I never actually thought of it that way 🙂. I think we might benefit from a slightly less complex AddProtocolMapperAction base class here in terms of "maintainability". The key point is that we wouldn't need to implement the "same" subsequent Action.execute and Action.undo functions for each individual mapper action type, even tho you could. Anyways, if you prefer to not do that, than that's totally fine for me 😉, might just take some more effort 😁.

Thanks again for the response!

Best regards Jan

klg71 commented 3 years ago

Thanks for the feedback. :) I think this all sounds reasonable. I would like to leave addMapper and deleteMapper in the ActionFactory as long as we don't have a migration path for them and this is definitely out of scope. But we should remove them from the docs and only mention addClientMapper, deleteClientMapper in the documentation.

I don't need the update-Mapper commands right now because you can always delete and recreate them without loosing any data but if you wish you can surely implement them. Thanks for the work you put into it ❤️ most times I just get issues of things i better immediately implement :D Best Regards Lukas

JanUnld commented 3 years ago

Hey Lukas 👋 🙂 so I did the changes we discussed and updated the doc contents and tests accordingly. Let me know if there's anything else I can do! Happy reviewing 😉

I also added a few @Deprecated annotations, just tell me if we want them a bit more comprehensive regardnig the replaceWith expressions.

klg71 commented 3 years ago

Looks all good so far for me :) a minor type in the docs. If you want you could do a full integration test with the yamls files. Therefore is the test changelog located at: /src/test/resources/keycloak-changelog.yaml and the corresponding files in /src/test/resources/changesets/. You can than execute these migrations against the test keycloak instance started by the startLocalKeycloak gradle task running on http://localhost:18080. So you can be sure that all your actions serialize correctly. If you are already dead sure we can skip this part :)

JanUnld commented 3 years ago

Happy to hear that 🙂! I fixed the typos in scope.md and also added/adjusted the integration tests for the client and client scope mappers. Unfortunately I'm not able to verify my integration tests at this very moment. Got a weird...

java.lang.NoSuchMethodError: java.nio.file.Files.readString

...exception when running the TestChangesets.kt test file 🤷. Probably some sdk related thing, anyways I definitely ran all of the related unit tests, so the action impls for sure work 😁! I also double checked the ActionFactory definitions and am also pretty sure that the mappings are there.

klg71 commented 3 years ago

I compile the code with openjdk version 11: https://jdk.java.net/archive/. I will checkout your pr and try it locally :)

JanUnld commented 3 years ago

Thanks for trying out the integration tests on your end 🙂! I've updated the changesets accordingly.

klg71 commented 3 years ago

Ok last thing todo is to merge the current master into your pull request as i am not able to do that :) there should be "command line instructions" at the bottom of this pull request

JanUnld commented 3 years ago

Done ✔️

klg71 commented 3 years ago

Merged I'll inform you as soon as I released it :)

klg71 commented 3 years ago

Released with https://github.com/mayope/keycloakmigration/releases/tag/0.2.22

JanUnld commented 3 years ago

Thank you! 🙂