Improve native execution #702

tiagojsag · 2021-12-17T08:50:17Z

This PR should help a bit with running things fully natively. I also added a few more env vars (which I suspect would be needed in the future anyway) and some basic docs here and there. Functionally this should have little to no impact in anyone's current setup... maybe... probably... sometimes

vercel · 2021-12-17T08:50:20Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

marxan – ./app

🔍 Inspect: https://vercel.com/vizzuality1/marxan/G2xb958XqfcqBcjk57qidoKfF6yx
✅ Preview: https://marxan-git-feature-make-native-execution-easier-vizzuality1.vercel.app

marxan-storybook – ./app

🔍 Inspect: https://vercel.com/vizzuality1/marxan-storybook/Fed3KFZKyCTCC9G3ZTKiXPYrjj2Q
✅ Preview: https://marxan-storybook-git-feature-make-native-exe-f16ba6-vizzuality1.vercel.app

hotzevzl · 2021-12-17T14:46:13Z

Makefile

-_API_POSTGRES_USER := $(if $(filter $(environment), ci),${API_POSTGRES_USER},$(shell grep -e API_POSTGRES_USER ${ENVFILE} | sed 's/^.*=//'))
-_API_POSTGRES_DB := $(if $(filter $(environment), ci),${API_POSTGRES_DB},$(shell grep -e API_POSTGRES_DB ${ENVFILE} | sed 's/^.*=//'))
-_GEO_POSTGRES_USER := $(if $(filter $(environment), ci),${GEO_POSTGRES_USER},$(shell grep -e GEO_POSTGRES_USER ${ENVFILE} | sed 's/^.*=//'))
-_GEO_POSTGRES_DB := $(if $(filter $(environment), ci),${GEO_POSTGRES_DB},$(shell grep -e GEO_POSTGRES_DB ${ENVFILE} | sed 's/^.*=//'))


it's been a looong time (and I barely remember details from last week, if I didn't document them), but IIRC the reason for using a prefix on these make vars was to avoid clashes with env vars (from which they may be set, or not, depending on the environment), so I think this change will break CI or running things in Docker locally.

shame on me for not clearly documenting/commenting why the dangling underscore, of course

also - _something may instinctively trigger "unused var/placeholder var" instinct, so the choice may not be the best. I have a vague recollection of not being able to decide on a better naming schema to differentiate the two layers of variables and ending up using a quick change to get the task done.

CI is happy with this change, local envs not sure, but I can assist anyone that has problems. Overall, I feel like the project has a bit of https://xkcd.com/927/ problem with env vars. If this breaks something, I suspect the underlying cause will be different vars being used for the same purpose, in which case we should (and I am happy to help with) fix that, not hide it.

hotzevzl · 2021-12-17T14:49:36Z

Makefile

+ENVFILE := $(if $(environment), .env-$(environment), .env)
+ifneq (,$(wildcard $(ENVFILE)))
+    include $(ENVFILE)
+    export
+endif


I like this, but I think that piggybacking on the existing environment make params may break CI and/or running some things (tests) in Docker locally. It should be ok to refactor the existing use of this param to make it possible to use it both to read an environment-specific .env file and to differentiate the test runs between CI and local Docker Compose setups.

we briefly discussed this during a call, but for reference:
Right now, the 'environment' argument is used more like a feature flag of sorts, and not so much as a way to define different environments. This change pushes to actually restore it to being basically an identifier between different sets of configurations. If we need to further tune specific behavior, we can add env vars to said environments, to control behavior. However, I don't think that will be necessary, at least for now

hotzevzl · 2021-12-17T14:51:38Z

api/apps/api/config/custom-environment-variables.json

+    "host": "API_POSTGRES_HOST",
+    "username": "API_POSTGRES_USER",
+    "password": "API_POSTGRES_PASSWORD",
+    "database": "API_POSTGRES_DB",
+    "port": "API_POSTGRES_PORT",


API_POSTGRES_URL already includes host, user, password, db and port - can we rather compose this single envvar when running natively so that we avoid having duplication of pg connection configs?

upon closer inspection on reverting my changes, I actually noticed that API_POSTGRES_URL is a sort of internal variable, as devs are always expected to provide the individual host/user/password/db values. So, in DRY spirit, I'm removing the URL var instead

hotzevzl · 2021-12-17T14:54:02Z

api/apps/api/src/migrations/api/1608149578000-EnablePostgis.ts

+  async up(queryRunner: QueryRunner): Promise<any> {
+    if (await PostgreSQLUtils.version13Plus()) {
+      await queryRunner.query(`
+CREATE EXTENSION IF NOT EXISTS postgis;


This will enable PostGIS in the apidb, but we don't need it there.

This should not be needed when running the services natively either... are you sure this is needed?

There are some columns of type geometry, so this is needed.

Which ones? pretty sure api.geo shouldn't have any (otherwise it shouldn't even boot 🤔 )

extent was dropped , as well as tr_GetBbox I believe 🤔

ok, but that means they still exist at some point. Perhaps there is an improvement opportunity here, where this can be refactored further down the road.

good point. I have created https://vizzuality.atlassian.net/browse/MARXAN-1165 to take care of this.

Makefile

kgajowy · 2021-12-20T07:03:49Z

README.md

+     Docker PostgreSQL service should listen on the local machine
+   * API PostgreSQL configuration variables:
+     * `API_POSTGRES_HOST` (string, required): host of the database server to be
+       used for the PostgreSQL connection (API)
+     * `API_POSTGRES_PORT` (number, required): port of the database server to be
+       used for the PostgreSQL connection (API)
+     * `API_POSTGRES_USER` (string, required): username to be used for the
+       PostgreSQL connection (API)
+     * `API_POSTGRES_PASSWORD` (string, required): password to be used for the
+       PostgreSQL connection (API)
+     * `API_POSTGRES_DB` (string, required): name of the database to be used for
+       the PostgreSQL connection (API)


[minor] it is always hard to maintain the .md files - for this very particular case, there is a common approach to provide .env.example tho for starters which document (and provides some defaults) the variables.

kgajowy · 2021-12-20T07:06:53Z

api/apps/api/src/utils/config.utils.spec.ts

+    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.`,
+      );
+    }


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

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).

