Improve CI code checks (URL path, Python syntax, linting, tests) #1289
Conversation
WalkthroughAdds two CI jobs to the existing GitHub Actions workflow: a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant GH as GitHub Actions
participant Runner as Runner
participant Lint as Lint job
participant Test as Test job
participant Other as Existing checks
GH->>Runner: trigger code_checks workflow
par Parallel jobs
Runner->>Other: run existing checks (check-url-paths, python-syntax)
Runner->>Lint: run lint steps
Runner->>Test: run test steps
end
Lint-->>Runner: report lint results
Test-->>Runner: report test results
Other-->>Runner: report results
Runner-->>GH: aggregate job statuses
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/code_checks.yml (2)
58-59: Hadolint download via /latest/ endpoint is fragile.Using
/latest/download/can break if the release structure or naming convention changes. Consider either:
- Pinning to a specific version (e.g.,
v2.12.0) for reproducibility- Using
apt-get install hadolintif available in the default repos- Using a dedicated Dockerfile linting GitHub Action
If pinning a version, apply this diff:
- wget -O /tmp/hadolint https://github.com/hadolint/hadolint/releases/latest/download/hadolint-Linux-x86_64 + wget -O /tmp/hadolint https://github.com/hadolint/hadolint/releases/download/v2.12.0/hadolint-Linux-x86_64Alternatively, simplify to use the system package (if available):
- wget -O /tmp/hadolint https://github.com/hadolint/hadolint/releases/latest/download/hadolint-Linux-x86_64 - chmod +x /tmp/hadolint + apt-get install -y hadolint
83-86: Remove redundant fallback operator with Docker lint.The step already has
continue-on-error: true, so the|| trueat the end of line 86 is redundant and may mask real hadolint failures. Simplify to let hadolint output speak for itself.- name: Docker lint continue-on-error: true run: | echo "🔍 Linting Dockerfiles..." - /tmp/hadolint Dockerfile* || true + find . -name "Dockerfile*" -exec /tmp/hadolint {} \;Using
findis more explicit and handles the edge case where no Dockerfiles exist without swallowing errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/code_checks.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.8)
.github/workflows/code_checks.yml
49-49: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
95-95: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: docker_dev
🔇 Additional comments (3)
.github/workflows/code_checks.yml (3)
100-102: Verify requirements.txt exists before installing.The test job assumes
requirements.txtexists at the repo root. If it's missing,pip install -r requirements.txtwill fail. Consider adding a guard or error handling to clarify the expectation.Please confirm that
requirements.txtis always present in the repository root. If it's optional or may not exist in all branches, apply defensive error handling:run: | - pip install -r requirements.txt + if [ -f requirements.txt ]; then pip install -r requirements.txt; fi pip install pytestAlternatively, if it's required, add a comment documenting that expectation.
69-74: Python linting strategy is sound.Running flake8 in two passes (specific errors first, then complexity/warnings) is a solid approach. The targeted error codes (E9, F63, F7, F82) catch correctness issues early, and the second pass enforces complexity constraints. This aligns well with the PR goal to catch critical errors without blocking on style warnings.
63-67: Shell linting integration looks good.The shellcheck step correctly discovers all
.shfiles and enforces shell script best practices. Combined withcontinue-on-error: true, it allows the workflow to capture warnings without blocking CI during iterative development.
|
Worked on my setup. Need time to re-evaluate in this env. Looks like it needs a python path. Will push tomorrow sometime. |
Updated Python setup action to version 5 and specified Python version 3.11. Also modified dependencies installation to include pyyaml.
adamoutler
force-pushed
the
more-code-checks
branch
from
d8f684c to
a93e874
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/code_checks.yml (2)
82-87: Remove redundant|| truein Docker lint step.Line 86 has
|| trueafter the first hadolint command, but the entire step already hascontinue-on-error: true. The|| trueis redundant and adds unnecessary noise.- name: Docker lint continue-on-error: true run: | echo "🔍 Linting Dockerfiles..." - /tmp/hadolint Dockerfile* || true + /tmp/hadolint Dockerfile* find . -name "Dockerfile*" -exec /tmp/hadolint {} \;
58-61: ---Consider adding checksum verification to the hadolint download for supply-chain security.
The hadolint binary is downloaded from GitHub releases (line 58) without integrity verification. Since hadolint is not available in Ubuntu's official package repositories, the binary download is the standard installation method. Adding checksum verification would strengthen supply-chain security without requiring a package manager alternative.
If desired, verify the binary against checksums published alongside the hadolint release on GitHub.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/code_checks.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: docker_dev
🔇 Additional comments (1)
.github/workflows/code_checks.yml (1)
42-110: All verification checks passed — code is properly configured.The pytest markers (
docker,compose,feature_complete) are actively used throughout the test suite and will be correctly excluded by the workflow. The./serverdirectory exists,requirements.txtis present, and the PYTHONPATH export is valid. The workflow structure, two-stage flake8 approach, andcontinue-on-error: trueflags are sound.
|
This PR is ready. The detected test failures are legitimate. These tests are failing because they are actually failing. #1291 fixes the tests. |
2 participants
Summary:
check-url-pathsjob to prevent incorrect absolute/php/calls in frontend code (should bephp/or./php/)..pyfiles.shellcheck,flake8,php -l, andhadolintcontinue-on-error: truefor non-blocking lint steps so we can iterate and fix warnings without breaking other CI checks immediately.pytestwith the-m "not (docker or compose or feature_complete)"marker exclusion.What changed and why:
.pyfiles to catch syntax errors early during CI rather than later in runs or on the backend startup.These checks exclude certain items lsuch as line length and certain artistic expressions such as code spacing which can be more readable when left as-is.
Summary by CodeRabbit