Add step to run Django check in CI

[dhruvkb]

Fixes

Fixes #631 by @dhruvkb

Description

This PR adds step to run Django check in CI.

Testing Instructions

View the CI logs to see that Django checks have been run successfully.

Checklist

• My pull request has a descriptive title (not a vague title like Update index.md).
• My pull request targets the default branch of the repository (main) or a parent feature branch.
• My commit messages follow best practices.
• My code follows the established code style of the repository.
• I added or updated tests for the changes I made (if applicable).
• I added or updated documentation (if applicable).
• I tried running the project locally and verified that there are no visible errors.

Developer Certificate of Origin

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

[AetherUnbound]

Looks great!

[dhruvkb]

@AetherUnbound would it be more preferable to club this check with the check for uncommitted migrations instead of with the API tests, considering that it's another Django command and not a proper test suite?

[AetherUnbound]

@AetherUnbound would it be more preferable to club this check with the check for uncommitted migrations instead of with the API tests, considering that it's another Django command and not a proper test suite?

That makes sense to me! Also, sorry for so much back and forth 😅 I wonder if we could further simplify this:

git diff
diff --git a/justfile b/justfile
index 6e4343e9..f94e6877 100644
--- a/justfile
+++ b/justfile
@@ -31,6 +31,7 @@ DOCKER_FILE := "-f " + (
     if IS_PROD == "true" {"ingestion_server/docker-compose.yml"}
     else {"docker-compose.yml"}
 )
+EXEC_DEFAULTS := if IS_CI == "" { "" } else { "-T" }
 
 # Build all (or specified) services
 build *args:
@@ -179,11 +180,11 @@ _api-install:
     exit 0
 
 # Run API tests inside Docker
-@api-test docker_args=(if IS_CI != "" { "-T" } else { "" }) tests="": _api-up
+@api-test docker_args=EXEC_DEFAULTS tests="": _api-up
     docker-compose exec {{ docker_args }} web ./test/run_test.sh {{ tests }}
 
 # Run Django check command inside Docker
-@api-check docker_args=(if IS_CI != "" { "-T" } else { "" }): _api-up
+@api-check docker_args=EXEC_DEFAULTS: _api-up
     just dj "{{ docker_args }}" check
 
 # Run API tests locally
@@ -195,7 +196,7 @@ dj-local +args:
     cd api && pipenv run python manage.py {{ args }}
 
 # Run Django administrative commands in the docker container
-@dj docker_args=(if IS_CI != "" { "-T" } else { "" }) +args="": _api-up
+@dj docker_args=EXEC_DEFAULTS +args="": _api-up
     docker-compose exec {{ docker_args }} web python manage.py {{ args }}
 
 # Make a test cURL request to the API
@@ -212,7 +213,7 @@ ipython:
 ##########
 
 # Compile Sphinx documentation into HTML output
-sphinx-make args=(if IS_CI != "" { "-T" } else { "" }) service="web": up wait-for-es wait-for-ing wait-for-web
+sphinx-make args=EXEC_DEFAULTS service="web": up wait-for-es wait-for-ing wait-for-web
     docker-compose exec {{ args }} {{ service }} sphinx-build -M html docs/ build/
 
 # Serve Sphinx documentation via a live-reload server

Example:

$ just --dry-run api-test
docker-compose -f docker-compose.yml up -d 
just _loop '"$(just es-health localhost:9200)" != "200"' "Waiting for Elasticsearch to be healthy..."
just _loop '"$(just ing-health localhost:8001)" != "200"' "Waiting for the ingestion-server to be healthy..."
just _loop '"$(just web-health)" != "200"' "Waiting for the API to be healthy..."
exit 0
docker-compose exec  web ./test/run_test.sh 

$ CI="true" just --dry-run api-test
docker-compose -f docker-compose.yml up -d 
just _loop '"$(just es-health localhost:9200)" != "200"' "Waiting for Elasticsearch to be healthy..."
just _loop '"$(just ing-health localhost:8001)" != "200"' "Waiting for the ingestion-server to be healthy..."
just _loop '"$(just web-health)" != "200"' "Waiting for the API to be healthy..."
exit 0
docker-compose exec -T web ./test/run_test.sh 

[sarayourfriend]

@AetherUnbound would it be more preferable to club this check with the check for uncommitted migrations instead of with the API tests, considering that it's another Django command and not a proper test suite?

I was actually going to mention something along these lines... it'd be nice to actually just have it as a separate job altogether, just like the uncommitted migrations. That way we get clear and individual failures in the CI output on the PR summary view.

[dhruvkb]

Having each check be a separate job is great, but what bugs me is that it has to download all images and orchestrate the entire infra up just to run a single command. There's gotta be a faster way to do this. I'm still going to make it a separate job, but trying to speed up CI is something to think about.

[sarayourfriend]

Having each check be a separate job is great, but what bugs me is that it has to download all images and orchestrate the entire infra up just to run a single command. There's gotta be a faster way to do this. I'm still going to make it a separate job, but trying to speed up CI is something to think about.

makemigrations and the django check we can run locally outside of docker. It takes a long time to pipenv install though, so it feels slow either way.

pipenv install should be cached between runs though, which the docker build never is. Theoretically that would be faster long run.

[dhruvkb]

Hmm, makes sense. I wonder if we could cache the entire pipenv virtualenv based on PIpfile.lock so that it's instantly restored. Will test in a separate PR.

[AetherUnbound]

I love these changes - lots more reusability!

[sarayourfriend]

LGTM! Really appreciative of the clean up in the justfile 🎉