GMOD / Chado

the GMOD database schema
http://gmod.org/wiki/Chado
Artistic License 2.0
38 stars 25 forks source link

GitHub workflows to help with review #143

Closed laceysanderson closed 4 months ago

laceysanderson commented 5 months ago

Issue #142

Description

This PR does the following things:

Testing

  1. Confirm the automated testing runs and passes.
  2. Clone this directory locally and test building docker images, running containers and using flyway (more instructions below).
  3. Try pulling the existing images on the container registry and playing with them. See this in the new README here.

When you build the image using the new dockerfile, it actually uses the current code in your Chado clone. In order to allow you to modify the migrations while running the container, you will mount your current directory inside the container using the following approach. This way you can edit the files locally and they will be automatically updated within the container and available to flyway.

git clone https://github.com/GMOD/Chado chado-142-dockerUpgrades
cd chado-142-dockerUpgrades
git checkout 142-github-workflows-to-help-with-review
docker build --tag gmod/chado:local --file docker/Dockerfile \
    --build-arg PGSQL_VERSION=<version> ./
docker run -it -rm --volume=$(pwd):/Chado gmod/chado:local

Replace the <version> with the version of postgresql you want installed. This can be one of 12, 13, 14, 15, or 16; the default is 16 if the build-arg is not specified.

Note: You should not go into the docker directory but rather stay in the root of the clone.

You can test FlyWay migrations by running these flyway commands:

spficklin commented 5 months ago

@laceysanderson this worked just fine for me. I love having this testing for a PR via Docker.

It's been so long since I've used Flyway that I'm having to reorient myself to it and I have one question about the migrations in this test. Can you you remind me where these are coming from? I'm assuming that if this were a real PR with suggested changes to Chado that the migrations would come from within the PR. When this gets merged will the migrations in this test get merged too?

laceysanderson commented 5 months ago

Thanks for the review @spficklin! All the migrations in this PR are ones already existing in the 1.4 branch (other merged PRs :-) ). That said, this one does make a small change to the last migration as it was actually not able to be applied ๐Ÿ™ˆ

Each PR will add a new migration to the current list and then they will all be applied in order. We'll keep this up with the list of migrations growing until we release a formal 1.4 at which point all the existing migrations will be applied, the new schema dumped and then we can start fresh with the numbering :-)

spficklin commented 4 months ago

Understood. Thanks for that reminder. Yes, this worked great for me. I approve the PR.

laceysanderson commented 4 months ago

Thank you both! ๐ŸŽ‰ โค๏ธ