Skip to content

feat: add Dockerfile and instruction#324

Open
Iondan587 wants to merge 1 commit into
mate-academy:mainfrom
Iondan587:develop
Open

feat: add Dockerfile and instruction#324
Iondan587 wants to merge 1 commit into
mate-academy:mainfrom
Iondan587:develop

Conversation

@Iondan587

Copy link
Copy Markdown

No description provided.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review Summary

I am requesting changes because several core checklist requirements are not yet met and they are blockers for this task. The Dockerfile does not implement a real multi-stage build: you define a base stage (FROM python:${PYTHON_VERSION} AS base) but never use it, instead starting a new FROM python:${PYTHON_VERSION}-slim stage; this violates the requirement for distinct build and run stages and needs to be refactored so dependencies/build steps happen in the build stage and artifacts are copied into the final run stage [CHECKLIST ITEM #5]. The Dockerfile is also missing a RUN python manage.py migrate instruction after the code and dependencies are in place, which is explicitly required and can cause schema-related runtime errors when starting from a fresh database [CHECKLIST ITEM #7]. In INSTRUCTION.md, you show how to build and run the todoapp image, but you do not document tagging the image as todoapp:1.0.0 or pushing it to your Docker Hub repository (e.g. iondan587/todoapp:1.0.0), which is a required part of the assignment [CHECKLIST ITEM #11]. You should also update INSTRUCTION.md to mention that migrations are performed during docker build (or clearly describe any additional migration steps) so the instructions accurately reflect the Dockerfile behavior [CHECKLIST ITEM #20]. As a minor follow-up, please verify that Skeleton CSS and jQuery are actually integrated in the app’s frontend as required, even though that isn’t visible directly from the Docker-related files. Once you address these points, your solution will align well with the task’s requirements and should be ready to pass.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread Dockerfile
Comment on lines +3 to +5
FROM python:${PYTHON_VERSION} AS base

FROM python:${PYTHON_VERSION}-slim

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The task requires the Dockerfile to have a proper build stage and run stage (checklist #5), but here you only define a base stage and then start a new unrelated python:${PYTHON_VERSION}-slim stage; consider explicitly using the build stage (e.g. FROM base AS build then FROM python:${PYTHON_VERSION}-slim AS run and copy artifacts from build).

Comment thread Dockerfile
Comment on lines +13 to +15
RUN pip install --no-cache-dir -r requirements.txt

COPY . .

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The description explicitly requires running database migrations as a RUN instruction in the Dockerfile (checklist #7), but there is no RUN python manage.py migrate here; you should add it after copying the project and installing dependencies.

Comment thread Dockerfile
Comment on lines +5 to +6
FROM python:${PYTHON_VERSION}-slim

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The Docker Hub requirement specifies the image should be pushed with tag 1.0.0 (todoapp:1.0.0, checklist #11), but the build command here does not include that tag; consider documenting building todoapp:1.0.0 and tagging/pushing to your iondan587/todoapp:1.0.0 repository.

Comment thread Dockerfile
Comment on lines +5 to +9
FROM python:${PYTHON_VERSION}-slim

ENV PYTHONUNBUFFERED=1

WORKDIR /app

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The instructions mention how to build and run the container, but they do not explain tagging the image as 1.0.0 and pushing it to Docker Hub, which is required by the task (checklist #11); add commands like docker tag and docker push to cover this.

@@ -0,0 +1,12 @@
# Инструкция по запуску ToDo App

Ссылка на Docker Hub: https://hub.docker.com/r/iondan587/todoapp

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You declare a base stage but never use it; checklist item #5 requires distinct build and run stages, so you should perform dependency installation or build steps in this base stage and then copy results into the final runtime stage instead of reinstalling everything in the second stage.

Comment on lines +5 to +6
## Как собрать образ:
docker build --build-arg PYTHON_VERSION=3.10 -t todoapp .

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The Docker Hub link and basic instructions are good, but checklist item #20 expects full instructions for building and running the container consistent with the Dockerfile, including the fact that database migrations are handled via a RUN instruction; consider clarifying that migrations occur during docker build or documenting any extra steps if needed.

@Losiev Losiev left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please check AI suggestions

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