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

Use NginX as web server #206

Merged
merged 1 commit into from
May 5, 2023
Merged

Conversation

sindre-nistad
Copy link
Contributor

@sindre-nistad sindre-nistad commented Mar 21, 2023

Why is this pull request needed?

It removes the need for a separate container, whose only job is to serve static files.

What does this pull request change?

It moves all the NginX configuration into web, so that we can use a multistage build for the web server.
It preserves hot-reloading when running locally

Issues related to this change:

@github-advanced-security
Copy link

You have successfully added a new CodeQL configuration .github/workflows/on-pull-request.yaml:analyze/language:javascript. As part of the setup process, we have scanned this repository and found 1 existing alert. Please check the repository Security tab to see all alerts.

@github-advanced-security
Copy link

You have successfully added a new CodeQL configuration .github/workflows/on-pull-request.yaml:analyze/language:python. As part of the setup process, we have scanned this repository and found no existing alerts. In the future, you will see all code scanning alerts on the repository Security tab.

@sindre-nistad sindre-nistad force-pushed the refactor/use-nginx-as-web-server branch from f1cde83 to 2d9fe85 Compare March 21, 2023 11:33
@sindre-nistad sindre-nistad requested a review from netr0m April 14, 2023 11:10
web/Dockerfile Show resolved Hide resolved
Copy link
Contributor

@netr0m netr0m left a comment

Choose a reason for hiding this comment

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

Nicely done! :shipit: Definitely an improvement for production deployments. Just a few remarks on trivial matters.

Copy link
Collaborator

@soofstad soofstad left a comment

Choose a reason for hiding this comment

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

Look great! :D
Might want to update radixconfig.yaml as well. Does not look like it will work deployed like this

@sindre-nistad
Copy link
Contributor Author

sindre-nistad commented Apr 20, 2023

Look great! :D
Might want to update radixconfig.yaml as well. Does not look like it will work deployed like this

Forgot to update it. Will do

@sindre-nistad
Copy link
Contributor Author

Look great! :D
Might want to update radixconfig.yaml as well. Does not look like it will work deployed like this

Forgot to update it. Will do

@soofstad Is there an easy way to test the new radix configuration?

@soofstad
Copy link
Collaborator

Look great! :D
Might want to update radixconfig.yaml as well. Does not look like it will work deployed like this

Forgot to update it. Will do

@soofstad Is there an easy way to test the new radix configuration?

Nope. Fingers crossed and deploy to test

@sindre-nistad
Copy link
Contributor Author

@soofstad , @netr0m : Any ideas as to why the api tests fail?
I tried executing docker-compose -f docker-compose.yml -f docker-compose.ci.yml run --rm api pytest --integration (same as in the GitHub Action) locally, and it seems to work.

@netr0m
Copy link
Contributor

netr0m commented Apr 20, 2023

@soofstad , @netr0m : Any ideas as to why the api tests fail? I tried executing docker-compose -f docker-compose.yml -f docker-compose.ci.yml run --rm api pytest --integration (same as in the GitHub Action) locally, and it seems to work.

Well, it seems the poetry install command fails - the workflow outputs:

[...]
Step 11/14 : RUN poetry install
[...]
  ChefBuildError

  Backend 'setuptools.build_meta:__legacy__' is not available.
[...]

Note: This error originates from the build backend, and is likely not a problem with poetry but with sentinels (1.0.0) not supporting PEP 517 builds. You can verify this by running 'pip wheel --use-pep517 "sentinels (==1.0.0)"'.

I found the following from a quick search - seems like a recent change (upstream, poetry or some other build dependency) has caused something to break?

I guess it might relate to us not being explicit about which poetry version we'd like to use? I.e., in the api/Dockerfile, we accept any version of poetry, which will break stuff at some point.

Also, did you do a fresh build of the Docker image prior to running the docker-compose / pytest locally?

@sindre-nistad sindre-nistad force-pushed the refactor/use-nginx-as-web-server branch from 5a5f19a to 3c6ebd9 Compare May 5, 2023 11:25
@sindre-nistad sindre-nistad merged commit ff3cb1e into main May 5, 2023
@sindre-nistad sindre-nistad deleted the refactor/use-nginx-as-web-server branch May 5, 2023 11:32
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.

3 participants