Skip to content

Solution#86

Open
salabam wants to merge 3 commits into
mate-academy:mainfrom
salabam:develop
Open

Solution#86
salabam wants to merge 3 commits into
mate-academy:mainfrom
salabam:develop

Conversation

@salabam

@salabam salabam commented Feb 9, 2026

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

Changes are needed to address two critical issues preventing your workflow from fully meeting the requirements. First, the defaults.run.working-directory: ./src at the job level will cause steps like pip install -r requirements.txt and artifact uploads to look in the wrong directory, likely breaking your build; please remove this default or apply working-directory explicitly where needed. Second, the assignment requires explicit usage of the coverage tool, but your workflow currently relies on pytest --cov; you'll need to install coverage and invoke it directly, for example, with coverage run -m pytest and coverage report -m. Once these adjustments are made, especially regarding the working directory and direct coverage tool usage, your CI will be robust and fully compliant. You've done a great job with the triggers, run name, Python version, and non-blocking flake8 checks, so you're very close to completion!


✨ 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

@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 there are a few critical issues in the workflow that will likely cause CI failures and do not fully meet the checklist requirements.

Critical issues that must be fixed (blockers):

  1. In .github/workflows/workflow.yaml, the install step uses pip install -r ./src/requirements.txt, but the task description expects requirements.txt at the repo root. This will fail if requirements.txt is not under src; update it to pip install -r requirements.txt or confirm and match the actual path.
  2. The tests are only run inside the coverage step (coverage run src/manage.py test), but the checklist requires a separate test-only step and a separate coverage step. Add a dedicated test step (e.g., python manage.py test or pytest) before the coverage step so both appear explicitly in the job.
  3. The coverage step calls coverage run src/manage.py test, assuming manage.py is under src/, while the project description suggests it lives at the repo root. Either change this to coverage run manage.py test or set the step’s working-directory so it points to the correct location.

Important but easier improvement:

  • The artifact upload step currently uses path: ., which uploads the entire workspace. Narrow this to Python sources only (for example path: '**/*.py' or path: src/**/*.py or specific app folders) to better match the requirement to upload “Python code as an artifact” and avoid unnecessary files.

You’ve done a solid job covering most checklist items (triggers, run-name, flake8, coverage, Python version, etc.); once you fix these path and step-structure issues, your workflow should be both functional and fully compliant.


✨ 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 .github/workflows/workflow.yaml Outdated
run: |
python -m pip install --upgrade pip
pip install pytest coverage flake8
pip install -r ./src/requirements.txt

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 install step runs pip install -r ./src/requirements.txt. The task description and typical project layout expect requirements.txt at the repository root (pip install -r requirements.txt). Verify where requirements.txt actually lives and update this path so dependency installation does not fail.

Comment on lines +35 to +37
- name: Generate coverage
run: |
coverage run src/manage.py test

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 checklist requires a distinct step that runs the tests and a separate step that generates coverage with coverage. Right now tests are only executed via coverage run src/manage.py test. Add an explicit test-only step (for example python manage.py test or pytest) so both steps appear separately in the job as required.


- name: Generate coverage
run: |
coverage run src/manage.py test

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

coverage run src/manage.py test assumes manage.py lives under src/. The project description suggests manage.py is at the repo root. Either correct the path (e.g., coverage run manage.py test) or set an explicit working-directory for that step so coverage runs against the real manage.py.

Comment thread .github/workflows/workflow.yaml Outdated
uses: actions/upload-artifact@v6.0.0
with:
name: python-code
path: .

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 Upload Artifact step uses path: ., which uploads the entire workspace. The requirement asks to upload Python code as an artifact — narrow this to the Python sources (for example path: '**/*.py' or path: src/**/*.py or list the app folders) to avoid uploading unrelated files and to match the spec.

@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 approving your workflow solution as an educational override so you can keep progressing while you refine your CI setup. You did a solid job including the correct triggers (push to main/develop and PRs to main), adding a python-ci job, using an environment variable for the Python version, and configuring flake8 with --exit-zero so style and complexity checks are non-blocking. The main issues to address next are: (1) the repository-wide defaults.run.working-directory: ./src in .github/workflows/workflow.yaml (around the top of the file) causes commands like pip install -r requirements.txt and manage.py calls to look in ./src, which breaks the requirement that they run from the repo root—remove that global default or set a working-directory only on the specific steps that truly need ./src; (2) you’re missing a dedicated test-only step before coverage (e.g., a separate python manage.py test step), which is required so tests aren’t only executed inside the coverage run [CHECKLIST ITEM – test-only step]; (3) your coverage step currently runs coverage run src/manage.py test with the ./src working directory, which will likely resolve to src/src/manage.py—instead, either run coverage run manage.py test from the repo root or set the step’s working-directory to the folder containing manage.py; and (4) the artifact upload path ../ is too broad and effectively uploads the whole workspace, so narrow it to Python sources like **/*.py or your app directories [CHECKLIST ITEM – artifact upload]. Once you fix the working directory usage, add the explicit test step, correct the coverage target path, and tighten the artifact path, your workflow will be both robust and fully aligned with the checklist. You’re very close—these are primarily structural/path tweaks, and your overall CI structure and choice of tools are on the right track.


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

Footnotes

  1. Rate AI review example

Comment on lines +13 to +15
defaults:
run:
working-directory: ./src

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Having a repository-wide defaults.run.working-directory: ./src will cause all steps to run from ./src. This will make pip install -r requirements.txt look for ./src/requirements.txt and will change paths for manage.py. Remove this global default or set working-directory only for steps that need it so commands resolve at the repository root as required by the task.

Comment on lines +24 to +28
- name: Install dependencies
run: |
python -m pip install --upgrade pip
pip install pytest coverage flake8
pip install -r requirements.txt

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 install step runs pip install -r requirements.txt, but with the current ./src working directory this will target ./src/requirements.txt instead of the repository root as the task requires. Ensure this step runs from the repo root (remove global working-directory or add working-directory: ./ to this step) so pip install -r requirements.txt installs the correct file.

Comment on lines +38 to +40
- name: Generate coverage
run: |
coverage run src/manage.py test

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There is no dedicated test-only step. The checklist requires a separate step that runs the tests (for example python manage.py test) before the coverage step. Add an explicit "Run tests" step so tests are not executed only within the coverage run.

Comment on lines +38 to +40
- name: Generate coverage
run: |
coverage run src/manage.py test

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 coverage step runs coverage run src/manage.py test. Given the current defaults this will likely resolve to src/src/manage.py and fail. Run coverage run manage.py test from the repo root or set working-directory for this step to the directory where manage.py actually resides.

Comment on lines +46 to +50
- name: Upload Artifact
uses: actions/upload-artifact@v6.0.0
with:
name: python-code
path: ../

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 artifact upload uses path: ../, which effectively uploads the whole workspace (and is brittle depending on working-directory). The requirement is to upload Python code as an artifact — narrow this to Python sources, e.g. path: '**/*.py' or explicit app folders like path: myapp/**/*.py, so only relevant Python files are uploaded.

Comment on lines +1 to +2
name: python-deployment
run-name: ${{ github.actor }} - ${{ github.sha }}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good: the run-name includes ${{ github.actor }} and ${{ github.sha }}, satisfying the requirement that the run name contain user and commit hash information.

Comment on lines +30 to +36
- name: Lint with flake8
run: |
flake8 . --exit-zero --show-source --statistics

- name: Check Complexity
run: |
flake8 . --exit-zero --max-complexity=10 --statistics

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good: flake8 steps use --exit-zero so style and complexity checks are non-blocking per the requirements.

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.

2 participants