Skip to content

Conversation

@clay53
Copy link
Contributor

@clay53 clay53 commented Mar 4, 2025

Most important aspect of this is that it tests django-run and asgi-run are working. Not intended to replace development or production environment.

@clay53 clay53 requested a review from joyliu-q March 4, 2025 18:44
Copy link
Contributor

@joyliu-q joyliu-q left a comment

Choose a reason for hiding this comment

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

Overall, a very good starting point! Added @ryantanen and @dkwncho, as well as @danielxue for posterity.

When adding new configs, it's good to specify a reason for that. Especially when changing the docker-compose.yaml, it's good to add in-line code comments on why this is needed. Same thing for top of nginx-proddev.conf.

Also not the biggest fan of the name proddev: if you can think of a better one would be cool

@@ -0,0 +1,17 @@
from officehoursqueue.settings.production import *
Copy link
Contributor

Choose a reason for hiding this comment

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

Whats the reason for adding this new proddev.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a basis for the django settings for proddev. It is based off of the production settings, but with some changes so it still actually works in the development environment. For example, unless ALLOWED_HOSTS is changed, it will return a 400 error on every request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't believe it's possible to override Django settings with just environment variables without changing settings files.

@joyliu-q joyliu-q requested review from dkwncho and ryantanen March 4, 2025 19:49
@joyliu-q joyliu-q marked this pull request as ready for review March 5, 2025 20:26
Copy link
Contributor

@joyliu-q joyliu-q left a comment

Choose a reason for hiding this comment

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

Theoretically lgtm. Would feel good about merging it as long as you tested it.

@clay53 clay53 requested a review from joyliu-q March 5, 2025 20:28
clay53 added 6 commits March 21, 2025 22:21
* Migrate dev environment to uv

* Migrate docker setup to uv

* Move instead of copy uv-installed binaries

* Move .dockerignore .venv from uv section to misc

* Revert default dependency manager back to pipenv for docker images

* Install uv in a simpler way in docker

* Merge project dependency copy in backend docker

* Simplify pipenv or uv install in docker
Copy link
Contributor

@joyliu-q joyliu-q left a comment

Choose a reason for hiding this comment

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

Shaping up quite nicely! A small comment (feel free to ping me once u addressed) and should be good to go!

Copy link
Contributor

@joyliu-q joyliu-q left a comment

Choose a reason for hiding this comment

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

lgtm

@joyliu-q joyliu-q merged commit 611cf6b into master Mar 28, 2025
5 checks passed
@joyliu-q joyliu-q deleted the proddev branch March 28, 2025 22:29
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