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

avoid overwriting evn vars with Make vars before recursive make exec #190

Merged
merged 18 commits into from
May 17, 2021

Conversation

hotzevzl
Copy link
Member

This PR proposes a change to avoid the issue we were experiencing with some environment vars (user and db names for api and geo) being blank in GH Actions CI only.

I eventually realised that the run-test-e2e-ci recipe would first trigger the expansion (not sure this is the correct terminology) of vars such as

API_POSTGRES_USER := $(if $(filter $(environment), ci),${API_POSTGRES_USER},$(shell grep -e API_POSTGRES_USER ${ENVFILE} | sed 's/^.*=//'))

and since the target itself is called without environment being set, this would lead to API_POSTGRES_USER from the environment to be set to an empty value (because we would try to read it from .env, but there's no such file in our GH Action filesystem), and by the time we would invoke make recursively while setting environment=ci in the run-test-e2e-ci recipe itself, the envvar API_POSTGRES_USER we got from the environment would have been overwritten with a blank value.

To avoid touching too many things, I have only used pivot variables within the Makefile, so the line above now is

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

and I have similarly updated the Make vars to use the _-prefixed form.

The bit missing from this PR is identifying why planning-units.e2e-spec.ts is causing Jest to hang indefinitely while waiting for all handles/etc to be closed, but I suggest to do this in a separate PR, as I have temporarily skipped this whole suite and we can at least have e2e tests working fine in CI, and then fix the leaking handles.

@vercel
Copy link

vercel bot commented May 17, 2021

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/5F4tCidUBVyCSJijU7KpeqYpJLpS
✅ Preview: https://marxan-git-chore-infratest-ci-workflow-vizzuality1.vercel.app

marxan-storybook – ./app

🔍 Inspect: https://vercel.com/vizzuality1/marxan-storybook/HFZRAH6AgnjgqXc6XUwBjPcTknHY
✅ Preview: https://marxan-storybook-git-chore-infratest-ci-workflow-vizzuality1.vercel.app

This is because each runs ok, while when running all via a single jest
command, Jest ends up hanging because of leftover open handles.

To be reviewed and fixed properly, of course.
@hotzevzl hotzevzl force-pushed the chore/infra/test-ci-workflow branch from 7d3c43a to 0be9569 Compare May 17, 2021 13:33
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