Skip to content
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

Merged
merged 3 commits into from
Mar 18, 2024
Merged

Add mariadb #6

merged 3 commits into from
Mar 18, 2024

Conversation

apcarp
Copy link
Contributor

@apcarp apcarp commented Mar 15, 2024

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.

@slominskir
Copy link
Member

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:

if [ ! -z "$2" ]
then
echo "------------------------"
echo "$2"
echo "------------------------"
$2
else
for i in "${!FUNCTIONS[@]}"; do
echo "------------------------"
echo "${FUNCTIONS[$i]}"
echo "------------------------"
${FUNCTIONS[$i]};
done
fi

This is currently used by ATLis-flavored apps (calendar, workmap, presenter):

https://github.com/JeffersonLab/calendar/blob/81c6ac707b3aa487475faef2495811e3c0ec1563/Dockerfile#L21

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.

@apcarp
Copy link
Contributor Author

apcarp commented Mar 18, 2024

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 configure_mariadb_client and configure_mariadb_driver commands to the function list in their respective scripts. However the first thing they do is check if the needed environment variables are defined. If they are not, then they skip any further work setting up the database. The equivalent oracle functions already did this. I have not tested this against an app that does not setup the mariadb database, so it's possible I missed something there.

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.

@slominskir
Copy link
Member

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:

VARIABLES=(KEYCLOAK_REALM
KEYCLOAK_RESOURCE
KEYCLOAK_SECRET
KEYCLOAK_SERVER_URL
KEYCLOAK_WAR

Here is an example in the keycloak lib.sh demonstrating per-function checks instead:

https://github.com/JeffersonLab/keycloak/blob/708604d8ea662bf643fc3fb169872d0868e62715/scripts/lib.sh#L44-L54

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.

@apcarp
Copy link
Contributor Author

apcarp commented Mar 18, 2024

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.

@apcarp apcarp merged commit 01268a0 into main Mar 18, 2024
1 check passed
@apcarp apcarp deleted the add-mariadb branch March 18, 2024 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants