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

feat: Make alembic version table configurable #77

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

gdiepen
Copy link

@gdiepen gdiepen commented Feb 27, 2025

If the environment variable OIDC_ALEMBIC_VERSION_TABLE is defined, use this as the table that alembic uses to store the alembic versions in. If not defined, use the default alembic_version.

Fixes: #76

Allows to use both mlflow-oidc-auth and mlflow in the same (postgres) database, each having its own alembic version table.

@gdiepen gdiepen changed the title Make alembic version table configurable feat: Make alembic version table configurable Feb 27, 2025
@gdiepen gdiepen force-pushed the feature/make_alembic_version_table_configurable branch from 7497d18 to b13f2fd Compare February 27, 2025 12:11
@gdiepen
Copy link
Author

gdiepen commented Feb 27, 2025

I am running this in a local development branch and with this modification I am now running both mlflow and the plugin inside the same postgres database

@kharkevich
Copy link
Contributor

Hi @gdiepen,
thanks for your PR, could you please update it to use mlflow_oidc_auth/config.py for configuration of this feature
do not forget to update tests as well please

@gdiepen
Copy link
Author

gdiepen commented Feb 28, 2025

Will check what needs to be changed to use mlflow_oidc_auth/config.py there

Also will try to see how to write the test for this

@gdiepen
Copy link
Author

gdiepen commented Feb 28, 2025

So the first part is done, I have moved everything to the config and use that.

Second part I am having some problems with, not familiar with python unittest (a bit more experience with pytest).

Getting stuck on how to ensure I can initialize an empty database and see if the table created is based on the os environment.....

@gdiepen
Copy link
Author

gdiepen commented Feb 28, 2025

@kharkevich If you understand what is going wrong here, that would be very useful

If I use :memory: for the create_engine function in line 49, like this
image

the tables list is always empty.

However, if I change it to engine = create_engine("sqlite:///foo.db"), then it does work.

Second question would be how to mock environment variable in this framework, such that I can create a second test where I let it be a custom alembic version table

@gdiepen
Copy link
Author

gdiepen commented Mar 3, 2025

@kharkevich Having the migration run inside of a temporary database would be the nicest unit test (but still stuck on trying to get the alembic unit test to work with :memory: database....) I could just do a unit test of only checking the value in the config object, not sure which test you would like more. If you want to have the full in-memory test, do you have any idea on how this works?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants