RFC: Split KeboolaClient into a transport and resource namespaces - #595
Draft
martinsifra wants to merge 1 commit into
Draft
RFC: Split KeboolaClient into a transport and resource namespaces#595martinsifra wants to merge 1 commit into
martinsifra wants to merge 1 commit into
Conversation
martinsifra
added a commit
that referenced
this pull request
Aug 19, 2026
Implements docs/merge-requests-layer3-rfc.md (decisions D1-D10 there; this
message covers what shapes the code):
client/merge_requests.py -- the nine MR endpoints as a namespace,
client.merge_requests.{list,get,conflicts,create,update,request_review,
approve,request_changes,merge}. The namespace depends on a StorageRequester
Protocol, not on the client; a temporary _ClientRequester adapter satisfies
it until the client-split RFC (draft #595) builds a real transport under the
seam (D10). Two invariants deliberately break the surrounding idioms and are
called out in docstrings: paths are NEVER branch-prefixed (every MR endpoint
is project-level), and bodies are JSON with real types (the backend asserts
branchFromId as int; form-encoded values stay strings and fail validation).
_optional_mr_fields keeps create/update from drifting and is keyword-only:
four of its five parameters are str | None, so a positional transposition
would type-check cleanly and surface only as a backend 422.
merge() awaits the Storage job implicitly like every job-backed method in
client/, with a dedicated MERGE_JOB_MAX_WAIT (600 s) budget -- merging a
many-config branch can outlive the default 60 s. It does NOT re-check the
returned job: raising on a failed job (fast-fail included) is the poller's
contract since #603, stated as a requirement on the Protocol so a future
transport cannot reintroduce the blind spot. The await covers the merge
outcome only; the source-branch deletion runs as a second, unhandled job.
client/configs.py -- get_config_diff + rebase_config/rebase_config_delete.
branch_id is required with no production fallback (the endpoints 400 on the
default branch, D5). Keep and delete rebases are separate methods so no
illegal combination is expressible (D6). The keep rebase requires the FULL
replaced body (name, rows, configuration, is_disabled, description): /rebase
replaces rather than patches, so an omitted key takes the server-side
default -- a caller sending only name+rows would wipe the configuration and
re-enable a disabled config, then merge that into production.
constants.py -- MERGE_JOB_MAX_WAIT and FEATURE_BRANCHES_MERGE_REQUESTS.
Layer 3 does no feature check itself (a missing feature is a 403 identical
to a role denial); Part 2's service pre-flights with the constant (D9).
tests/test_merge_request_client.py pins the wire contract: bare vs
branch-prefixed paths, JSON types, presence detection, the diff envelope and
the {} delete resolution, merge-job waiting and its 600 s budget, the
replaced-body requirement, include=activityLog, and the stub-requester seam.
Part 2 (service + commands) follows separately; no CLI command is added
here, so no E2E / docs surfaces change yet.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
martinsifra
added a commit
that referenced
this pull request
Aug 19, 2026
client/merge_requests.py -- the nine MR endpoints as a namespace,
client.merge_requests.{list,get,conflicts,create,update,request_review,
approve,request_changes,merge}. The namespace depends on a StorageRequester
Protocol, not on the client; a temporary _ClientRequester adapter satisfies
it until the client-split work (draft #595) builds a real transport under
the seam. Two invariants deliberately break the surrounding idioms and are
called out in docstrings: paths are NEVER branch-prefixed (every MR endpoint
is project-level), and bodies are JSON with real types (the backend asserts
branchFromId as int; form-encoded values stay strings and fail validation).
_optional_mr_fields keeps create/update from drifting and is keyword-only:
four of its five parameters are str | None, so a positional transposition
would type-check cleanly and surface only as a backend 422.
merge() awaits the Storage job implicitly like every job-backed method in
client/, with a dedicated MERGE_JOB_MAX_WAIT (600 s) budget -- merging a
many-config branch can outlive the default 60 s. It does NOT re-check the
returned job: raising on a failed job (fast-fail included) is the poller's
contract since #603, stated as a requirement on the Protocol so a future
transport cannot reintroduce the blind spot. The await covers the merge
outcome only; the source-branch deletion runs as a second, unhandled job.
client/configs.py -- get_config_diff + rebase_config/rebase_config_delete.
branch_id is required with no production fallback (the endpoints 400 on the
default branch). Keep and delete rebases are separate methods so no illegal
combination is expressible. The keep rebase requires the FULL replaced body
(name, rows, configuration, is_disabled, description): /rebase replaces
rather than patches, so an omitted key takes the server-side default -- a
caller sending only name+rows would wipe the configuration and re-enable a
disabled config, then merge that into production.
constants.py -- MERGE_JOB_MAX_WAIT and FEATURE_BRANCHES_MERGE_REQUESTS.
Layer 3 does no feature check itself (a missing feature is a 403 identical
to a role denial); Part 2's service pre-flights with the constant.
tests/test_merge_request_client.py pins the wire contract: bare vs
branch-prefixed paths, JSON types, presence detection, the diff envelope and
the {} delete resolution, merge-job waiting and its 600 s budget, the
replaced-body requirement, include=activityLog, and the stub-requester seam.
Part 2 (service + commands) follows separately; no CLI command is added
here, so no E2E / docs surfaces change yet.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
martinsifra
added a commit
that referenced
this pull request
Aug 19, 2026
client/merge_requests.py -- the nine MR endpoints as a namespace,
client.merge_requests.{list,get,conflicts,create,update,request_review,
approve,request_changes,merge}. The namespace depends on a StorageRequester
Protocol, not on the client; a temporary _ClientRequester adapter satisfies
it until the client-split work (draft #595) builds a real transport under
the seam. Two invariants deliberately break the surrounding idioms and are
called out in docstrings: paths are NEVER branch-prefixed (every MR endpoint
is project-level), and bodies are JSON with real types (the backend asserts
branchFromId as int; form-encoded values stay strings and fail validation).
_optional_mr_fields keeps create/update from drifting and is keyword-only:
four of its five parameters are str | None, so a positional transposition
would type-check cleanly and surface only as a backend 422.
merge() awaits the Storage job implicitly like every job-backed method in
client/, with a dedicated MERGE_JOB_MAX_WAIT (600 s) budget -- merging a
many-config branch can outlive the default 60 s. It does NOT re-check the
returned job: raising on a failed job (fast-fail included) is the poller's
contract since #603, stated as a requirement on the Protocol so a future
transport cannot reintroduce the blind spot. The await covers the merge
outcome only; the source-branch deletion runs as a second, unhandled job.
client/configs.py -- get_config_diff + rebase_config/rebase_config_delete.
branch_id is required with no production fallback (the endpoints 400 on the
default branch). Keep and delete rebases are separate methods so no illegal
combination is expressible. The keep rebase requires the FULL replaced body
(name, rows, configuration, is_disabled, description): /rebase replaces
rather than patches, so an omitted key takes the server-side default -- a
caller sending only name+rows would wipe the configuration and re-enable a
disabled config, then merge that into production. rebase_config is
keyword-only after the ids: name/description/change_description are
same-typed neighbours, so a positional transposition would type-check
cleanly and silently land review text inside the replaced body.
constants.py -- MERGE_JOB_MAX_WAIT and FEATURE_BRANCHES_MERGE_REQUESTS.
Layer 3 does no feature check itself (a missing feature is a 403 identical
to a role denial); Part 2's service pre-flights with the constant.
tests/test_merge_request_client.py pins the wire contract: bare vs
branch-prefixed paths, JSON types, presence detection, the diff envelope and
the {} delete resolution, merge-job waiting and its 600 s budget, the
replaced-body requirement (incl. the keyword-only signature),
include=activityLog, and the stub-requester seam.
Part 2 (service + commands) follows separately; no CLI command is added
here, so no E2E / docs surfaces change yet.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
martinsifra
added a commit
that referenced
this pull request
Aug 19, 2026
client/merge_requests.py -- the nine MR endpoints as a namespace,
client.merge_requests.{list,get,conflicts,create,update,request_review,
approve,request_changes,merge}. The namespace depends on a StorageRequester
Protocol, not on the client; a temporary _ClientRequester adapter satisfies
it until the client-split work (draft #595) builds a real transport under
the seam. Two invariants deliberately break the surrounding idioms and are
called out in docstrings: paths are NEVER branch-prefixed (every MR endpoint
is project-level), and bodies are JSON with real types (the backend asserts
branchFromId as int; form-encoded values stay strings and fail validation).
_optional_mr_fields keeps create/update from drifting and is keyword-only:
four of its five parameters are str | None, so a positional transposition
would type-check cleanly and surface only as a backend 422.
merge() awaits the Storage job implicitly like every job-backed method in
client/, with a dedicated MERGE_JOB_MAX_WAIT (600 s) budget -- merging a
many-config branch can outlive the default 60 s. It does NOT re-check the
returned job: raising on a failed job (fast-fail included) is the poller's
contract since #603, stated as a requirement on the Protocol so a future
transport cannot reintroduce the blind spot. The await covers the merge
outcome only; the source-branch deletion runs as a second, unhandled job.
client/configs.py -- get_config_diff + rebase_config/rebase_config_delete.
branch_id is required with no production fallback (the endpoints 400 on the
default branch). Keep and delete rebases are separate methods so no illegal
combination is expressible. The keep rebase requires the FULL replaced body
(name, rows, configuration, is_disabled, description): /rebase replaces
rather than patches, so an omitted key takes the server-side default -- a
caller sending only name+rows would wipe the configuration and re-enable a
disabled config, then merge that into production.
constants.py -- MERGE_JOB_MAX_WAIT and FEATURE_BRANCHES_MERGE_REQUESTS.
Layer 3 does no feature check itself (a missing feature is a 403 identical
to a role denial); Part 2's service pre-flights with the constant.
tests/test_merge_request_client.py pins the wire contract: bare vs
branch-prefixed paths, JSON types, presence detection, the diff envelope and
the {} delete resolution, merge-job waiting and its 600 s budget, the
replaced-body requirement, include=activityLog, and the stub-requester seam.
Part 2 (service + commands) follows separately; no CLI command is added
here, so no E2E / docs surfaces change yet.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
martinsifra
added a commit
that referenced
this pull request
Aug 19, 2026
client/merge_requests.py -- the nine MR endpoints as a namespace,
client.merge_requests.{list,get,conflicts,create,update,request_review,
approve,request_changes,merge}. The namespace depends on a StorageRequester
Protocol, not on the client; a temporary _ClientRequester adapter satisfies
it until the client-split work (draft #595) builds a real transport under
the seam. Two invariants deliberately break the surrounding idioms and are
called out in docstrings: paths are NEVER branch-prefixed (every MR endpoint
is project-level), and bodies are JSON with real types (the backend asserts
branchFromId as int; form-encoded values stay strings and fail validation).
_optional_mr_fields keeps create/update from drifting and is keyword-only:
four of its five parameters are str | None, so a positional transposition
would type-check cleanly and surface only as a backend 422.
merge() awaits the Storage job implicitly like every job-backed method in
client/, with a dedicated MERGE_JOB_MAX_WAIT (600 s) budget -- merging a
many-config branch can outlive the default 60 s. It does NOT re-check the
returned job: raising on a failed job (fast-fail included) is the poller's
contract since #603, stated as a requirement on the Protocol so a future
transport cannot reintroduce the blind spot. The await covers the merge
outcome only; the source-branch deletion runs as a second, unhandled job.
client/configs.py -- get_config_diff + rebase_config/rebase_config_delete.
branch_id is required with no production fallback (the endpoints 400 on the
default branch). Keep and delete rebases are separate methods so no illegal
combination is expressible. The keep rebase requires the FULL replaced body
(name, rows, configuration, is_disabled, description): /rebase replaces
rather than patches, so an omitted key takes the server-side default -- a
caller sending only name+rows would wipe the configuration and re-enable a
disabled config, then merge that into production.
constants.py -- MERGE_JOB_MAX_WAIT and FEATURE_BRANCHES_MERGE_REQUESTS.
Layer 3 does no feature check itself (a missing feature is a 403 identical
to a role denial); Part 2's service pre-flights with the constant.
tests/test_merge_request_client.py pins the wire contract: bare vs
branch-prefixed paths, JSON types, presence detection, the diff envelope and
the {} delete resolution, merge-job waiting and its 600 s budget, the
replaced-body requirement, include=activityLog, and the stub-requester seam.
Part 2 (service + commands) follows separately; no CLI command is added
here, so no E2E / docs surfaces change yet.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
martinsifra
added a commit
that referenced
this pull request
Aug 19, 2026
client/merge_requests.py -- the nine MR endpoints as a namespace,
client.merge_requests.{list,get,conflicts,create,update,request_review,
approve,request_changes,merge}. The namespace depends on a StorageRequester
Protocol, not on the client; a temporary _ClientRequester adapter satisfies
it until the client-split work (draft #595) builds a real transport under
the seam. Two invariants deliberately break the surrounding idioms and are
called out in docstrings: paths are NEVER branch-prefixed (every MR endpoint
is project-level), and bodies are JSON with real types (the backend asserts
branchFromId as int; form-encoded values stay strings and fail validation).
_optional_mr_fields keeps create/update from drifting and is keyword-only:
four of its five parameters are str | None, so a positional transposition
would type-check cleanly and surface only as a backend 422.
merge() awaits the Storage job implicitly like every job-backed method in
client/, with a dedicated MERGE_JOB_MAX_WAIT (600 s) budget -- merging a
many-config branch can outlive the default 60 s. It does NOT re-check the
returned job: raising on a failed job (fast-fail included) is the poller's
contract since #603, stated as a requirement on the Protocol so a future
transport cannot reintroduce the blind spot. The await covers the merge
outcome only; the source-branch deletion runs as a second, unhandled job.
client/configs.py -- get_config_diff + rebase_config/rebase_config_delete.
branch_id is required with no production fallback (the endpoints 400 on the
default branch). Keep and delete rebases are separate methods so no illegal
combination is expressible. The keep rebase requires the FULL replaced body
(name, rows, configuration, is_disabled, description): /rebase replaces
rather than patches, so an omitted key takes the server-side default -- a
caller sending only name+rows would wipe the configuration and re-enable a
disabled config, then merge that into production.
constants.py -- MERGE_JOB_MAX_WAIT and FEATURE_BRANCHES_MERGE_REQUESTS.
Layer 3 does no feature check itself (a missing feature is a 403 identical
to a role denial); Part 2's service pre-flights with the constant.
tests/test_merge_request_client.py pins the wire contract: bare vs
branch-prefixed paths, JSON types, presence detection, the diff envelope and
the {} delete resolution, merge-job waiting and its 600 s budget, the
replaced-body requirement, include=activityLog, and the stub-requester seam.
Part 2 (service + commands) follows separately; no CLI command is added
here, so no E2E / docs surfaces change yet.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
What
Design document only — no code. Adds
docs/client-split-rfc.md: the problem statement and requirements for splitting the 109-methodKeboolaClientGod class into:client.tables.*,client.merge_requests.*, …) that receive the requester at construction,following the architecture of PyGithub (
Requester) and stripe-python (_APIRequestor).Status
Draft — problem statement, requirements (R1–R6), target-model sketch, non-goals, and open design questions. The investigation-backed design section lands in follow-up commits on this PR; the current state reflects the intake interview of 2026-08-17.
Key requirements captured:
services/+sync/) migrate family-by-family; flat aliases survive only where an external consumer exists (Client.raw).KeboolaClientonly for now; the other sixBaseHttpClientsubclasses need a wider team discussion first.docs/merge-requests-rfc.md, branchms/dmd-1833) ship first on a small adapter seam and migrate onto this architecture at near-zero cost.lib.py) is untouched; no wire-behavior change anywhere.No Linear issue exists for this work — this PR is the anchor.
🤖 Generated with Claude Code