KNOX-3335: Add pylint pre-push linting for .github/workflows/tests Python integration tests#1245
KNOX-3335: Add pylint pre-push linting for .github/workflows/tests Python integration tests#1245Raghav-Mah3shwari wants to merge 6 commits into
Conversation
Test Results21 tests 21 ✅ 1s ⏱️ Results for commit fac48ab. |
moresandeep
left a comment
moresandeep
left a comment
There was a problem hiding this comment.
@Raghav-Mah3shwari looks like the patch has more changes then linting (that i though), can you please describe all the changes? i.e. the images and changes to docker-compose and why are they needed.
Thanks!
| context: ../../../ | ||
| context: ../../.. | ||
| dockerfile: .github/workflows/build/Dockerfile | ||
| image: apache/knox-dev:${IMAGE_TAG:-master} |
There was a problem hiding this comment.
Why are we updating the image tag from ${IMAGE_TAG:-master} to knox-dev:master
What if there are multiple parallel builds? this will cause conflicts.
There was a problem hiding this comment.
yes, good point.. updating it, thank you
|
Hi @moresandeep , Why docker-compose changed Previously, the tests service used python:3.9-slim, bind-mounted ../tests, and only ran pytest. There was no lint step in CI. Why the images changed apache/knox-compose-tests:local (new): built from tests/Dockerfile with pylint, .pylintrc, and test sources baked in. Needed so CI has pylint available and lints the same code it tests. Other compose tweaks tests.yml now builds knox-dev and tests (was knox-dev only). In a summary we can say that local lint (run_pylint.sh) = fast developer feedback; compose/CI lint = mandatory gate for all PRs. |
The tests service builds from ../tests/Dockerfile; without this file CI fails with "open Dockerfile: no such file or directory". Co-authored-by: Cursor <cursoragent@cursor.com>
2 participants
What changes were proposed in this pull request?
This PR adds local pylint linting for Python integration tests under .github/workflows/tests/ and fixes existing pylint violations so the suite passes cleanly.
Linting infrastructure
Added .pylintrc with test-friendly settings (100-char line limit, common test-docstring rules disabled).
Added run_pylint.sh to lint changed or staged .py files under .github/workflows/tests/; creates a local venv (.venv/) and installs requirements.txt plus pylint.
Updated README.md with a Linting (pylint) section covering manual runs and optional git hook setup.
Code quality fixes
Resolved pylint issues across all integration test modules and common_utils.py (line length, trailing whitespace, trailing newlines).
All files under .github/workflows/tests/ now score 10.00/10 with the new config.
No functional changes to test logic — only style and lint tooling.
How was this patch tested?
Pylint (manual)
./.github/workflows/tests/run_pylint.sh
Result: all test_*.py and common_utils.py pass at 10.00/10.
Pylint (targeted)
cd .github/workflows/tests
.venv/bin/pylint --rcfile=.pylintrc -j 0 test_*.py common_utils.py
Result: exit code 0, no violations.
Script behavior
Verified run_pylint.sh creates .venv/ on first run and reuses it on subsequent runs.
Verified it only lints files under .github/workflows/tests/ that are modified or staged.
Integration Tests
No new integration tests were added. This change is lint tooling and style fixes for the existing Docker Compose test suite.
Existing tests in .github/workflows/tests/ were not changed in behavior. To confirm they still pass end-to-end:
docker compose -f ./.github/workflows/compose/docker-compose.yml build knox-dev tests
docker compose -f ./.github/workflows/compose/docker-compose.yml up -d
docker compose -f ./.github/workflows/compose/docker-compose.yml up --exit-code-from tests tests
UI changes
N/A — no UI changes.