python upgrade #82
python upgrade #82
Conversation
WalkthroughUpdated Docker Compose image tags and streamlined Python invocations; provisioning scripts switched pip/pip3 usage, adjusted Python version references, and added frontend asset build steps (yarn/npm) and related housekeeping for assets and node modules. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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 |
hmalabehqrf
force-pushed
the
hmalabeh/python_upgrade
branch
from
bc9e97f to
715c73a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
provision-progs.sh (2)
14-14: Clarify the commented cache copy line.Line 14 is commented out (
#docker compose ...). Is this intentional, or should it be restored? If this line is no longer needed due to changes in how the container is provisioned, consider removing the comment entirely to reduce confusion.Would you like me to remove this commented line, or should it be restored?
21-27: Verify the asset compilation sequence is optimal.The expanded asset pipeline now includes
npm rebuild node-sass,chown,rm -rf node_modules/,npm install, andnpm run devin sequence. This looks reasonable for a clean build, but verify that:
- The order is correct (e.g., chown before npm install)
- These steps align with the mktg provisioning script (which includes
yarnandnpm run dev)- Node-sass rebuild is necessary given the fresh
npm installConsider consolidating related commands to reduce the number of Docker Compose exec calls.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docker-compose-mktg.yml(1 hunks)docker-compose-progs.yml(1 hunks)provision-mktg.sh(1 hunks)provision-progs.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docker-compose-mktg.yml
🧰 Additional context used
🪛 Shellcheck (0.11.0)
provision-mktg.sh
[warning] 12-12: Quote this to prevent word splitting.
(SC2046)
[warning] 15-15: Quote this to prevent word splitting.
(SC2046)
provision-progs.sh
[warning] 11-11: Quote this to prevent word splitting.
(SC2046)
[warning] 15-15: Quote this to prevent word splitting.
(SC2046)
[warning] 18-18: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (3)
provision-mktg.sh (1)
11-15: Verify Python version alignment with the container image.This script removes the explicit
python3.8reference and relies on a genericpythoncommand. Unlikeprovision-progs.sh(line 11), this script does not set upupdate-alternativesto explicitly mappythonto a specific version (e.g.,python3.11). Ensure the mktg container image has the correct Python default or add the same setup as inprovision-progs.sh.Can you confirm:
- What Python version is configured as the default (or via
/usr/bin/pythonsymlink) in themktgservice image?- Should the mktg provisioning script also include the
update-alternativessetup for consistency withprovision-progs.sh?docker-compose-progs.yml (2)
16-16: Verify the updated image tag is available and contains the expected Python version.The image tag has been updated from
20250618-155233-mto20251001-113817-m. Ensure this new image exists in the registry and confirm it ships with the correct Python version (presumably 3.11).
20-20: Verify the container's default Python interpreter matches expectations.The command now uses
pythoninstead ofpython3.8. Confirm that the updated image (tag20251001-113817-m) haspythonpointing to the intended version. If the container does not have an explicit symlink, it may fail at runtime.
| echo "** Marketing: Install requirements **" | ||
| docker compose `echo ${DOCKER_COMPOSE_FILES}` exec mktg bash -c 'pip install -r requirements.txt' |
There was a problem hiding this comment.
Fix unquoted backticks to prevent word splitting.
The Shellcheck warning SC2046 indicates that backticks should be quoted to prevent word splitting if DOCKER_COMPOSE_FILES contains spaces or special characters.
Apply this diff:
-echo "** Marketing: Install requirements **"
-docker compose `echo ${DOCKER_COMPOSE_FILES}` exec mktg bash -c 'pip install -r requirements.txt'
+echo "** Marketing: Install requirements **"
+docker compose $(echo ${DOCKER_COMPOSE_FILES}) exec mktg bash -c 'pip install -r requirements.txt'Alternatively, if DOCKER_COMPOSE_FILES is a single value or already properly quoted, consider using it directly:
+docker compose ${DOCKER_COMPOSE_FILES} exec mktg bash -c 'pip install -r requirements.txt'📝 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 "** Marketing: Install requirements **" | |
| docker compose `echo ${DOCKER_COMPOSE_FILES}` exec mktg bash -c 'pip install -r requirements.txt' | |
| echo "** Marketing: Install requirements **" | |
| docker compose $(echo ${DOCKER_COMPOSE_FILES}) exec mktg bash -c 'pip install -r requirements.txt' |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 12-12: Quote this to prevent word splitting.
(SC2046)
🤖 Prompt for AI Agents
In provision-mktg.sh around lines 11-12, the command uses unquoted backticks
which can cause word splitting when DOCKER_COMPOSE_FILES contains spaces;
replace the backticked expansion with a quoted command substitution and quote
the variable, e.g. change `docker compose \`echo ${DOCKER_COMPOSE_FILES}\` exec
...` to use docker compose "$(echo "${DOCKER_COMPOSE_FILES}")" exec mktg bash -c
'pip install -r requirements.txt' or, if DOCKER_COMPOSE_FILES is a single
properly quoted value, use docker compose ${DOCKER_COMPOSE_FILES} exec ...
ensuring the variable is quoted as needed.
|
|
||
| echo "** Marketing: Migrating databases **" | ||
| docker compose `echo ${DOCKER_COMPOSE_FILES}` exec mktg bash -c 'python3.8 manage.py migrate --settings=marketingsite.envs.dev' | ||
| docker compose `echo ${DOCKER_COMPOSE_FILES}` exec mktg bash -c 'python manage.py migrate --settings=marketingsite.envs.dev' |
There was a problem hiding this comment.
Fix unquoted backticks to prevent word splitting.
The Shellcheck warning SC2046 indicates that backticks should be quoted to prevent word splitting if DOCKER_COMPOSE_FILES contains spaces or special characters.
Apply this diff:
-docker compose `echo ${DOCKER_COMPOSE_FILES}` exec mktg bash -c 'python manage.py migrate --settings=marketingsite.envs.dev'
+docker compose $(echo ${DOCKER_COMPOSE_FILES}) exec mktg bash -c 'python manage.py migrate --settings=marketingsite.envs.dev'Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 15-15: Quote this to prevent word splitting.
(SC2046)
🤖 Prompt for AI Agents
In provision-mktg.sh around line 15, the backticks around echo
${DOCKER_COMPOSE_FILES} are unquoted which can cause word splitting; replace the
backticks with a quoted command substitution and quote the variable: use "$(echo
"${DOCKER_COMPOSE_FILES}")" (i.e. change the command to use docker compose
"$(echo "${DOCKER_COMPOSE_FILES}")" exec mktg bash -c 'python manage.py migrate
--settings=marketingsite.envs.dev') so the expanded value is treated as a
single, safely quoted argument.
|
|
||
| echo "** Progs: Setting correct python version **" | ||
| docker compose `echo ${DOCKER_COMPOSE_FILES}` exec progs bash -c 'update-alternatives --install /usr/bin/python python /usr/bin/python3.8 1' | ||
| docker compose `echo ${DOCKER_COMPOSE_FILES}` exec progs bash -c 'update-alternatives --install /usr/bin/python python /usr/bin/python3.11 1' |
There was a problem hiding this comment.
Fix unquoted backticks to prevent word splitting.
The Shellcheck warning SC2046 indicates that backticks should be quoted. Additionally, verify that the update-alternatives command is necessary in the provisioning script, or if it should be baked into the container image instead.
Apply this diff:
-docker compose `echo ${DOCKER_COMPOSE_FILES}` exec progs bash -c 'update-alternatives --install /usr/bin/python python /usr/bin/python3.11 1'
+docker compose $(echo ${DOCKER_COMPOSE_FILES}) exec progs bash -c 'update-alternatives --install /usr/bin/python python /usr/bin/python3.11 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.
| docker compose `echo ${DOCKER_COMPOSE_FILES}` exec progs bash -c 'update-alternatives --install /usr/bin/python python /usr/bin/python3.11 1' | |
| docker compose $(echo ${DOCKER_COMPOSE_FILES}) exec progs bash -c 'update-alternatives --install /usr/bin/python python /usr/bin/python3.11 1' |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 11-11: Quote this to prevent word splitting.
(SC2046)
🤖 Prompt for AI Agents
In provision-progs.sh around line 11, the use of unquoted backticks causes
shellword-splitting (SC2046); replace the backticks with a POSIX command
substitution and properly quote the variable (e.g., use "$(echo
"${DOCKER_COMPOSE_FILES}")" or simply "${DOCKER_COMPOSE_FILES}" if already
formatted) and wrap the inner bash -c string in double quotes so the
`update-alternatives --install /usr/bin/python python /usr/bin/python3.11 1`
command is passed intact; also verify whether running update-alternatives here
is required or should instead be performed in the container image build and, if
so, move that step into the Dockerfile.
| echo "** Progs: Copy cacheed files to code dir **" | ||
| #docker compose `echo ${DOCKER_COMPOSE_FILES}` exec progs bash -c 'cp -Rn /cache/* /app/.' | ||
| docker compose `echo ${DOCKER_COMPOSE_FILES}` exec progs bash -c 'pip3 install -r requirements.txt' | ||
| docker compose `echo ${DOCKER_COMPOSE_FILES}` exec progs bash -c 'pip install -r requirements.txt' |
There was a problem hiding this comment.
Fix unquoted backticks to prevent word splitting.
The Shellcheck warning SC2046 indicates that backticks should be quoted to prevent word splitting if DOCKER_COMPOSE_FILES contains spaces or special characters.
Apply this diff:
-docker compose `echo ${DOCKER_COMPOSE_FILES}` exec progs bash -c 'pip install -r requirements.txt'
+docker compose $(echo ${DOCKER_COMPOSE_FILES}) exec progs bash -c 'pip install -r requirements.txt'📝 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.
| docker compose `echo ${DOCKER_COMPOSE_FILES}` exec progs bash -c 'pip install -r requirements.txt' | |
| docker compose "$(echo ${DOCKER_COMPOSE_FILES})" exec progs bash -c 'pip install -r requirements.txt' |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 15-15: Quote this to prevent word splitting.
(SC2046)
🤖 Prompt for AI Agents
In provision-progs.sh around line 15, the command uses unquoted backticks which
can cause word splitting when DOCKER_COMPOSE_FILES contains spaces; replace the
backticks with a quoted command substitution and quote the variable, e.g. use
"$(echo "${DOCKER_COMPOSE_FILES}")" (or simply "${DOCKER_COMPOSE_FILES}" if
safe) so the expanded value is preserved as a single argument: update the line
to use quoted $(...) and quoted variable expansion.
|
|
||
| echo "** Progs: Migrating databases **" | ||
| docker compose `echo ${DOCKER_COMPOSE_FILES}` exec progs bash -c 'python3.8 manage.py migrate --settings=edraakprograms.dev' | ||
| docker compose `echo ${DOCKER_COMPOSE_FILES}` exec progs bash -c 'python manage.py migrate --settings=edraakprograms.dev' |
There was a problem hiding this comment.
Fix unquoted backticks to prevent word splitting.
The Shellcheck warning SC2046 indicates that backticks should be quoted to prevent word splitting if DOCKER_COMPOSE_FILES contains spaces or special characters.
Apply this diff:
-docker compose `echo ${DOCKER_COMPOSE_FILES}` exec progs bash -c 'python manage.py migrate --settings=edraakprograms.dev'
+docker compose $(echo ${DOCKER_COMPOSE_FILES}) exec progs bash -c 'python manage.py migrate --settings=edraakprograms.dev'📝 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.
| docker compose `echo ${DOCKER_COMPOSE_FILES}` exec progs bash -c 'python manage.py migrate --settings=edraakprograms.dev' | |
| docker compose "$(echo ${DOCKER_COMPOSE_FILES})" exec progs bash -c 'python manage.py migrate --settings=edraakprograms.dev' |
| docker compose `echo ${DOCKER_COMPOSE_FILES}` exec progs bash -c 'python manage.py migrate --settings=edraakprograms.dev' | |
| docker compose $(echo "${DOCKER_COMPOSE_FILES}") exec progs bash -c 'python manage.py migrate --settings=edraakprograms.dev' |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 18-18: Quote this to prevent word splitting.
(SC2046)
🤖 Prompt for AI Agents
In provision-progs.sh around line 18, the backtick command substitution is
unquoted which can cause word splitting; replace the backticks with a quoted
command substitution that quotes the variable, e.g. change the line to use
"$(echo "${DOCKER_COMPOSE_FILES}")" (or simply "$DOCKER_COMPOSE_FILES" if no
transformation is needed) so the expansion is quoted and prevents unwanted word
splitting.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.