Test PR #1
Test PR #1
Conversation
Reviewer's GuideThis pull request revamps the project's Python development and build infrastructure by migrating to Class Diagram: LockManager UpdateclassDiagram
class LockManager {
-_lock_conf: dict
-_process_lockfile_path: str
-_process_zk_lockfile_path: str
-_zk_lock: Lock
-_file: IO
-_lock_id: str
+__init__(config, name)
+__enter__()
+__exit__()
-_flock()
-_zk_flock()
}
note for LockManager "Changes in _flock():\n- File opened in 'r+' mode instead of 'r'.\n- flock() now uses the file object directly (self._file) instead of a file descriptor (self._fd).\n- _fd attribute removed.\nChange in __exit__():\n- self._file.close() used instead of os.close(self._fd)."
Flow Diagram: Makefile Tasks Execution via uvflowchart TD
subgraph Makefile Task Execution with uv
User["User/CI"] -- "invokes 'make <target>' (e.g. lint, test-unit, build)" --> Makefile["Makefile"]
Makefile -- "'make lint' triggers multiple 'uv run ...'" --> UV_Lint["uv run isort<BR>uv run black<BR>uv run ruff<BR>..."]
UV_Lint --> Tools_Lint["Linters"]
Makefile -- "'make test-unit' triggers 'uv run py.test ...'" --> UV_Test["uv run py.test ..."]
UV_Test --> PyTest["pytest"]
Makefile -- "'make build' triggers 'uv build ...'" --> UV_Build["uv build --python <version>"]
UV_Build --> Build_Process["Python packaging (wheel/sdist)"]
Makefile -- "'make setup' (dependency for many tasks)" --> UV_Check["uv check / uv sync (implicitly via uv run)"]
UV_Check --> Dep_Management["Dependency Management"]
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
|
@sourcery-ai review |
|
@sourcery-ai summary |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Alex-Burmak - I've reviewed your changes - here's some feedback:
- For the 'Invalid thread ID'
ValueErrorinpipelines.py, consider adding an inline comment linking to any upstreamstopit/pypelnissue for context. - The
LockManagernow opens its lockfile inr+mode forLOCK_SH; clarify if this change fromrmode was intentional, asris usually sufficient.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| - python: "3.13" | ||
| clickhouse: "latest" | ||
| runs-on: ubuntu-22.04 | ||
| steps: |
There was a problem hiding this comment.
suggestion (bug_risk): Propagating PYTHON_VERSION in CI workflows is a good practice.
Apply PYTHON_VERSION=${{ matrix.python }} to every step (unit and integration) so uv runs with the intended Python version, and verify consistency across commands to prevent mismatches.
Suggested implementation:
- uses: astral-sh/setup-uv@v5
with:
python-version: ${{ matrix.python }}
Make sure that the setup action (astral-sh/setup-uv@v5) supports accepting a "python-version" parameter.
There was a problem hiding this comment.
Hey @Alex-Burmak - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
-
The server.key file is generated in the Dockerfile, which is a security risk. (link)
-
The server.key file is copied into the Docker image, which is a security risk. (link)
-
Consider creating a tracking issue for the
stopitlibrary workaround inpipelines.pyto explore a long-term fix.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🔴 Security: 2 blocking issues
- 🟡 Testing: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| try: | ||
| itr = iter(pipeline) | ||
| exhaust_iterator(itr) | ||
| except ValueError as e: |
There was a problem hiding this comment.
question (bug_risk): Exception handling in pipeline iteration.
Ensure stopit reliably raises ValueError with 'Invalid thread ID'; otherwise, other ValueErrors might be inadvertently skipped.
| with ( | ||
| open(original_file_path, "rb") as orig_fobj, | ||
| open(backward_file_name, "rb") as res_fobj, | ||
| ): |
There was a problem hiding this comment.
issue (testing): Add tests for pipelines.run() error handling to cover new ValueError logic.
Please add unit tests for these cases:
- When
exhaust_iteratorraisesValueError("Invalid thread ID"), confirm it’s caught, logs a warning withexc_info=True, and isn’t re-raised. - When it raises
ValueErrorwith a different message, confirm it’s re-raised. - (Optional) The normal path where no exception occurs.
You’ll need to mockexhaust_iteratoror the underlying iterator to simulate each scenario.
| RUN openssl genrsa -out ${CH_TMP_DIR}/ssl/server.key 2048 && \ | ||
| openssl req -new -key ${CH_TMP_DIR}/ssl/server.key -out ${CH_TMP_DIR}/ssl/server.csr \ | ||
| COPY staging/images/{{ instance_name }}/ssl/ /var/tmp/ssl/ | ||
| RUN openssl genrsa -out /var/tmp/ssl/server.key 2048 && \ |
There was a problem hiding this comment.
🚨 issue (security): The server.key file is generated in the Dockerfile, which is a security risk.
The server.key file should not be generated in the Dockerfile. Instead, it should be generated outside of the Dockerfile and then copied into the Dockerfile using the COPY command.
| cp ${CH_TMP_DIR}/ssl/server.key /etc/clickhouse-server/ && \ | ||
| cp ${CH_TMP_DIR}/ssl/server.crt /etc/clickhouse-server/ && \ | ||
| cp ${CH_TMP_DIR}/ssl/rootCACert.pem /etc/clickhouse-server/allCAs.pem && \ | ||
| cp /var/tmp/ssl/server.key /etc/clickhouse-server/ && \ |
There was a problem hiding this comment.
🚨 issue (security): The server.key file is copied into the Docker image, which is a security risk.
The server.key file should not be copied into the Docker image. Instead, it should be mounted as a volume at runtime.
| from tests.unit.time_mocker import TimeMocker | ||
|
|
||
| from ch_backup.storage.async_pipeline.base_pipeline.rate_limiter import RateLimiter | ||
| from tests.unit.time_mocker import TimeMocker |
There was a problem hiding this comment.
issue (code-quality): Don't import test modules. (dont-import-test-modules)
Explanation
Don't import test modules.Tests should be self-contained and don't depend on each other.
If a helper function is used by multiple tests,
define it in a helper module,
instead of importing one test from the other.
| from tests.unit.time_mocker import TimeMocker | ||
|
|
||
| from ch_backup.storage.engine.s3.s3_retry import retry | ||
| from tests.unit.time_mocker import TimeMocker |
There was a problem hiding this comment.
issue (code-quality): Don't import test modules. (dont-import-test-modules)
Explanation
Don't import test modules.Tests should be self-contained and don't depend on each other.
If a helper function is used by multiple tests,
define it in a helper module,
instead of importing one test from the other.
|
|
||
| from ch_backup.clickhouse.models import Table | ||
| from ch_backup.clickhouse.schema import rewrite_table_schema | ||
| from tests.unit.utils import parametrize |
There was a problem hiding this comment.
issue (code-quality): Don't import test modules. (dont-import-test-modules)
Explanation
Don't import test modules.Tests should be self-contained and don't depend on each other.
If a helper function is used by multiple tests,
define it in a helper module,
instead of importing one test from the other.
| from ch_backup.backup_context import BackupContext | ||
| from ch_backup.clickhouse.models import Database | ||
| from ch_backup.logic.upload_part_observer import UploadPartObserver | ||
| from tests.unit.utils import parametrize |
There was a problem hiding this comment.
issue (code-quality): Don't import test modules. (dont-import-test-modules)
Explanation
Don't import test modules.Tests should be self-contained and don't depend on each other.
If a helper function is used by multiple tests,
define it in a helper module,
instead of importing one test from the other.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis update transitions the project’s Python environment management from manual virtual environments to the Changes
(Files grouped for brevity; see raw summary for exhaustive details.) Sequence Diagram(s)sequenceDiagram
participant Developer
participant Makefile
participant uv
participant Python
participant TestRunner
Developer->>Makefile: make test-unit
Makefile->>uv: run --python $(PYTHON_VERSION) pytest tests/unit
uv->>Python: Setup Python environment (3.10/.python-version)
Python->>TestRunner: Run unit tests
TestRunner-->>Python: Results
Python-->>uv: Exit
uv-->>Makefile: Exit
Makefile-->>Developer: Test results/output
sequenceDiagram
participant CI
participant GitHub Actions
participant astral-sh/setup-uv
participant uv
participant Python
participant Makefile
CI->>GitHub Actions: Trigger workflow
GitHub Actions->>astral-sh/setup-uv: Setup environment
astral-sh/setup-uv->>uv: Install uv tool
uv->>Python: Setup Python environment (matrix version)
GitHub Actions->>Makefile: Run lint/test/build steps via uv
Makefile->>uv: Execute commands (e.g., pytest, ruff)
uv-->>Makefile: Command output
Makefile-->>GitHub Actions: Step results
GitHub Actions-->>CI: Workflow status
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (8)
tests/unit/test_cli.py (1)
5-7: Avoid importing test modules in tests
The test still importsparametrizefromtests.unit.utils, which couples tests to each other. Consider moving shared utilities into a separate helper or conftest to keep tests self-contained.tests/unit/test_retry_exponetial.py (1)
9-11: Avoid importing test modules in tests
The test continues to importTimeMockerfromtests.unit.time_mocker, creating inter-test dependencies. Please extract this utility to a shared helper location to keep tests independent.tests/unit/test_schema.py (1)
7-7: LGTM: Import order adjustment.The import reordering is consistent with the code style changes in this pull request.
tests/unit/test_pipeline.py (1)
1-338: Missing tests for ValueError handlingAdd tests for
pipelines.run()error handling to cover newValueErrorlogic.Please add unit tests for these cases:
- When
exhaust_iteratorraisesValueError("Invalid thread ID"), confirm it's caught, logs a warning withexc_info=True, and isn't re-raised.- When it raises
ValueErrorwith a different message, confirm it's re-raised.- (Optional) The normal path where no exception occurs.
You'll need to mockexhaust_iteratoror the underlying iterator to simulate each scenario.ch_backup/storage/async_pipeline/pipelines.py (1)
200-211: Improved error handling for thread termination issue.The added try-except block addresses a specific threading-related issue during pipeline iteration exhaustion. The change properly suppresses only the specific "Invalid thread ID" error from the stopit library while re-raising all other ValueError exceptions, improving robustness without changing core functionality.
This change directly addresses the previous review comment that noted: "Ensure stopit reliably raises ValueError with 'Invalid thread ID'; otherwise, other ValueErrors might be inadvertently skipped." The implementation correctly handles this specific case.
.github/workflows/workflow.yml (2)
19-19: Propagate Python version to setup-uv action
Theastral-sh/setup-uv@v5step in the lint job should explicitly receive the Python version from the matrix to avoid any mismatch between the runner’s default Python and the one used byuv.Consider:
- - uses: astral-sh/setup-uv@v5 + - uses: astral-sh/setup-uv@v5 + with: + python-version: ${{ matrix.python }}
67-67: Propagate Python version to setup-uv action in test job
Same as in the lint job, the test job’sastral-sh/setup-uv@v5step should takepython-version: ${{ matrix.python }}to guarantee consistent environments.images/clickhouse/Dockerfile (1)
61-76: 🚨 SECURITY: Avoid generating and storing private keys in image
The Dockerfile still generatesserver.keyinside the image and copies it into the container. This reproduces the security risk noted in previous reviews. Private keys should be generated outside the Docker build and mounted at runtime.
🧹 Nitpick comments (4)
mypy.ini (1)
2-2: Align Mypy Python version with project default
Thepython_version = 3.9setting should match the version in your.python-versionfile (3.10) to avoid discrepancies between type-checking and runtime. Consider updating this line or deriving the version dynamically.images/clickhouse/Dockerfile (1)
53-55: Install pre-built ch_backup wheel from /var/tmp
Switching to wheel installation is a nice improvement. Consider cleaning up/var/tmpafter install to reduce image size:RUN cd /var/tmp/ && \ pip3 install ch_backup*.whl && \ + rm ch_backup*.whl && \ mkdir -p /etc/yandex/ch-backup && \pyproject.toml (1)
15-15: Nitpick: Verify Trove classifier for MacOS
The classifier"Operating System :: MacOS"may not match PyPI’s accepted trove list (usually"Operating System :: MacOS"vs"macOS"). It’s worth double-checking against the official classifier list.Makefile (1)
100-108: Test targets: add explicit Python version forbehave
Thetest-unittarget correctly usesuv run --python. Fortest-integration, consider adding--python $(PYTHON_VERSION)to theuv run behaveinvocation for consistency:- uv run behave --show-timings --stop -D skip_setup $(BEHAVE_ARGS) @tests/integration/ch_backup.featureset + uv run --python $(PYTHON_VERSION) behave --show-timings --stop -D skip_setup $(BEHAVE_ARGS) @tests/integration/ch_backup.featureset
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (44)
.codespellrc(0 hunks).github/workflows/test_clickhouse_version.yml(1 hunks).github/workflows/workflow.yml(2 hunks).gitignore(1 hunks).isort.cfg(0 hunks).pylintrc(1 hunks).python-version(1 hunks)AUTHORS(1 hunks)Dockerfile-deb-build(1 hunks)Makefile(7 hunks)ch_backup/clickhouse/control.py(1 hunks)ch_backup/logic/lock_manager.py(2 hunks)ch_backup/storage/async_pipeline/base_pipeline/exec_pool.py(1 hunks)ch_backup/storage/async_pipeline/pipeline_builder.py(1 hunks)ch_backup/storage/async_pipeline/pipelines.py(2 hunks)debian/control(1 hunks)debian/rules(1 hunks)images/clickhouse/Dockerfile(2 hunks)mypy.ini(1 hunks)pyproject.toml(1 hunks)requirements-dev.txt(0 hunks)requirements.txt(0 hunks)setup.py(0 hunks)tests/integration/features/backup_replicated.feature(1 hunks)tests/integration/modules/clickhouse.py(1 hunks)tests/integration/modules/compose.py(2 hunks)tests/integration/modules/utils.py(1 hunks)tests/integration/modules/zookeeper.py(1 hunks)tests/integration/steps/ch_backup.py(1 hunks)tests/integration/steps/clickhouse.py(1 hunks)tests/integration/steps/common.py(1 hunks)tests/integration/steps/s3.py(1 hunks)tests/integration/steps/zookeeper.py(1 hunks)tests/unit/test_access.py(1 hunks)tests/unit/test_backup_tables.py(1 hunks)tests/unit/test_cli.py(1 hunks)tests/unit/test_clickhouse_encryption.py(1 hunks)tests/unit/test_control.py(1 hunks)tests/unit/test_disks.py(1 hunks)tests/unit/test_pipeline.py(1 hunks)tests/unit/test_rate_limiter.py(1 hunks)tests/unit/test_retry_exponetial.py(1 hunks)tests/unit/test_schema.py(1 hunks)tests/unit/test_upload_part_observer.py(1 hunks)
💤 Files with no reviewable changes (5)
- .isort.cfg
- .codespellrc
- setup.py
- requirements-dev.txt
- requirements.txt
🧰 Additional context used
🧬 Code Graph Analysis (10)
tests/unit/test_upload_part_observer.py (1)
tests/unit/utils.py (1)
parametrize(16-74)
tests/unit/test_cli.py (1)
tests/unit/utils.py (1)
parametrize(16-74)
tests/unit/test_control.py (1)
tests/unit/utils.py (1)
parametrize(16-74)
tests/unit/test_retry_exponetial.py (1)
tests/unit/time_mocker.py (1)
TimeMocker(6-14)
tests/unit/test_clickhouse_encryption.py (1)
tests/unit/utils.py (2)
assert_equal(154-160)parametrize(16-74)
tests/unit/test_rate_limiter.py (1)
tests/unit/time_mocker.py (1)
TimeMocker(6-14)
tests/unit/test_disks.py (1)
tests/unit/utils.py (2)
assert_equal(154-160)parametrize(16-74)
tests/unit/test_access.py (1)
tests/unit/utils.py (1)
parametrize(16-74)
tests/unit/test_schema.py (1)
tests/unit/utils.py (1)
parametrize(16-74)
ch_backup/storage/async_pipeline/pipelines.py (2)
ch_backup/util.py (1)
exhaust_iterator(409-413)ch_backup/logging.py (1)
warning(150-155)
🔇 Additional comments (54)
tests/integration/steps/common.py (1)
6-7: Import grouping formatting improvement
The added blank line after thebehaveimports enhances readability and aligns with the grouping style used across other integration step files.tests/integration/modules/zookeeper.py (1)
9-11: Import grouping formatting improvement
The blank line added after thekazoo.exceptionsimport matches the import grouping conventions elsewhere and improves clarity.tests/integration/steps/s3.py (1)
6-7: Import grouping formatting improvement
Inserting a blank line after thehamcrestimports maintains consistent grouping and mirrors the style updates in other integration steps.tests/unit/test_rate_limiter.py (1)
9-10: Consistent import grouping
TheTimeMockerimport is now placed after the third-partyRateLimiterimport, improving separation between external and local test utility imports.tests/integration/steps/zookeeper.py (1)
6-7: Add blank line for import separation
A blank line was inserted after thehamcrestimport to clearly separate third-party dependencies from local module imports, aligning with the project’s import grouping conventions.tests/integration/steps/clickhouse.py (1)
8-9: Insert blank line between import groups
This formatting change adds a blank line after thetenacityimport, improving readability by distinguishing external libraries from application-specific modules.tests/integration/steps/ch_backup.py (1)
16-18: Insert blank line post-import block
A blank line was added after the Hamcrest matcher imports to consistently group and separate external dependencies from local test modules.tests/unit/test_clickhouse_encryption.py (1)
11-12: Reorder test utility imports
The imports ofassert_equalandparametrizefromtests.unit.utilshave been moved below the application imports, ensuring that third-party and application imports precede test helper utilities.tests/unit/test_control.py (1)
2-2: LGTM: Import reordering aligns with project standardizationThis change is part of a broader standardization of imports across the codebase, moving imports from test utilities to appear after imports from main application modules.
tests/unit/test_upload_part_observer.py (1)
11-11: LGTM: Import reordering follows project conventionThis change is consistent with the standardization of import order across the codebase as part of the migration to use the
uvtool and modern Python packaging.tests/unit/test_disks.py (1)
13-13: LGTM: Standardized import orderThis import reordering places test utility imports after main application module imports, consistent with other changes in the PR.
tests/unit/test_access.py (1)
11-11: LGTM: Consistent import orderingMoving the test utility import below main application imports follows the standardized pattern being applied across the codebase.
tests/unit/test_backup_tables.py (1)
109-112: Improved syntax for multiple context managers.The improved syntax for using multiple context managers in a single
withstatement enhances code readability. This is a good Python practice.ch_backup/storage/async_pipeline/base_pipeline/exec_pool.py (1)
57-57: Added trailing comma in parameter list.Adding a trailing comma after the last parameter in a multi-line parameter list is good practice. It improves maintainability by making future parameter additions cleaner.
ch_backup/storage/async_pipeline/pipeline_builder.py (1)
245-245: Added trailing comma after unpacked arguments.Adding a trailing comma after the
*stagesunpacking is good practice and consistent with modern Python style guidelines.ch_backup/logic/lock_manager.py (2)
96-97: Good simplification of file management.This change improves the code by using the file object's close method directly instead of closing a file descriptor with
os.close(). This makes the code more consistent and eliminates potential file descriptor management issues.
111-112: Improved file locking implementation.The changes to use the "r+" mode when opening the file and apply the lock directly on the file object rather than using a file descriptor is more Pythonic and consistent with the rest of the implementation. This eliminates mixing different file handling approaches and simplifies the code.
AUTHORS (2)
1-2: Name and format standardization looks good.The standardization of email formats and name ordering improves consistency in the contributor list.
Also applies to: 6-6
18-20: New contributors added correctly.The addition of the new contributors is properly formatted and consistent with the rest of the file.
.pylintrc (1)
92-92: Python version update aligns with project modernization.Updating the minimum Python version from 3.6 to 3.9 for Pylint checks is appropriate as part of the project's migration to modern Python standards. This ensures Pylint will apply the correct syntax and feature checks based on Python 3.9+ compatibility.
.python-version (1)
1-1: Good addition for standardizing Python environments.Adding a
.python-versionfile with a specific Python version (3.10) is an excellent practice for ensuring consistent development environments. This works with tools like pyenv and aligns with the project's move to theuvtool for Python dependency management.debian/control (1)
5-5: Minimum Python version bump is consistent
Requiringpython3 (>= 3.9)aligns with the CI, mypy, and packaging changes elsewhere in the project.ch_backup/clickhouse/control.py (1)
15-15: Usepackaging.version.parsefor version comparisons
Switching frompkg_resourcestopackaging.version.parseis a solid modernization—packagingis lighter and the recommended API for version handling.tests/integration/modules/utils.py (1)
13-13: Replacepkg_resourceswithpackaging.version
Updating the import tofrom packaging.version import parse as parse_versionkeeps version parsing consistent across modules and removes the setuptools dependency..gitignore (1)
14-14: Ignore the new local virtual environment directory
Adding.venv/is appropriate given the switch touv-managed environments.tests/integration/modules/clickhouse.py (1)
11-11: Updated import for version parsingThe change from
pkg_resourcestopackaging.versionimproves dependency handling by using a more modern and maintained package. This is consistent with the PR objective of updating dependency management to useuv.tests/integration/features/backup_replicated.feature (1)
867-867: Fixed indentation in YAML configurationThe indentation correction improves readability and ensures proper parsing of the YAML block.
Dockerfile-deb-build (1)
30-35: Added uv tool installationThe Docker build now installs and configures the
uvtool, which aligns with the PR's goal of migrating to modern Python dependency management. The symbolic links ensure proper system-wide access.tests/unit/test_pipeline.py (1)
167-170: Improved readability for multiple context managersThe refactored syntax using parentheses to group multiple
open()calls makes the code more readable while maintaining the same functionality..github/workflows/test_clickhouse_version.yml (1)
23-23: LGTM! The migration touvfor Python environment management looks good.This change aligns with the PR objectives of migrating to the
uvtool for Python dependency management. Replacing the explicit Python setup step withastral-sh/setup-uv@v5is the correct approach for integrating with the new tooling system.tests/integration/modules/compose.py (2)
7-7: LGTM! Added import for the new dynamic Docker Compose detection.The added import of
shutilis properly used for the dynamic Docker Compose command detection implementation.
187-190: LGTM! Great compatibility improvement for Docker Compose.The implementation intelligently detects whether to use the legacy
docker-composecommand or the newerdocker composesyntax based on command availability. This ensures compatibility across different environments where Docker Compose might be installed differently.debian/rules (1)
4-5: LGTM! Standardization to explicitpython3usage.This change simplifies the build process by removing the dependency on the
$(PYTHON)variable and directly usingpython3to determine version components. This aligns with the overall project modernization goals and standardizes the Python tooling approach.ch_backup/storage/async_pipeline/pipelines.py (1)
9-9: LGTM! Added import for the enhanced error handling.The added import of the logging module is properly used in the error handling enhancement.
.github/workflows/workflow.yml (2)
51-63: Approve updated test matrix entries
The ClickHouse matrix has been expanded to include versions24.3.18.7,24.8.14.39,25.3.3.42, and Python 3.13. This aligns with the updated compatibility goals.
69-71: Passing PYTHON_VERSION to Make targets
Great to seePYTHON_VERSION=${{ matrix.python }}being set for bothmake test-unitandmake test-integration. This ensures the Makefile picks up the intended Python version.images/clickhouse/Dockerfile (1)
35-36: Keyring file copy and apt sources configuration look good
The clickhouse-keyring is correctly copied into/usr/share/keyrings, and the newRUNblock configures the apt source list securely withsigned-by.🧰 Tools
🪛 Hadolint (2.12.0)
[error] 35-35: COPY with more than 2 arguments requires the last argument to end with /
(DL3021)
pyproject.toml (5)
1-14: Project metadata is well-defined
The[project]section correctly centralizes name, description, license, dynamic versioning, and Python compatibility (>=3.9).
33-55: Runtime dependencies look accurate and conditional bounds are clear
Version constraints forboto3,botocore,requests, and others correctly account for Python-version-specific compatibility.
57-81: Development dependency-group is comprehensive
All linting, testing, and packaging tools have been migrated intodev. Conditional bounds fordockerandwebsocket-clientare properly set.
83-89: Build system and entry point configuration is correct
Usinghatchlingas the build-backend and exposing thech-backupscript aligns with modern Python packaging standards.
98-103: Tool-specific configurations migrated successfully
Theisortprofile andcodespellignore-words-list are brought intopyproject.toml, removing legacy config files.Makefile (12)
3-8: Environment variable exports fine
ReadingPYTHON_VERSIONfrom.python-version, plus setting encoding,COMPOSE_HTTP_TIMEOUT, andCLICKHOUSE_VERSION, provides a clear configuration surface.
28-33: Project directory and tool variables
DefiningSRC_DIR,TESTS_DIR,VENV,SESSION_FILE, andINSTALL_DIRimproves readability and DRYness.
36-43: Build and setup targets useuvcorrectly
Thebuildtarget now depends onsetupand runsuv build --python $(PYTHON_VERSION), andsetupchecks environment before generating version.
45-51:allandlinttarget chaining looks solid
Aggregatinglint,test-unit,build, andtest-integrationunderall, and makinglintdepend on the newly updated lint sub-targets is consistent.
53-92: Lint sub-targets updated to useuv run --python
All linters (isort,black,codespell,ruff,pylint,mypy,bandit) invokeuv run --python $(PYTHON_VERSION), ensuring they run in the correct managed environment.
94-98: Format target consistency
Theformattarget invokesisortandblackviauv run. This aligns with the CI and keeps local workflows uniform.
111-119: Cleanup targets remain effective
cleanandclean-pycacheremove artifacts including.venv, caches, and generated version files. This ensures a fresh state.
121-144: Install/uninstall targets for packaging
Using a Python venv inINSTALL_DIRto install the wheel simplifies the packaging step and preserves isolation. Uninstall cleans up accordingly.
146-161: Debian package build targets are in place
build-deb-package,build-deb-package-local, andprepare-changelogtargets correctly wrap the deb build scripts, leveraging the version file.
170-189: Integration test environment lifecycle
create-test-env,start-test-env,stop-test-env, andclean-test-envencapsulate test environment management cleanly withINTEGRATION_TEST_TOOL.
193-204: Version file generation and environment check
Generatingch_backup/version.txtvia git, and verifying the presence ofuvand a non-emptyPYTHON_VERSIONprovides a robust guardrail.
207-236: Help target updated to reflect new commands
Thehelptarget lists all current make targets and environment variables, removing outdated references.
|
@CodiumAI-Agent /describe |
TitleTest PR User descriptionSummary by SourceryMigrate project to use uv for Python dependency management and build system, update project configuration, and modernize development workflows Enhancements:
Build:
CI:
Chores:
Summary by CodeRabbit
PR TypeEnhancement, Bug fix, Tests Description
Changes walkthrough 📝
|
|
@CodiumAI-Agent /review |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
@CodiumAI-Agent /improve |
PR Code Suggestions ✨
|
Alex-Burmak
force-pushed
the
main
branch
5 times, most recently
from
bcef9a7 to
d45fbce
Compare
Alex-Burmak
force-pushed
the
main
branch
2 times, most recently
from
6c1cd87 to
6b13808
Compare
2 participants
Summary by Sourcery
Migrate project to use uv for Python dependency management and build system, update project configuration, and modernize development workflows
Enhancements:
Build:
CI:
Chores:
Summary by CodeRabbit
New Features
pyproject.tomlfor unified project configuration, dependency management, and build system..python-version(now set to 3.10).Improvements
uvtool for consistent Python environment management.Bug Fixes
docker-composeanddocker composecommands in integration tests.Style
Chores
requirements.txt,requirements-dev.txt,setup.py,.isort.cfg,.codespellrc)..gitignoreto exclude.venv/and stop ignoring.python-version.AUTHORSfile.