fix(day03): move cd outside if-block, add docker-compose down before deploy#17
fix(day03): move cd outside if-block, add docker-compose down before deploy#17Farhan-Mendhro wants to merge 1 commit intoLondheShubham153:mainfrom
Conversation
📝 WalkthroughWalkthroughA bash script for deploying a Django application was refactored: repository cloning became unconditional, helper functions were renamed and simplified with reduced error handling, and the deployment process now includes a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
day03/deploy_django_app.sh (1)
29-29: Quote the$USERvariable for safety.Unquoted variables can cause issues if they contain spaces or special characters (unlikely for
$USER, but good practice).Proposed fix
- sudo chown $USER /var/run/docker.sock + sudo chown "$USER" /var/run/docker.sock🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@day03/deploy_django_app.sh` at line 29, The chown invocation using sudo chown $USER /var/run/docker.sock should quote the variable to avoid word-splitting or globbing; update the command referenced (sudo chown $USER /var/run/docker.sock) to use a quoted variable (e.g., "$USER") so it becomes sudo chown "$USER" /var/run/docker.sock.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@day03/deploy_django_app.sh`:
- Around line 11-15: The code_clone function assumes any git clone failure means
the repo already exists and continues silently; change it to first check for the
target directory (e.g., test -d django-notes-app) and skip cloning if present,
otherwise run git clone and immediately check its exit status ($?) and on
failure print a descriptive error with the exit code and exit the script
non‑zero; apply the same pattern to the other clone usage noted around the
second clone call (lines 46-49) so both locations explicitly validate directory
existence first and handle git clone failures instead of continuing silently.
- Line 64: Fix the typo in the echo string on the failing line: change the
message passed to echo in deploy_django_app.sh from "System fault identitfied"
to "System fault identified" (update the echo statement to use the corrected
spelling).
- Around line 4-7: The heredoc opener "<<" task is being used as a comment and
triggers Shellcheck SC2188; replace it with a no-op heredoc by prefixing with a
colon (change "<< task" to ": << task") or simply convert the block to normal
line comments (prepend "#" to each line) so the heredoc has a consuming command
and the Shellcheck warning is resolved.
- Around line 18-22: The install_req function is missing an apt-get update
before installing packages; update the apt cache first and then install packages
with proper flags and ordering: inside install_req call sudo apt-get update &&
sudo apt-get install -y docker.io nginx docker-compose (or split update and
install into two commands), keeping the function name install_req and ensuring
the -y flag and package list are passed to apt-get install rather than appended
after the command.
---
Nitpick comments:
In `@day03/deploy_django_app.sh`:
- Line 29: The chown invocation using sudo chown $USER /var/run/docker.sock
should quote the variable to avoid word-splitting or globbing; update the
command referenced (sudo chown $USER /var/run/docker.sock) to use a quoted
variable (e.g., "$USER") so it becomes sudo chown "$USER" /var/run/docker.sock.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7afcc1f3-db2d-4075-a1c1-db1c0d8ce43b
📒 Files selected for processing (1)
day03/deploy_django_app.sh
| << task | ||
| deploy django app | ||
| and handle errors | ||
| task |
There was a problem hiding this comment.
Heredoc used without a command triggers Shellcheck warning SC2188.
The << task heredoc is used as a comment but has no command to consume it. Either prefix with : << (no-op) or use standard # comments.
Proposed fix
-<< task
-deploy django app
-and handle errors
-task
+# Task:
+# Deploy django app and handle errorsOr use a no-op heredoc:
-<< task
+: << 'task'
deploy django app
and handle errors
task📝 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.
| << task | |
| deploy django app | |
| and handle errors | |
| task | |
| # Task: | |
| # Deploy django app and handle errors |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 4-4: This redirection doesn't have a command. Move to its command (or use 'true' as no-op).
(SC2188)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@day03/deploy_django_app.sh` around lines 4 - 7, The heredoc opener "<<" task
is being used as a comment and triggers Shellcheck SC2188; replace it with a
no-op heredoc by prefixing with a colon (change "<< task" to ": << task") or
simply convert the block to normal line comments (prepend "#" to each line) so
the heredoc has a consuming command and the Shellcheck warning is resolved.
| code_clone(){ | ||
|
|
||
| echo "Cloning the Django app..." | ||
| git clone https://github.com/LondheShubham153/django-notes-app.git | ||
| } |
There was a problem hiding this comment.
Fragile error handling: clone failure assumed to mean directory exists.
git clone can fail for multiple reasons (network error, authentication, disk space), not just because the directory exists. The current logic silently continues on any failure, which could leave the script in an undefined state.
Consider checking explicitly:
Proposed fix
code_clone(){
-
echo "Cloning the Django app..."
- git clone https://github.com/LondheShubham153/django-notes-app.git
+ if [ -d "django-notes-app" ]; then
+ echo "Directory already exists, skipping clone..."
+ else
+ git clone https://github.com/LondheShubham153/django-notes-app.git || { echo "Git clone failed"; return 1; }
+ fi
}Then simplify the call site:
-if ! code_clone;
-then
- echo "directory already exists, skipping cloning...."
-fi
+if ! code_clone; then
+ echo "Failed to clone repository"
+ exit 1
+fiAlso applies to: 46-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@day03/deploy_django_app.sh` around lines 11 - 15, The code_clone function
assumes any git clone failure means the repo already exists and continues
silently; change it to first check for the target directory (e.g., test -d
django-notes-app) and skip cloning if present, otherwise run git clone and
immediately check its exit status ($?) and on failure print a descriptive error
with the exit code and exit the script non‑zero; apply the same pattern to the
other clone usage noted around the second clone call (lines 46-49) so both
locations explicitly validate directory existence first and handle git clone
failures instead of continuing silently.
| install_req() { | ||
|
|
||
| echo " Installing dependencies..." | ||
| sudo apt-get install docker.io nginx -y docker-compose | ||
| } |
There was a problem hiding this comment.
Missing apt-get update before package installation.
The function omits apt-get update, which could cause installation failures or install stale package versions on systems without a fresh apt cache. The codebase pattern in batch-10-scripts/install_packages.sh pairs update with install.
Proposed fix
install_req() {
-
echo " Installing dependencies..."
- sudo apt-get install docker.io nginx -y docker-compose
+ sudo apt-get update && sudo apt-get install -y docker.io nginx docker-compose
}📝 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.
| install_req() { | |
| echo " Installing dependencies..." | |
| sudo apt-get install docker.io nginx -y docker-compose | |
| } | |
| install_req() { | |
| echo " Installing dependencies..." | |
| sudo apt-get update && sudo apt-get install -y docker.io nginx docker-compose | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@day03/deploy_django_app.sh` around lines 18 - 22, The install_req function is
missing an apt-get update before installing packages; update the apt cache first
and then install packages with proper flags and ordering: inside install_req
call sudo apt-get update && sudo apt-get install -y docker.io nginx
docker-compose (or split update and install into two commands), keeping the
function name install_req and ensuring the -y flag and package list are passed
to apt-get install rather than appended after the command.
|
|
||
| if ! req_restarts; | ||
| then | ||
| echo "System fault identitfied" |
There was a problem hiding this comment.
Typo: "identitfied" should be "identified".
Proposed fix
- echo "System fault identitfied"
+ echo "System fault identified"📝 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.
| echo "System fault identitfied" | |
| echo "System fault identified" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@day03/deploy_django_app.sh` at line 64, Fix the typo in the echo string on
the failing line: change the message passed to echo in deploy_django_app.sh from
"System fault identitfied" to "System fault identified" (update the echo
statement to use the corrected spelling).
1 participant
What was broken
cdcommand only ran if clone failed, so the script stayed in the wrong folder on first run.What I fixed
cdoutside the if/else so it always enters the app folder.docker-compose downat the start of deploy() to clean up old containers and networks.Tested
Ran locally in WSL — both a fresh clone and a re-run worked correctly.
Summary by CodeRabbit
Release Notes