Repository navigation
Conversation
f91f88f to
4a0b496
Compare
1f7cee9 to
f4192bc
Compare
…on-managed client with attachTo. Signed-off-by: Lukas Hanusovsky <lhanusov@redhat.com>
f4192bc to
7fc6f92
Compare
There was a problem hiding this comment.
There's a few issues with this approach; in ManagedClient constructor you are fetching the representation resulting in an extra call to Keycloak everytime we create a client, which is not needed as we already have that in createdRepresentation.
In the ManagedClient close method you are re-creating the client, which is rather against what is intended in the close method.
I'm not really convinced we should support cleanup for non-managed clients, but if we really want to do that we need a different approach. I'd probably say the updateWithCleanup should use dirty for managed clients, while for non-managed clients it would add a cleanup task that resets it to the createdRepresentation.
A last thing is I don't see why this is hardcoding the special admin-client client as a client that can't be updated.
|
I question whether or not we really should support cleanup with non-managed clients, users, etc. It feels like an anti-pattern (honestly I don't quite like the attachTo in the first place), and it also adds a lot of complexity/overhead. I would argue that a better/simpler approach would be to simply not support any cleanup with non-managed resources. For those probably very few cases where it is needed that can be handled with |
There was a problem hiding this comment.
Thinking about this more I'm not in favour of adding this; the framework really shouldn't support cleanup for non-managed (it's kinda in the name right?) clients. Rather, looking at how attachTo is used, in general it is abused, and there's better ways to write tests than attaching to existing clients.
|
this issue is not fixing the problem rather creating a shady behavior, instead we plan to remove the attachTo functionality from ManagedClient. |
Closes #48263