Implement solution#89
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because the current workflow is very likely to fail its core responsibilities (installing dependencies, running tests, and generating coverage) due to the defaults.run.working-directory: ./src setting. With that default in place, steps like pip install -r requirements.txt and coverage run --source='.' manage.py test will not find requirements.txt or manage.py if they are in the repository root, which breaks the required tests/coverage checklist items. To fix this, either remove the defaults.run.working-directory block entirely, or set it to the directory that actually contains manage.py and requirements.txt, and adjust commands if needed (for example, coverage run --source='.' ./manage.py test from the repo root). In addition, the task requires separate steps for running tests, generating coverage, and displaying the coverage report; currently these are combined into a single step, so you should split them into three clearly named steps (e.g., Run tests, Generate coverage, Show coverage report). As a smaller improvement, consider narrowing the artifact upload path instead of path: . so you only upload relevant Python source files (for example your app packages or **/*.py), which keeps artifacts smaller and more focused. Once the working-directory and step separation issues are resolved, your workflow will meet the functional requirements well, since the triggers, flake8 checks, artifact upload, and Python version handling are all set up nicely.
✨ 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
| run: | ||
| working-directory: ./src |
There was a problem hiding this comment.
This sets a default working-directory: ./src. If manage.py and requirements.txt are at the repository root (as is common for Django projects), the install and test steps will fail because those files won't be found. Remove this default or change it to the correct directory so tests and coverage run successfully (required to satisfy the tests/coverage checklist items).
|
|
||
| - name: Run tests | ||
| run: | | ||
| coverage run --source='.' manage.py test |
There was a problem hiding this comment.
The test step runs coverage run --source='.' manage.py test. If manage.py is not in the current working directory (see the working-directory default above), this will fail. Ensure the working directory and path to manage.py align (either remove the defaults.run.working-directory, run coverage run --source='.' ./manage.py test from repo root, or set working-directory to the folder containing manage.py).
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: python-bundle | ||
| path: . |
There was a problem hiding this comment.
Uploading path: . will attach the entire repository workspace as an artifact (including potential large or irrelevant files). It's functional but noisy; consider narrowing this to only the Python source (for example package folders or **/*.py) to avoid uploading unnecessary files.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because the current workflow is likely to fail in a typical Django repo layout and does not fully meet the stated CI requirements. In .github/workflows/blank.yml, the Install step uses pip install -r ./src/requirements.txt and the test step runs coverage run --source='./src' ./src/manage.py test; if requirements.txt and manage.py are at the repository root (as the project instructions imply), these commands will break at runtime [CHECKLIST ITEM #28]. To fix this, either remove the ./src/ prefixes (for example, pip install -r requirements.txt and coverage run --source='.' ./manage.py test) or uncomment and set defaults.run.working-directory so it points to the directory containing manage.py and requirements.txt, then adjust paths consistently. Additionally, tests and coverage generation are combined in one step (coverage run directly in the “Run tests” step), but the requirement is to have separate steps for: 1) running tests, 2) generating coverage, and 3) displaying the coverage report [CHECKLIST ITEM #29]. As a minor, non-blocking improvement, consider narrowing the artifact upload path from path: . to something like **/*.py or your app directories to avoid uploading unnecessary files [CHECKLIST ITEM #30]. Overall, your triggers, job naming, flake8 configuration, and use of PYTHON_VERSION are well done; after you align the paths and separate the test/coverage steps, this workflow should satisfy the CI requirements.
✨ 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
| - name: Install dependencies | ||
| run: | | ||
| python -m pip install --upgrade pip | ||
| pip install flake8 coverage | ||
| pip install -r ./src/requirements.txt |
There was a problem hiding this comment.
The Install step uses pip install -r ./src/requirements.txt. The project instructions (and typical Django layout) expect requirements.txt at the repository root, so this path will fail if requirements.txt is not under ./src. Either remove the ./src/ prefix or set the working-directory to the directory that contains requirements.txt (see checklist item #28).
| - name: Run tests | ||
| run: | | ||
| coverage run --source='./src' ./src/manage.py test |
There was a problem hiding this comment.
The Run tests step executes coverage run --source='./src' ./src/manage.py test. If manage.py is at the repo root this will fail. Run from the repo root with coverage run --source='.' ./manage.py test or adjust the working-directory/paths consistently to where the Django project actually lives.
| # defaults: | ||
| # run: | ||
| # working-directory: ./src |
There was a problem hiding this comment.
The defaults working-directory block is commented out. Leaving it commented is safe only if all paths point to the correct locations; to avoid confusion either remove these commented lines or enable/set defaults.run.working-directory to the directory containing manage.py and requirements.txt so commands don't need ./src/ prefixes.
| - name: Upload Python source code | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: python-bundle | ||
| path: . |
There was a problem hiding this comment.
Uploading the whole repository as the artifact (path: .) is functional but uploads a lot of irrelevant files. Consider narrowing this to only Python source files or package directories (for example **/*.py or myapp/**) to keep artifacts small and focused (see checklist item #30).
| name: CI | ||
| run-name: Triggered by ${{ github.actor }} (commit hash - ${{ github.sha}}) | ||
| env: | ||
| PYTHON_VERSION: "3.10" |
There was a problem hiding this comment.
The workflow correctly uses env: PYTHON_VERSION and references it in the setup-python step; triggers and run-name also meet requirements. One remaining non-file action: create a pull request with these changes so the PR workflow triggers and we can verify the checks actually pass (checklist items #16 and #17).
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 refine it further. You did a solid job including the required triggers, setting a PYTHON_VERSION environment variable, configuring flake8 checks as non-blocking, and uploading artifacts — these are all key CI concepts you’ve applied correctly. The main functional issue is that at .github/workflows/blank.yml lines where you set defaults.run.working-directory: ./src, your steps like pip install -r requirements.txt and python manage.py test will fail if manage.py and requirements.txt are actually at the repository root; you can fix this by either removing the defaults.run block or setting it to the directory that truly contains those files [CHECKLIST ITEM #28]. Another important improvement is to separate testing and coverage into three steps instead of combining them: one step running python manage.py test, a second step generating coverage with coverage run --source='.' manage.py test, and a third step showing the report with coverage report -m [CHECKLIST ITEM #29]. As a smaller refinement, your artifact upload at the end currently uses path: ., which grabs the entire repository; consider narrowing that to only relevant files (for example, your app directories or **/*.py) to keep artifacts cleaner and faster to upload [CHECKLIST ITEM #30]. Overall, you’re very close — with these adjustments, your workflow will be both more robust and more closely aligned with professional CI practices.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| defaults: | ||
| run: | ||
| working-directory: ./src |
There was a problem hiding this comment.
The workflow sets a default working directory of ./src for all steps. The project description and the common Django layout expect manage.py and requirements.txt to be in the repository root, so pip install -r requirements.txt and manage.py test will fail unless those files are actually inside ./src. Either remove the defaults.run.working-directory block or set it to the directory that contains manage.py and requirements.txt to ensure commands run from the correct location.
| - name: Run tests | ||
| run: | | ||
| coverage run --source='.' manage.py test |
There was a problem hiding this comment.
The Run tests step uses coverage run --source='.' manage.py test, which combines running tests and collecting coverage in a single step. The task requires separate steps: (1) run tests, (2) generate coverage using coverage, and (3) display the coverage report. Split into distinct steps so the workflow matches the specification (for example: run python manage.py test in the tests step, then coverage run --source='.' manage.py test in the coverage-generation step, then coverage report -m to display results).
| - name: Upload Python source code | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: python-bundle | ||
| path: . |
There was a problem hiding this comment.
The artifact upload uses path: ., which will upload the whole repository (including virtual envs, large files, etc.). This works but is noisy; consider narrowing the path (for example **/*.py or your application directories) so only relevant Python source files are uploaded.
No description provided.