-
Notifications
You must be signed in to change notification settings - Fork 148
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
refactor: add migration to keycloak startup to set client redirect uris #3640
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is an "all the time" type operation, not a one time migration, it would be more aligned with the sync_client_secrets
than configure_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
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. |
6b8baf2
to
5872ea6
Compare
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 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. |
They're moved to utilities now.
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 |
5872ea6
to
d82850d
Compare
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 |
d82850d
to
80f74f9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feature can't be taken full advantage of until other keycloak work also makes it in, but this piece is ready to go 👍
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.