Closed shreddedbacon closed 5 months ago
Since this is an "all the time" type operation, not a one time migration, it would be more aligned with the
sync_client_secrets
thanconfigure_blah_blah
.That would mean a slight rename and function definition moved to utilities section and invocation moved after all migrations.
I'm happy to move it to the utilities section, along with
configure_admin_email
configure_smtp_settings
configure_realm_settings
as these also run every time to ensure that the admin email, smtp and realm settings are up to date by configuring them from what is defined in the provided varaibles or config maps.as for sync vs configure, i don't really think it matters. if there are enough strongly held opinions that it should be changed to sync, then we will should also update the other 3 function names to be sync.
For invocation after all other migrations take place, we would also need to move the other functions I mentioned too then. They were added to the top so that they're applied sooner rather than later, due to the slow nature of this script. I really hope that we can make movement on #3499 or #3624 to reduce how many migrations need to run so that this delay in execution is less of a problem in the future
There's two "issues:" The sync functions not using the naming "standards" and not living in the "correct" place is only a organizational and self-documentation concern. It's not a deal breaker. Having the functions live in the migrations section is confusing IMO, and doesn't match our sections comments For those reasons, every migration must be designed to run __only once__ per keycloak install.
, so moving them is a DX concern. Moving them all in the file is easy and solves all the concerns. I don't care if they all start with sync_
it's just another DX self-documenting nod. If none of this part happens now, I'll just open a PR to do it later, since I also didn't realize that there were other sync functions besides the client secrets ones (I thought those other ones happened in another file for some reason).
The other one is where in the invocation it runs. IMO it's a bug that the sync functions where placed in the middle of the migrations. Considering the non-idempotent bugs we've had in the past the migrations were cleaned up and the sync parts where moved separately for a good reason. The migrations should "know" exactly what the state was from prior migrations. They should for sure all get moved after the migrations.
There's two "issues:" The sync functions not using the naming "standards" and not living in the "correct" place is only a organizational and self-documentation concern. It's not a deal breaker. Having the functions live in the migrations section is confusing IMO, and doesn't match our sections comments
For those reasons, every migration must be designed to run __only once__ per keycloak install.
, so moving them is a DX concern. Moving them all in the file is easy and solves all the concerns. I don't care if they all start withsync_
it's just another DX self-documenting nod. If none of this part happens now, I'll just open a PR to do it later, since I also didn't realize that there were other sync functions besides the client secrets ones (I thought those other ones happened in another file for some reason).
They're moved to utilities now.
The other one is where in the invocation it runs. IMO it's a bug that the sync functions where placed in the middle of the migrations. Considering the non-idempotent bugs we've had in the past the migrations were cleaned up and the sync parts where moved separately for a good reason. The migrations should "know" exactly what the state was from prior migrations. They should for sure all get moved after the migrations.
Yes, I agree with the whole invocation thing to a degree. But I absolutely believe that we need to get #3499 or #3624 in place so that we can properly reset though, there are far too many migrations taking place currently and it is confusing and a horrible experience to try and ensure that future migrations work correctly with all the other migrations that are already there.
Since I didn't realize there were existing sync funcs in the "wrong" place it's fair to defer that for later. It'll have to get done at some point in the refactor anyway.
No reason to keep configure_lagoon_redirect_uris
there though? It can run after all migrations.
I've moved that to the end. We can pick up discussion of migration/startup procedure once we've made decisions on the realm import process, but not in this PR.
waiting on #3624 and any further updates to Keycloak
General Checklist
Database Migrations
Just adds support to be able to change keycloak redirect uris via an environment variable (and eventually via helmchart) rather than having to define them in keycloak UI manually.