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

Local development via Docker #261

Merged
merged 20 commits into from
Dec 10, 2024
Merged

Local development via Docker #261

merged 20 commits into from
Dec 10, 2024

Conversation

jamesgorrie
Copy link
Contributor

@jamesgorrie jamesgorrie commented Dec 4, 2024

Description

Creating local, Docker based development workflows for:

  • Running unit / integration test suits
  • Running the application with a prod-like set of data in the local Postgres store

This doesn't optimise Dockerfile and docker-compose files to avoid breaking things already used in prod. This is already a large PR and I want to focus on the dev workflow.

The README should describe the workflow
https://github.com/climatepolicyradar/navigator-admin-backend/pull/261/files?plain=1#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R25-R50

I've found this PR easier to review in split mode https://github.com/climatepolicyradar/navigator-admin-backend/pull/261/files?diff=split&w=0

Main features that these workflows support

  • not overriding data between tests and having the app ready for development
  • not needing to swap values in the .env dependant on context, but rather set them on the context of where they are needed

Things this could do, but feels out of scope or optimisations for later

  • separates unit and integration tests (the DB isn't needed for unit)
  • ???

Proposed version

  • Skip auto-tagging
  • Patch
  • Minor version
  • Major version

Type of change

Please select the option(s) below that are most relevant:

  • DevX

How Has This Been Tested?

Part of https://linear.app/climate-policy-radar/issue/PDCT-1719/discuss-integration-tests-destroying-local-db-in-admin-service-backend

Fixes https://linear.app/climate-policy-radar/issue/PDCT-1719/discuss-integration-tests-destroying-local-db-in-admin-service-backend

@jamesgorrie jamesgorrie requested a review from a team as a code owner December 4, 2024 09:13
@jamesgorrie jamesgorrie marked this pull request as draft December 4, 2024 09:17
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needed updating to be able to parse Dockerfiles with HEREDOC

@jamesgorrie jamesgorrie marked this pull request as ready for review December 9, 2024 14:27
Copy link

linear bot commented Dec 9, 2024

unit_test: build
docker compose run --rm navigator-admin-backend pytest -vvv tests/unit_tests
dev_rds_dump:
[ ! -f ./dumps/navigator.sql ] && aws --profile staging s3 cp s3://cpr-staging-rds/dumps/navigator.sql ./dumps/ || echo 0
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is currently a file auto generated every day manually. We will try to make this more automated in future.

@odrakes-cpr
Copy link
Contributor

odrakes-cpr commented Dec 9, 2024

All looks good so far, I will say when running locally - I am getting an error I believe due to the test_bulk_import.py file appearing in multiple places @jamesgorrie @annaCPR. But i just changed the name and then ran the tests ok

import file mismatch:
imported module 'test_bulk_import' has this __file__ attribute:
  /usr/src/tests/integration_tests/bulk_import/test_bulk_import.py
which is not the same as the test file we want to collect:
  /usr/src/tests/unit_tests/routers/bulk_import/test_bulk_import.py
HINT: remove __pycache__ / .pyc files and/or use a unique basename for your test file modules

@jamesgorrie
Copy link
Contributor Author

@odrakes-cpr

But i just changed the name and then ran the tests ok

Ah, interesting - I wonder if this is because we've tended not to run the whole suit as a whole, did you get this from running make test?

@odrakes-cpr
Copy link
Contributor

@odrakes-cpr

But i just changed the name and then ran the tests ok

Ah, interesting - I wonder if this is because we've tended not to run the whole suit as a whole, did you get this from running make test?

yes, it was from make test

@jamesgorrie
Copy link
Contributor Author

@odrakes-cpr - I've added some __init__ files that helps pytest work out it's cache a little better.

Nabbed from this comment.

@jamesgorrie
Copy link
Contributor Author

@odrakes-cpr - this seems to fix the env variables problem we were having.

I'm not a huge fan of the solution, and would like to dig into a better one, but I think it's a solution for now to help us get this in.

@jamesgorrie jamesgorrie enabled auto-merge (squash) December 10, 2024 16:40
@jamesgorrie jamesgorrie disabled auto-merge December 10, 2024 16:40
@jamesgorrie jamesgorrie merged commit 812ff4a into main Dec 10, 2024
10 checks passed
@jamesgorrie jamesgorrie deleted the docker-orchestration branch December 10, 2024 16:40
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