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

fix dev docker file and enable sql logging #2649

Merged
merged 1 commit into from
Dec 11, 2024
Merged

fix dev docker file and enable sql logging #2649

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 3 additions & 2 deletions config/settings/local.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,9 @@
# django-silk
# ------------------------------------------------------------------------------
# https://github.com/jazzband/django-silk#requirements
INSTALLED_APPS += ["silk"]
MIDDLEWARE += ["silk.middleware.SilkyMiddleware"]
if env("ENABLE_SILK", default=False):
INSTALLED_APPS += ["silk"]
MIDDLEWARE += ["silk.middleware.SilkyMiddleware"]
Comment on lines +22 to +24
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Would be lovely if we could add documentation for the ENABLE_SILK environment variable

The local setup documentation in docs/local-setup/configuration.rst seems to be missing any mention of this rather important development feature. Since developers might want to use Django Silk for profiling, it would be quite helpful to document this configuration option alongside other environment variables.

  • Add documentation about ENABLE_SILK environment variable in docs/local-setup/configuration.rst, perhaps under the development environment setup section
  • Include a brief explanation of what Silk does and when developers might want to enable it
🔗 Analysis chain

Documentation update might be nice.

Since this introduces a new environment variable that affects development workflow, it would be absolutely wonderful if we could document this change in the development setup guide.

Let's check if there's a relevant documentation file:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Look for development setup documentation
fd -t f -e md -e rst "setup|development|configuration" docs/

Length of output: 421

Script:

#!/bin/bash
# Check content of local setup configuration docs
cat docs/local-setup/configuration.rst

# Also check if there's any existing mention of silk in docs
rg -i "silk" docs/

Length of output: 5087

# https://github.com/jazzband/django-silk#profiling
SILKY_PYTHON_PROFILER = True

Expand Down
3 changes: 2 additions & 1 deletion docker/.local.env
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ DATABASE_URL=postgres://postgres:postgres@db:5432/care
REDIS_URL=redis://redis:6379
CELERY_BROKER_URL=redis://redis:6379/0

DJANGO_DEBUG=False
DJANGO_DEBUG=true
ATTACH_DEBUGGER=false

BUCKET_REGION=ap-south-1
BUCKET_KEY=key
Expand Down
4 changes: 2 additions & 2 deletions docker/dev.Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,9 @@ RUN ARCH=$(dpkg --print-architecture) && \
rm -rf typst.tar.xz typst-${TYPST_ARCH}

# use pipenv to manage virtualenv
RUN pip install pipenv==2024.4.0

RUN python -m venv /.venv
RUN --mount=type=cache,target=/root/.cache/pip pip install pipenv==2024.4.0

Comment on lines +35 to +36
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Looks like we found a lonely pip install without cache in prod.Dockerfile

The build cache optimization is only implemented in dev.Dockerfile but not in prod.Dockerfile. For consistency and build performance, the same cache mount should be added to the pip install command in prod.Dockerfile as well.

  • docker/prod.Dockerfile: Add --mount=type=cache,target=/root/.cache/pip to the pip install command
🔗 Analysis chain

Finally, someone remembered that build caches exist!

Nice optimization using --mount=type=cache. This should make our builds significantly faster, assuming everyone remembers to enable BuildKit.

Let's check if we're consistent with cache usage across our Dockerfiles:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check other Dockerfiles for potential cache optimization opportunities
fd Dockerfile | while read -r file; do
  echo "=== $file ==="
  rg "pip install" "$file"
done

Length of output: 392

COPY Pipfile Pipfile.lock $APP_HOME/
RUN --mount=type=cache,target=/root/.cache/pip pipenv install --system --categories "packages dev-packages docs"

Expand Down
6 changes: 3 additions & 3 deletions scripts/start-dev.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@ python manage.py collectstatic --noinput
python manage.py compilemessages -v 0

echo "starting server..."
if [[ "${DJANGO_DEBUG,,}" == "true" ]]; then
if [[ "${ATTACH_DEBUGGER}" == "true" ]]; then
echo "waiting for debugger..."
python -m debugpy --wait-for-client --listen 0.0.0.0:9876 manage.py runserver_plus 0.0.0.0:9000
python -m debugpy --wait-for-client --listen 0.0.0.0:9876 manage.py runserver_plus 0.0.0.0:9000 --print-sql
else
python manage.py runserver 0.0.0.0:9000
python manage.py runserver_plus 0.0.0.0:9000 --print-sql
fi
Loading