-
Notifications
You must be signed in to change notification settings - Fork 0
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 mariadb #6
Add mariadb #6
Conversation
Looks good. One thing to note is that a-la-carte feature selection is now required, and before it was optional. Specifically the bottom of both the server-setup.sh and app-setup.sh contained code that would execute all functions in order if no specific function was listed as a command line argument: wildfly/scripts/server-setup.sh Lines 261 to 274 in 029939b
This is currently used by ATLis-flavored apps (calendar, workmap, presenter): This is no longer possible/desirable and should probably be removed at some point as configuring both Oracle and MariaDB is most commonly not what users want and also not supported. As this lib has been generalized to work with more apps I noticed this was becoming a problem and is not new. For example the ability to optionally configure email meant that it can't be enabled default so it wasn't listed in the function list and is then required to be manually invoked by smoothness apps: https://github.com/JeffersonLab/srm/blob/main/Dockerfile#L22-L27 How to organize a bash library to make it easy to use and easy to customize is an on-going effort and can be seen most recently in the related jeffersonlab/keycloak project: JeffersonLab/keycloak#4. The alternative approach there is to allow importing a single lib.sh file and then calling the functions a-la-carte. This has the advantage of avoiding invoking the same script repeatedly with different argument and instead import the lib.sh once and then call the desired functions. |
I don't think the way I wrote the updates necessitates a la carte feature selection for databases in the sense that you have to call the setup scripts with explicit mariadb related commands. I added the I do however only wait for one database to start. That's easy enough to change if you want to make the entrypoint script wait for both. |
I think what you have is good and can be merged as is. I was mostly just making a note mentioning that early versions of this project assumed all users of it did the same thing and could rely on bottom piece of code that auto executed all functions. However, as this project evolves I don't think that's a good strategy anymore. It's probably less error prone user-API-wise to instead ask users to explicitly indicate what functions they want and have missing parameters error instead of ignore - calling a function explicitly and then forgetting to set a env/param should fail fast instead of silently log skipped message that user may miss. Currently it is tricky/impossible to check if required parameters are present as many of them are optional and if we say all functions are optional then no parameter checking can be done at the top of the file anyways. It make more sense to do all parameter checks per function. This also avoids the odd state we're in where even if you're intention is to call a single function, the env that you pass in must include other function envs to pass the top of file required env check. For example app-setup.sh REQUIRES all keycloak setup envs at the moment, even if you're setting up a DB: Lines 9 to 13 in 029939b
Here is an example in the keycloak lib.sh demonstrating per-function checks instead: So drop the auto-call all functions thing, but probably should keep the call specific function passed in via arg though. Something to add to Keycloak lib.sh I guess. Requires the file to be executable though. |
I think that's a reasonable take on the situation. It is certainly easy to miss an environment variable and takes a little sleuthing to identify that problem. I'll go ahead and merge this in. |
I've added support for a MariaDB database similar to Oracle.
I did not require that only one database be configured, but left a comment that the general expectation is for a single database to configured. In theory, I do not see why only one database would have to be used. But I don't see their to be a need to prohibit users from doing so. On quick reflection, the current entry-point script only waits for one database. Perhaps I should update that to wait for both if they both exist?
I also made a few minor edits/fixes in the README.md.