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.

Sign up for GitHub

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

Improve native execution #702

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from
Draft

Improve native execution #702

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
88e69ba
Minor improvements to assist with native execution and db setup loading
tiagojsag Dec 17, 2021
225d12f
WIP - further sanitize Makefile and env vars
tiagojsag Dec 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/api-e2e-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ jobs:
- name: Checkout envs
run: docker-compose -f docker-compose-test-e2e.yml -f docker-compose-test-e2e.ci.yml config
- name: Run CI tests via make task
run: make run-test-e2e-ci
run: make test-e2e-backend
test-api-unit:
runs-on: ubuntu-18.04
timeout-minutes: 10
Expand Down
33 changes: 15 additions & 18 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,25 @@
.PHONY: start

# Read values as needed from .env
#
# Optionally pass 'environment=<something>' to load env vars from '.env.something' instead
# Useful to separate i.e. dev from testing databases
#
# If using the same variables in recipes that need to use a dotenv file other
# than .env, remember to check that no values from .env are being used
# inadvertently.
ENVFILE := $(if $(environment), .env-test-e2e, .env)
ENVFILE := $(if $(environment),.env.$(environment),.env)
ifneq (,$(wildcard $(ENVFILE)))
include $(ENVFILE)
DOCKER_COMPOSE_FILE := --env-file $(ENVFILE)
export
endif

CIENV := $(if $(filter $(environment), ci), -f docker-compose-test-e2e.ci.yml , -f docker-compose-test-e2e.local.yml)
API_DB_INSTANCE := $(if $(environment), test-e2e-postgresql-api, postgresql-api)
GEO_DB_INSTANCE := $(if $(environment), test-e2e-postgresql-geo-api, postgresql-geo-api)
REDIS_INSTANCE := $(if $(environment), test-e2e-redis, redis)
API_DB_INSTANCE := $(if $(filter $(IS_DOCKER_E2E_TESTS_RUNTIME), true), test-e2e-postgresql-api, postgresql-api)
GEO_DB_INSTANCE := $(if $(filter $(IS_DOCKER_E2E_TESTS_RUNTIME), true), test-e2e-postgresql-geo-api, postgresql-geo-api)
REDIS_INSTANCE := $(if $(filter $(IS_DOCKER_E2E_TESTS_RUNTIME), true), test-e2e-redis, redis)
Comment on lines +18 to +20
Copy link
Member

Choose a reason for hiding this comment

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

I like the way you added the ability to manage arbitrary environments and then adapted the previous hardcoded special treatment of two environments (ci and non-ci) here, but I am not clear (no wonder, it's make-land and it's late Friday afternoon) how this new bool var is supposed to be working: I don't have it defined locally, so IIUC the else branch of these conditional assignments should be used, but then e2e tests still work fine, so I am losing the plot.

Irrespective of my puzzlement, I would suggest to add a comment (in the Makefile itself, maybe, for this all to be self-contained? or in the main README.md to go along all/most of the other env vars we may use in .env files) about how to use this var and when/where to set it.


