additing ci workflow#88
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are a few critical issues in the CI workflow that prevent it from reliably meeting the stated requirements. First, in .github/workflows/ci.yml the environment variable is defined as PYTHON_VERSION : 3.8 (with a space before the colon), which means the key actually becomes "PYTHON_VERSION " and env.PYTHON_VERSION will not be set correctly; this breaks the requirement that the Python version is controlled from a variable [CHECKLIST ITEM #16, #17]. Second, the coverage step uses pytest -v --cov=. --cov-report=term-missing via pytest-cov, but the task requires using the coverage tool, so you should install coverage and run something like coverage run -m pytest followed by coverage report instead [CHECKLIST ITEM #8]. Third, several steps (Install requirements, Run tests, Lint, Code complexity check) use working-directory: ./src/; if your project (including requirements.txt and tests) is at the repository root, these commands will either fail or run against the wrong files, so you need to either remove working-directory or update it to the correct path [CHECKLIST ITEMS #7, #10, #11, #16]. Once you fix the env key, switch to the coverage CLI, and correct the working directories, your workflow will align well with the requirements; the non-blocking flake8 setup and triggers are otherwise nicely done. Keep those parts as they are while you adjust the issues above.
✨ 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
| workflow_dispatch: | ||
|
|
||
| env: | ||
| PYTHON_VERSION : 3.8 |
There was a problem hiding this comment.
The env entry is written as PYTHON_VERSION : 3.8 (note the extra space before the colon). That makes the environment variable key include a trailing space and env.PYTHON_VERSION will be empty/unset. Change it to PYTHON_VERSION: 3.8 so the Python version is actually controlled by the variable (checklist items #16 and #17).
| working-directory: ./src/ | ||
| run: pip install -r requirements.txt |
There was a problem hiding this comment.
Install requirements runs in working-directory: ./src/ and executes pip install -r requirements.txt. The task description expects pip install -r requirements.txt for the project; if your repository places requirements.txt at the repo root this will fail. Remove or correct working-directory to point to the real project path so requirements are installed correctly (checklist item #16).
| - name: Coverage report | ||
| working-directory: ./src/ | ||
| run: | | ||
| pytest -v --cov=. --cov-report=term-missing |
There was a problem hiding this comment.
The coverage step runs pytest -v --cov=. --cov-report=term-missing (pytest-cov). The task explicitly requests generating coverage with the coverage tool. Install coverage and run coverage run -m pytest followed by coverage report to satisfy checklist item #8.
| - name: Install dependencies | ||
| run: | | ||
| python -m pip install --upgrade pip | ||
| pip install flake8 pytest pytest-cov pytest-django |
There was a problem hiding this comment.
The Install dependencies step installs pytest-cov but not the coverage package. If you switch to coverage run -m pytest (recommended to meet checklist item #8) add coverage to the installed packages here.
| working-directory: ./src/ | ||
| run: | | ||
| flake8 . --show-source --statistics --exit-zero | ||
|
|
||
| - name: Code complexity check | ||
| working-directory: ./src/ |
There was a problem hiding this comment.
Lint and Code complexity check steps use working-directory: ./src/. If your Python code is at the repository root these lint checks will run in the wrong folder. Either remove working-directory to run at the repo root or set it to the correct folder so flake8 inspects the project files (relevant to checklist items #10, #11 and #16).
| run: | | ||
| flake8 . --show-source --statistics --exit-zero |
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are still a couple of configuration issues in .github/workflows/ci.yml that can break the CI and prevent the task from being reliably completed. The main problem is that several steps (installing requirements.txt, running tests with coverage run -m pytest, and running flake8) use working-directory: ./src/, which will fail or run against the wrong files if, as described, requirements.txt, manage.py, and the tests live at the repository root. To fix this, remove working-directory: ./src/ from those steps or change it so they run from the actual project root (or wherever requirements.txt, manage.py, and your tests really are). A second issue is that the artifact upload step currently uses path: ., which uploads the entire repository instead of just the Python code required by the checklist; update this to something like path: "**/*.py" or explicit directories such as path: "app/**" plus key Python entry files (for example manage.py).
Once you correct the working directories so the install, tests, and flake8 all operate on the real project files, and restrict the artifact path to only Python sources, your workflow will fully align with the requirements. The rest of the setup (triggers, run name with user and SHA, coverage commands, and non-blocking flake8 with complexity) looks solid, so you are very close—these are targeted fixes to make the CI reliable and 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
| working-directory: ./src/ | ||
| run: pip install -r requirements.txt |
There was a problem hiding this comment.
If requirements.txt is at the repository root (as in the project description), working-directory: ./src/ will cause this step to fail because the file won't be found. Remove working-directory or set it to the correct path where requirements.txt actually resides so the requirements install runs successfully.
| - name: Runing tests and coverage report | ||
| working-directory: ./src/ | ||
| run: | | ||
| coverage run -m pytest -v | ||
| coverage report -m |
There was a problem hiding this comment.
This test/coverage step runs in ./src/. If tests and manage.py are at the repository root the step will run against the wrong location or find no tests. Run coverage run -m pytest -v from the project root (remove or correct working-directory) so tests and coverage execute correctly.
| - name: Lint | ||
| working-directory: ./src/ | ||
| run: | | ||
| flake8 . --show-source --statistics --exit-zero |
There was a problem hiding this comment.
flake8 is being run from ./src/. Ensure flake8 executes against the actual Python source (remove this working-directory or point it to the package directory). The current use of --exit-zero is correct to keep linting non-blocking as required.
| - name: Code complexity check | ||
| working-directory: ./src/ | ||
| run: | | ||
| flake8 . --statistics --max-complexity=10 --exit-zero |
There was a problem hiding this comment.
The code complexity check is also running in ./src/. Update the working directory or run flake8 against the correct path so --max-complexity analyses your real codebase. The --exit-zero flag correctly makes this non-blocking.
| name: python-source | ||
| path: . |
There was a problem hiding this comment.
The artifact upload uses path: . which will upload the whole repository. The requirement is to upload Python code as an artifact. Change path to include only Python sources (for example **/*.py and package directories, or explicit folders like app/, manage.py) so the artifact contains only Python code.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your workflow as an educational override so you can keep progressing while you learn, but there are a few important fixes to make.
The overall structure is strong: you have the correct triggers, a python-ci job with coverage and flake8, non‑blocking style/complexity checks, artifact upload of **/*.py, and Python version controlled via an env variable, which all align well with the requirements. The main critical issue is the use of working-directory: ./src/ in several steps (for example in .github/workflows/ci.yml in the "Install requirements", "Lint", and "Code complexity check" steps): since requirements.txt, manage.py, and tests are described as living at the repo root, these steps will either fail or run against the wrong files, breaking the requirement to install dependencies with pip install -r requirements.txt and to lint the actual project code. To fix this, run those steps from the repository root (remove working-directory or set it to .) or point them to the real source directory if your project layout differs. Additionally, your coverage test step currently uses coverage run manage.py test; to make this more robust in the Action environment, prefer something like python -m coverage run manage.py test (or coverage run -m pytest if you use pytest) so the command is reliably executed.
Once you correct the working directories and slightly harden the coverage command, your CI will be both functional and aligned with the checklist, and you’ll have a solid, professional‑grade pipeline in place.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| - name: Install requirements | ||
| working-directory: ./src/ | ||
| run: pip install -r requirements.txt |
There was a problem hiding this comment.
The Install requirements step sets working-directory: ./src/ but the task description shows requirements.txt is at the repository root. This will make pip install -r requirements.txt fail or install the wrong file. Remove working-directory: ./src/ or adjust it to the real path so this step runs where requirements.txt actually is. This currently breaks the requirement to install dependencies with pip install -r requirements.txt.
| - name: Lint | ||
| working-directory: ./src/ | ||
| run: | | ||
| flake8 . --show-source --statistics --exit-zero |
There was a problem hiding this comment.
The Lint step runs flake8 with working-directory: ./src/. If your Python sources and manage.py live at the repo root, flake8 will run against the wrong directory (or nothing). Run flake8 from the project root (or point working-directory to the actual source directory). Note: using --exit-zero makes this non-blocking as required, but the directory must still be correct for the check to be meaningful.
| - name: Code complexity check | ||
| working-directory: ./src/ | ||
| run: | | ||
| flake8 . --statistics --max-complexity=10 --exit-zero |
There was a problem hiding this comment.
The Code complexity check step also uses working-directory: ./src/. Same problem as the previous comment — point this to the real source dir or remove the working-directory so flake8 actually analyzes your project files. You already made complexity non-blocking with --exit-zero as required; ensure it runs against the correct files.
| - name: Runing tests and coverage report | ||
| run: coverage run manage.py test |
There was a problem hiding this comment.
The test/coverage step runs coverage run manage.py test. That can be brittle if manage.py is not executable in the runner. Consider invoking coverage robustly, for example coverage run -m pytest if you use pytest, or python -m coverage run manage.py test (or coverage run -m django ...) so the command reliably executes the tests under coverage in the action environment.
No description provided.