hotzevzl

@tiagojsag thanks for this (and your previous work on making native execution possible).

Besides the comments inline in the code review, there's something that we should address - I don't see how any of the changes in this PR may affect that, so it may be due to changes in the earlier PR (hence no strict request for changes here), but basically what is now happening is that when I run e2e tests locally through the docker compose setup (rather than simply running them within a running docker container for normal API use), the "normal" apidb and geodb volumes get wiped (via clean-slate, I guess).

I can try to figure out which change introduced this regression, but that will have to be for next week.

hotzevzl · 2022-01-07T16:58:20Z

Makefile

@@ -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


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.

hotzevzl · 2022-01-07T17:04:15Z

api/apps/api/config/default.json

@@ -17,7 +17,6 @@
    "concurrency": 50
  },
  "postgresApi": {
-    "url": null,


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 😄 .

hotzevzl · 2022-01-07T18:00:34Z

Makefile

+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)


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.

hotzevzl · 2022-01-07T18:10:42Z

api/apps/api/src/utils/config.utils.spec.ts

+    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.`,
+      );
+    }


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).

tiagojsag force-pushed the feature/make-native-execution-easier branch from 22038b7 to c5be7ae Compare December 17, 2021 10:03

vercel bot deployed to Preview – marxan December 17, 2021 10:03 View deployment

vercel bot deployed to Preview – marxan-storybook December 17, 2021 10:03 View deployment

tiagojsag force-pushed the feature/make-native-execution-easier branch from c5be7ae to 10f8b6d Compare December 17, 2021 10:13

vercel bot deployed to Preview – marxan-storybook December 17, 2021 10:13 View deployment

vercel bot deployed to Preview – marxan December 17, 2021 10:13 View deployment

tiagojsag force-pushed the feature/make-native-execution-easier branch from 10f8b6d to ee32ab5 Compare December 17, 2021 14:34

vercel bot deployed to Preview – marxan December 17, 2021 14:34 View deployment

vercel bot deployed to Preview – marxan-storybook December 17, 2021 14:34 View deployment

tiagojsag changed the title ~~[DO NOT MERGE] Improve native execution~~ Improve native execution Dec 17, 2021

tiagojsag requested review from hotzevzl and kgajowy December 17, 2021 14:34

hotzevzl reviewed Dec 17, 2021

View reviewed changes

kgajowy reviewed Dec 20, 2021

View reviewed changes

vercel bot deployed to Preview – marxan-storybook December 20, 2021 11:34 View deployment

vercel bot deployed to Preview – marxan December 20, 2021 11:34 View deployment

tiagojsag added the WIP Work in progress label Dec 20, 2021

tiagojsag marked this pull request as draft December 20, 2021 11:36

tiagojsag mentioned this pull request Dec 20, 2021

Improve native execution - MVP #707

Merged

tiagojsag added the API Everything related the api label Dec 22, 2021

tiagojsag added 2 commits January 4, 2022 10:46

Minor improvements to assist with native execution and db setup loading

88e69ba

WIP - further sanitize Makefile and env vars

225d12f

tiagojsag force-pushed the feature/make-native-execution-easier branch from 8c58a53 to 225d12f Compare January 4, 2022 09:49

vercel bot deployed to Preview – marxan January 4, 2022 09:49 View deployment

vercel bot deployed to Preview – marxan-storybook January 4, 2022 09:49 View deployment

hotzevzl reviewed Jan 7, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve native execution #702

Improve native execution #702

tiagojsag commented Dec 17, 2021 •

edited

vercel bot commented Dec 17, 2021 •

edited

hotzevzl Dec 17, 2021

hotzevzl Dec 17, 2021

tiagojsag Dec 17, 2021

hotzevzl Dec 17, 2021

tiagojsag Dec 17, 2021

hotzevzl Dec 17, 2021

tiagojsag Dec 20, 2021

hotzevzl Dec 17, 2021

tiagojsag Dec 17, 2021

kgajowy Dec 20, 2021

tiagojsag Dec 20, 2021

kgajowy Dec 20, 2021

tiagojsag Dec 20, 2021

hotzevzl Jan 3, 2022

kgajowy Dec 20, 2021

kgajowy Dec 20, 2021

hotzevzl Jan 7, 2022

hotzevzl left a comment

hotzevzl Jan 7, 2022

hotzevzl Jan 7, 2022

hotzevzl Jan 7, 2022

hotzevzl Jan 7, 2022

Improve native execution #702

Are you sure you want to change the base?

Improve native execution #702

Conversation

tiagojsag commented Dec 17, 2021 • edited

vercel bot commented Dec 17, 2021 • edited

marxan – ./app

marxan-storybook – ./app

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hotzevzl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiagojsag commented Dec 17, 2021 •

edited

vercel bot commented Dec 17, 2021 •

edited