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

feat: Update Heroku deployment for Celery and Redis integration #2600

Closed
wants to merge 2 commits into from
Closed

feat: Update Heroku deployment for Celery and Redis integration #2600

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
17bfae0
Update Heroku deployment for Celery and Redis integration
areebahmeddd Nov 15, 2024
0bfd424
Added coderrabbitai critical suggestions
areebahmeddd Nov 15, 2024
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
2 changes: 2 additions & 0 deletions Procfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,4 @@
web: gunicorn config.wsgi:application
release: python manage.py collectstatic --noinput && python manage.py migrate
worker: celery -A config.celery_app worker --loglevel=info
beat: celery -A config.celery_app beat --loglevel=info
74 changes: 47 additions & 27 deletions app.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,27 +1,47 @@
{
"environments": {
"test": {
"scripts": {
"test": "python manage.py test"
},
"buildpacks": [{
"url": "https://github.com/cyberdelia/heroku-geo-buildpack.git"
},
{
"url": "heroku/python"
}
],
"env": {
"DJANGO_SETTINGS_MODULE": {
"value": "config.settings.test"
},
"LD_LIBRARY_PATH": {
"value": "/app/lib"
},
"BUILD_WITH_GEO_LIBRARIES": {
"value": "1"
}
}
}
}
}
{
"environments": {
"test": {
"scripts": {
"test": "python manage.py test"
},
"buildpacks": [
{
"url": "https://github.com/cyberdelia/heroku-geo-buildpack.git"
},
{
"url": "heroku/python"
}
],
Comment on lines +7 to +14
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Oh, I see you've forgotten something rather important in production...

The production environment is missing the buildpack configurations that are present in the test environment. If your application uses geographical features (which it seems to, given the geo buildpack in test), this could lead to some... interesting behavior in production.

Add the buildpacks to the production environment:

 "production": {
     "scripts": {
         "postdeploy": "python manage.py migrate && python manage.py load_redis_index"
     },
+    "buildpacks": [
+        {
+            "url": "https://github.com/cyberdelia/heroku-geo-buildpack.git"
+        },
+        {
+            "url": "heroku/python"
+        }
+    ],
     "env": {

Also applies to: 27-30

"env": {
"DJANGO_SETTINGS_MODULE": {
"value": "config.settings.test"
},
"LD_LIBRARY_PATH": {
"value": "/app/lib"
},
"BUILD_WITH_GEO_LIBRARIES": {
"value": "1"
}
}
},
"production": {
"scripts": {
"postdeploy": "python manage.py migrate && python manage.py load_redis_index"
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Migrations in postdeploy? How... brave.

Running migrations directly in the postdeploy script without any safety checks could lead to deployment failures or data inconsistencies. Consider adding some basic error handling.

Here's a slightly safer approach:

-"postdeploy": "python manage.py migrate && python manage.py load_redis_index"
+"postdeploy": "python manage.py migrate --check && python manage.py migrate && python manage.py load_redis_index || echo 'Post-deploy failed' && exit 1"

Committable suggestion skipped: line range outside the PR's diff.

},
"env": {
"DJANGO_SETTINGS_MODULE": {
"value": "config.settings.production"
},
"DATABASE_URL": {
"value": "${DATABASE_URL}"
},
"REDIS_URL": {
"value": "${REDIS_URL}"
},
"CELERY_BROKER_URL": {
"value": "${CELERY_BROKER_URL}"
}
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

The environment variables section could use some attention...

A few concerns about the environment configuration:

  1. Missing LD_LIBRARY_PATH and BUILD_WITH_GEO_LIBRARIES that are present in test
  2. No validation for required environment variables
  3. Direct interpolation of environment variables might fail if they're not set

Add the missing variables and consider adding required flag:

 "env": {
     "DJANGO_SETTINGS_MODULE": {
         "value": "config.settings.production"
     },
     "DATABASE_URL": {
-        "value": "${DATABASE_URL}"
+        "value": "${DATABASE_URL}",
+        "required": true
     },
     "REDIS_URL": {
-        "value": "${REDIS_URL}"
+        "value": "${REDIS_URL}",
+        "required": true
     },
     "CELERY_BROKER_URL": {
-        "value": "${CELERY_BROKER_URL}"
+        "value": "${CELERY_BROKER_URL}",
+        "required": true
     },
+    "LD_LIBRARY_PATH": {
+        "value": "/app/lib"
+    },
+    "BUILD_WITH_GEO_LIBRARIES": {
+        "value": "1"
+    }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
"env": {
"DJANGO_SETTINGS_MODULE": {
"value": "config.settings.production"
},
"DATABASE_URL": {
"value": "${DATABASE_URL}"
},
"REDIS_URL": {
"value": "${REDIS_URL}"
},
"CELERY_BROKER_URL": {
"value": "${CELERY_BROKER_URL}"
}
}
"env": {
"DJANGO_SETTINGS_MODULE": {
"value": "config.settings.production"
},
"DATABASE_URL": {
"value": "${DATABASE_URL}",
"required": true
},
"REDIS_URL": {
"value": "${REDIS_URL}",
"required": true
},
"CELERY_BROKER_URL": {
"value": "${CELERY_BROKER_URL}",
"required": true
},
"LD_LIBRARY_PATH": {
"value": "/app/lib"
},
"BUILD_WITH_GEO_LIBRARIES": {
"value": "1"
}
}

}
}
}
42 changes: 42 additions & 0 deletions docs/local-setup/configuration.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,3 +207,45 @@ If you get an :code:`ImproperlyConfigured` error regarding the Spatialite librar
::

$ sudo apt install libsqlite3-mod-spatialite

Heroku Deployment
=================

To deploy the backend along with Celery on Heroku, follow these steps:

1. Ensure you have the Heroku CLI installed and are logged in.

2. Create a new Heroku app:

.. code-block:: bash

heroku create your-app-name

3. Add the necessary add-ons for PostgreSQL and Redis:

.. code-block:: bash

heroku addons:create heroku-postgresql:hobby-dev
heroku addons:create heroku-redis:hobby-dev

4. Set the environment variables:

.. code-block:: bash

heroku config:set DJANGO_SETTINGS_MODULE=config.settings.production
heroku config:set DATABASE_URL=your-database-url
heroku config:set REDIS_URL=your-redis-url
heroku config:set CELERY_BROKER_URL=your-redis-url

Comment on lines +231 to +239
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Some rather important environment variables seem to be missing

The current set of environment variables might leave our application feeling a bit exposed. Consider adding:

heroku config:set DJANGO_SECRET_KEY=<your-secret-key>
heroku config:set DJANGO_ALLOWED_HOSTS=.herokuapp.com
heroku config:set DJANGO_DEBUG=False
heroku config:set CELERY_WORKER_CONCURRENCY=2  # adjust based on dyno size

5. Deploy the app:

.. code-block:: bash

git push heroku main

Comment on lines +240 to +245
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

The deployment section could use some verification steps

After pushing to Heroku, it would be nice to verify the deployment:

# Verify the deployment
heroku ps  # Check if dynos are running
heroku logs --tail  # Monitor the logs
heroku ps:scale worker=1  # Scale up Celery worker

6. Run the migrations and load the Redis index:

.. code-block:: bash

heroku run python manage.py migrate
heroku run python manage.py load_redis_index
Comment on lines +246 to +251
Copy link

@coderabbitai coderabbitai bot Nov 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue

Running migrations directly in production? How adventurous!

Consider adding these safer practices:

  • Check migration status first: heroku run python manage.py showmigrations
  • Backup database before migrations: heroku pg:backups:capture
  • Verify Redis index after loading: heroku run python manage.py shell -c "from django.core.cache import cache; print(cache.keys('*'))"

Copy link
Member

Choose a reason for hiding this comment

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

Migrations should be run in production. Please update your Learning.

Copy link

Choose a reason for hiding this comment

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

@vigneshhari, thank you for the clarification. I've updated my understanding accordingly.

✏️ Learnings added 
Learnt from: vigneshhari
PR: ohcnetwork/care#2600
File: docs/local-setup/configuration.rst:246-251
Timestamp: 2024-11-16T17:24:49.641Z
Learning: In this project, running migrations directly in production is acceptable and intended practice.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.