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

Automated testing for applying migrations in a named schema #146

Merged
merged 3 commits into from
May 27, 2024

Conversation

laceysanderson
Copy link
Contributor

Issue #145

This PR builds on and is dependant on PR #143. It alters the dockerfile to allow you to name the schema you want chado installed in and creates some automated workflows that do this. This way we can confirm that there is nothing in the migration that is specific to the public schema!

Testing

This really just needs a code review and to confirm the automated testing passes. If you want to be extra thorough or are curious, then you can build the image with a set schema name as follows:

docker build --tag gmod/chado:local \
  --file docker/Dockerfile \
  --build-arg SCHEMA_NAME="teacup" ./
docker run -it --rm --volume=$(pwd):/Chado gmod/chado:local

This will install chado into a schema named teacup. You can use psql -U postgres -h localhost -d chado with the password chadotest to look at the schema. Specifically, \dt public.* should only have a flyway table and \dt teacup.* should list out the whole chado schema + a flyway migrations table.

@laceysanderson laceysanderson self-assigned this May 8, 2024
@laceysanderson laceysanderson linked an issue May 8, 2024 that may be closed by this pull request
@spficklin
Copy link
Contributor

Code changes to the Dockerfile look good and makes sense to me. I ran the test and the docker built just fine and the tests performed as described. I also applied the migrations and that worked as well.

One question though I'm getting this message when I fire up the Docker image:

37 SQL migrations were detected but not run because they did not follow the filename convention.

Do you know the cause of that? Whatever is causing that doesn't seem to interfere with the migration but it may create confusion when folks see it.

@spficklin
Copy link
Contributor

I'm approving as it works as expected. My question above is just a question.

@spficklin
Copy link
Contributor

An update. @laceysanderson and I spoke by Slack and she reminded me that the warning about the 37 SQL migrations that don't follow the migration naming scheme are left over from when we migrated Chado to use Flyway. This can probably be fixed if we dig into it. This warning has been present for quite a while and not related to this PR.

@laceysanderson
Copy link
Contributor Author

Great, thanks @spficklin! Are you ok with me merging this one as well @scottcain? It simply extends the last one to allow Chado to be installed into a separate schema in the docker and creates a workflow that tests this with schema installed in a schema named teacup 🍵 🤪 . Not Tripal specific in any way :-)

@laceysanderson laceysanderson merged commit 5858a73 into 1.4 May 27, 2024
11 checks passed
@laceysanderson laceysanderson deleted the 145-testing-flyway-with-tripal branch May 27, 2024 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using + testing Flyway with Tripal
2 participants