refactor(sync): split sync_service.py -- extract bindings + clone + models (tech-debt) - #437
Merged
Conversation
…y (tech-debt) `sync_service.py` had grown to ~4033 lines -- ~2.7x the 1500-LOC ceiling for `services/*.py` (CONTRIBUTING.md calls this file out by name). #432 added ~321 LOC without splitting; this is the deferred split. No behavior change -- pure mechanical extraction, public `SyncService` API identical: - `_sync_models.py`: the 5 dataclasses (WritebackResult, CreatedConfig, VariableBindingResult, FlowBindingResult, LocalConfigHashes) + the VARIABLES/FLOW component-id constants. `sync_service` re-exports `CreatedConfig` (the only externally-imported name) for back-compat. - `_sync_bindings.py`: the push-time link backfill -- Phase C (variable links) and Phase D (keboola.flow task configIds) -- as free functions taking the `SyncService` as first arg (for the 3 on-disk helpers they need). push() now calls `resolve_variable_bindings(self, ...)` / `resolve_flow_task_bindings(self, ...)`. - `_sync_clone.py`: the `clone_project` orchestration. `SyncService.clone_project` is now a thin delegator (public method preserved). `sync_service.py`: 4033 -> 3435 lines (-598). The free-function-taking-service pattern keeps typing explicit (ty-clean via a TYPE_CHECKING `SyncService` import) and makes the bindings/clone unit-testable in isolation. Further extractions (writeback, storage-metadata/jobs/samples) are natural follow-ups in the same pattern. make check green (4092 passed).
…h-debt) Continues the sync_service.py split. No behavior change. - `_sync_writeback.py`: manifest + local-file writeback after a push (writeback_create_config_in_manifest, writeback_create_row_in_manifest, propagate_kbc_metadata, writeback_after_push). Only writeback_after_push needs the service (for _write_config_file); the rest are pure. - `_sync_storage.py`: pull-side data writers (write_storage_metadata, fetch_jobs_per_config, write_per_config_jobs, fetch_samples, mask_encrypted_columns) + the `_ensure_path_within` storage-write traversal guard moved here with its only callers. Only fetch_jobs_per_config needs the service (for _resolve_max_workers). push()/pull() call the free functions; tests updated to the relocated symbols (test_sync_service.py writeback calls, test_sync_storage_jobs.py storage calls). `sync_service.py`: 3435 -> 2931 lines. Cumulative over this PR: 4033 -> 2931 (-1102, ~27%), now across 5 cohesive modules (_sync_models, _sync_bindings, _sync_clone, _sync_writeback, _sync_storage). make check green (4092 passed, identical to pre-split).
…h-debt) Continues the sync_service.py split. No behavior change. - `_sync_bulk.py`: the all-projects orchestrators (pull_all, diff_all, push_all) as free functions taking the service; the public methods are now thin delegators. - `_sync_branch.py`: git-branch <-> Keboola-dev-branch linking (branch_link, branch_unlink, branch_status). branch_link takes the service; the others are pure. `get_current_branch` is imported LOCALLY inside each function (as the original did) so tests that patch `keboola_agent_cli.sync.git_utils. get_current_branch` keep working -- a top-level import would capture the reference at import time and defeat source-module patching. Public methods preserved as shims (commands + tests call them unchanged). `sync_service.py`: 2581 lines (was 2931 this commit; 4033 at the PR base). make check green (4092 passed).
Continues the sync_service.py split. No behavior change. - `_sync_push_ops.py`: the per-change create/update/delete operations for configs and rows (push_create, push_update, push_row_change + the row create/update/delete helpers) as free functions taking the service. push() calls push_create(self, ...) / push_update(self, ...) / push_row_change(self, ...). The 2 direct test calls to _push_update_row were updated to the relocated push_update_row. `sync_service.py`: 2244 lines. Cumulative over this PR: 4033 -> 2244 (-1789, ~44%), across 8 cohesive modules (_sync_models, _sync_bindings, _sync_clone, _sync_writeback, _sync_storage, _sync_bulk, _sync_branch, _sync_push_ops). The remainder is the core orchestration (init/pull/diff/push/status) + shared branch-path/manifest helpers. make check green (4092 passed).
padak
force-pushed
the
refactor/split-sync-service
branch
from
June 17, 2026 07:54
80850c1 to
12b9f83
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tech-debt split of the oversized
sync_service.py, deferred from #432.Why
sync_service.pyreached ~4033 lines -- ~2.7x the 1500-LOC hard ceiling forservices/*.pyin CONTRIBUTING.md "Code Quality Patterns", which names this file as the known extreme case and says the next PR adding material should split first. #432 added ~321 LOC (push Phase-D flow remap +clone_project) without splitting; this is that split.What (no behavior change -- public
SyncServiceAPI identical)Eight new cohesive modules;
sync_service.py: 4033 -> 2244 lines (-1789, ~44%)._sync_models.pyVARIABLES/FLOWcomponent-id constants (re-exportsCreatedConfig)_sync_bindings.pykeboola.flowtask configId remap)_sync_clone.pyclone_projectorchestration_sync_writeback.py_sync_storage.py_ensure_path_within_sync_bulk.py_sync_branch.py_sync_push_ops.pyThe remainder (2244 lines) is the genuine core:
init_sync/pull/diff/push/statusorchestration + the shared branch-path / manifest / conflict-detection helpers. Splitting those too would turnSyncServiceinto a near-empty delegating shell -- left as a judgement-call follow-up.Approach
Free functions taking the
SyncServiceas first arg (not mixins): keeps typing explicit and ty-clean (aTYPE_CHECKINGSyncServiceimport types theserviceparam) and makes each group unit-testable in isolation. Most extracted functions are pure; only those needing an on-disk helper (_read_config_file/_write_config_file/_compute_config_hashes/_file_hash/_resolve_source_branch_path/_resolve_max_workers) takeservice. Public methods (clone_project,pull_all/diff_all/push_all,branch_link/branch_unlink/branch_status) are preserved as thin delegators so commands + tests are unchanged. A handful of direct test calls to relocated private helpers were updated.Patch-point fidelity:
_sync_branchimportsget_current_branchLOCALLY inside each function (as the original did) so tests thatpatch("keboola_agent_cli.sync.git_utils.get_current_branch")keep working -- a module-top import would capture the reference at import time and defeat source-module patching.Verification
make checkgreen: 4092 passed, 8 skipped -- identical to pre-split, so behavior is unchanged. ty clean on all new modules.