DOCKER_COMPOSE_FILE := $(if $(environment), -f docker-compose-test-e2e.yml $(CIENV), -f docker-compose.yml )
DOCKER_COMPOSE_FILE := $(DOCKER_COMPOSE_FILE) $(if $(filter $(IS_DOCKER_E2E_TESTS_RUNTIME), true), -f docker-compose-test-e2e.yml, -f docker-compose.yml )
DOCKER_CLEAN_VOLUMES := $(if $(environment), , \
docker volume rm -f marxan-cloud_marxan-cloud-postgresql-api-data && \
docker volume rm -f marxan-cloud_marxan-cloud-postgresql-geo-data && \
Expand All @@ -29,7 +33,6 @@ NC :=\033[0m # No Color
test-commands:
@echo $(ENVFILE)
@echo $(DOCKER_COMPOSE_FILE)
@echo $(CIENV)
@echo $(API_POSTGRES_DB)
@echo $(GEO_POSTGRES_USER)

Expand Down Expand Up @@ -103,7 +106,7 @@ seed-geoapi-init-data:
sed -e "s/\$$feature_id/$$featureid/g" api/apps/api/test/fixtures/features/$${table_name}.sql | docker-compose $(DOCKER_COMPOSE_FILE) exec -T $(GEO_DB_INSTANCE) psql -U "${GEO_POSTGRES_USER}"; \
done;

# need notebook service to execute a expecific notebook. this requires a full geodb
# need notebook service to execute a specific notebook. this requires a full geodb
generate-geo-test-data: extract-geo-test-data
docker-compose --project-name ${COMPOSE_PROJECT_NAME} -f ./data/docker-compose.yml exec marxan-science-notebooks papermill --progress-bar --log-output work/notebooks/Lab/convert_csv_sql.ipynb /dev/null
mv -f -u -Z data/data/processed/test-wdpa-data.sql api/apps/api/test/fixtures/test-wdpa-data.sql
Expand All @@ -124,8 +127,7 @@ test-start-services: clean-slate
docker-compose $(DOCKER_COMPOSE_FILE) exec -T api ./apps/api/entrypoint.sh run-migrations-for-e2e-tests && \
docker-compose $(DOCKER_COMPOSE_FILE) exec -T geoprocessing ./apps/geoprocessing/entrypoint.sh run-migrations-for-e2e-tests

seed-dbs-e2e: test-start-services
$(MAKE) seed-api-with-test-data
seed-dbs-e2e: test-start-services seed-api-with-test-data
Copy link
Member

Choose a reason for hiding this comment

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

I think this happens to work, but is not fully correct, because seed-api-with-test-data depends on test-start-services, so if anyone decides to run (which we aren't, but maybe we should? it could save some time in GH actions) e2e tests with make -j2 test-e2e-backend (or any -j N with N>1) then the seed task may be attempted while services are still being started.


test-e2e-api: seed-dbs-e2e
docker-compose $(DOCKER_COMPOSE_FILE) exec -T api ./apps/api/entrypoint.sh test-e2e
Expand All @@ -134,13 +136,7 @@ test-e2e-geoprocessing: seed-dbs-e2e
docker-compose $(DOCKER_COMPOSE_FILE) exec -T geoprocessing ./apps/geoprocessing/entrypoint.sh test-e2e

test-e2e-backend: test-e2e-api test-e2e-geoprocessing
$(MAKE) test-clean-slate

run-test-e2e-local:
$(MAKE) --keep-going test-e2e-backend environment=local

run-test-e2e-ci:
$(MAKE) --keep-going test-e2e-backend environment=ci
$(MAKE) test-clean-slate $(ARGS)

setup-test-unit-backend:
# build API and geoprocessing containers
Expand All @@ -157,7 +153,7 @@ test-unit-geo:
test-unit-backend: setup-test-unit-backend test-unit-api test-unit-geo

run-test-unit:
$(MAKE) --keep-going test-unit-backend
$(MAKE) --keep-going test-unit-backend $(ARGS)

dump-geodb-data:
docker-compose exec -T postgresql-geo-api pg_dump -T migrations -a -U "${GEO_POSTGRES_USER}" -F t ${GEO_POSTGRES_DB} | gzip > data/data/processed/db_dumps/geo_db-$$(date +%Y-%m-%d).tar.gz
Expand Down Expand Up @@ -236,4 +232,5 @@ native-seed-geoapi-init-data:
done;

native-seed-api-with-test-data: native-db-migrate native-seed-api-init-data | native-seed-geoapi-init-data
kgajowy marked this conversation as resolved.
Show resolved Hide resolved
@echo "seeding db with testing project and scenarios"
psql -U "${API_POSTGRES_USER}" -h "${API_POSTGRES_HOST}" ${API_POSTGRES_DB} < api/apps/api/test/fixtures/test-data.sql
2 changes: 0 additions & 2 deletions api/apps/api/config/default.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
"concurrency": 50
},
"postgresApi": {
"url": null,
Copy link
Member

Choose a reason for hiding this comment

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

I am fine with your suggestion.

IIRC I copypasta'd this strategy from earlier projects as I thought it would be a good way to on one hand let devs configure each aspect of the connection through a separate envvar (in case they want/need to define or override only some values while leaving the rest to env defaults or other defaults), while allowing to configure staging/prod environments with one single variable.

But of course it's part of a set-once-and-forget-about-it flow, so mapping 6 vars instead of 1 in a k8s manifest is not the end of the world either 😄 .

"port": 5432,
"host": null,
"username": null,
Expand All @@ -27,7 +26,6 @@
"logging": "error"
},
"postgresGeoApi": {
"url": null,
"port": 5432,
"host": null,
"username": null,
Expand Down
8 changes: 8 additions & 0 deletions api/apps/api/src/utils/config.utils.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,14 @@ process.env.NETWORK_CORS_ORIGINS = extraCorsOrigin;
import { AppConfig } from './config.utils';

describe('AppConfig', () => {
beforeAll(() => {
if (process.env.NODE_CONFIG_DIR !== 'apps/api/config') {
throw Error(
`Running the test suite with NODE_CONFIG_DIR=${process.env.NODE_CONFIG_DIR}, which may cause this test to fail. Please use NODE_CONFIG_DIR=apps/api/config.`,
);
}
Comment on lines +12 to +16
Copy link
Contributor

Choose a reason for hiding this comment

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

due to how AppConfig is implemented, even just importing it (which is pretty common in an indirect way, due to a lot of usage everywhere) makes it load the config from envs. Could be a good idea to move it to global setup for unit tests.

In the long run, I would suggest to replace the mechanism with https://docs.nestjs.com/techniques/configuration

Copy link
Member

Choose a reason for hiding this comment

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

agree, the Nest-y way would be handy as part of a future tech debt cleanup (and alignment with the Nest way wherever there's no need to go ad-hoc).

});

describe('getFromArrayAndParsedString', () => {
// Expected full result from `network.cors.origins`. If updating the default
// list in `config`, relevant tests should break and this list should be
Expand Down
2 changes: 0 additions & 2 deletions api/apps/geoprocessing/config/default.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
}
},
"postgresApi": {
"url": null,
"port": 5432,
"host": null,
"username": null,
Expand All @@ -20,7 +19,6 @@
"logging": "error"
},
"postgresGeoApi": {
"url": null,
"port": 5432,
"host": null,
"username": null,
Expand Down
85 changes: 0 additions & 85 deletions docker-compose-test-e2e.ci.yml
View file
Open in desktop

This file was deleted.

16 changes: 0 additions & 16 deletions docker-compose-test-e2e.local.yml
View file
Open in desktop

This file was deleted.

2 changes: 2 additions & 0 deletions docker-compose-test-e2e.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ services:
- ./api/apps:/opt/marxan-api/apps
- ./api/libs:/opt/marxan-api/libs
- marxan-cloud-backend-e2e-temp-storage:/tmp/storage
env_file: ${ENVFILE}
environment:
- NODE_PATH=src
- NODE_ENV=test
Expand Down Expand Up @@ -54,6 +55,7 @@ services:
- ./api/apps:/opt/marxan-geoprocessing/apps
- ./api/libs:/opt/marxan-geoprocessing/libs
- marxan-cloud-backend-e2e-temp-storage:/tmp/storage
env_file: ${ENVFILE}
environment:
- NODE_PATH=src
- NODE_ENV=test
Expand Down
17 changes: 8 additions & 9 deletions docker-compose.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,16 @@ services:
- ./api/apps:/opt/marxan-api/apps
- ./api/libs:/opt/marxan-api/libs
- marxan-cloud-backend-temp-storage:/tmp/storage
env_file: .env
env_file: ${ENVFILE}
environment:
- NODE_PATH=src
- NODE_ENV=development
- API_POSTGRES_HOST=marxan-postgresql-api
- API_POSTGRES_HOST=postgresql-api
- API_POSTGRES_USER
- API_POSTGRES_PASSWORD
- API_POSTGRES_PORT=5432
- GEO_POSTGRES_HOST=postgresql-geo-api
- API_POSTGRES_DB
- GEO_POSTGRES_HOST=marxan-postgresql-geo-api
- GEO_POSTGRES_USER
- GEO_POSTGRES_PASSWORD
- GEO_POSTGRES_PORT=5432
Expand All @@ -49,7 +49,7 @@ services:
volumes:
- ./app/src:/opt/marxan-app/src
- ./app/test:/opt/marxan-app/test
env_file: .env
env_file: ${ENVFILE}
environment:
- NODE_PATH=src
- NODE_ENV=development
Expand All @@ -70,16 +70,16 @@ services:
- ./api/apps:/opt/marxan-geoprocessing/apps
- ./api/libs:/opt/marxan-geoprocessing/libs
- marxan-cloud-backend-temp-storage:/tmp/storage
env_file: .env
env_file: ${ENVFILE}
environment:
- NODE_PATH=src
- NODE_ENV=development
- API_POSTGRES_HOST=marxan-postgresql-api
- API_POSTGRES_HOST=postgresql-api
- API_POSTGRES_USER
- API_POSTGRES_PASSWORD
- API_POSTGRES_PORT=5432
- API_POSTGRES_DB
- GEO_POSTGRES_HOST=marxan-postgresql-geo-api
- GEO_POSTGRES_HOST=postgresql-geo-api
- GEO_POSTGRES_USER
- GEO_POSTGRES_PASSWORD
- GEO_POSTGRES_PORT=5432
Expand Down Expand Up @@ -176,8 +176,7 @@ services:
image: apache/airflow:2.0.0
restart: on-failure
entrypoint: ['sh','./scripts/entrypoint.sh']
env_file:
- .env
env_file: ${ENVFILE}
environment:
- AIRFLOW__CORE__SQL_ALCHEMY_CONN=postgresql+psycopg2://airflow:airflow@postgresql-airflow/airflow
- AIRFLOW__CORE__EXECUTOR=LocalExecutor
Expand Down
1 change: 1 addition & 0 deletions env.default
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,3 +32,4 @@ APPLICATION_BASE_URL=http://localhost:3000
PASSWORD_RESET_TOKEN_PREFIX=/auth/reset-password?token=
PASSWORD_RESET_EXPIRATION=1800000
SIGNUP_CONFIRMATION_TOKEN_PREFIX=/auth/sign-up-confirmation?token=
IS_DOCKER_E2E_TESTS_RUNTIME=false