deployment: add native node-config builder and wire it into the ConfigMap#14609
deployment: add native node-config builder and wire it into the ConfigMap#14609nimrod-starkware wants to merge 1 commit into
Conversation
9e2747d to
88fff14
Compare
e8502f3 to
8967b72
Compare
88fff14 to
8e7c47b
Compare
8967b72 to
98a58a8
Compare
nimrod-starkware
left a comment
There was a problem hiding this comment.
@nimrod-starkware made 9 comments.
Reviewable status: 0 of 8 files reviewed, 9 unresolved discussions.
deployments/sequencer/src/config/native.py line 69 at r1 (raw file):
Returns a new dict that is fully independent of `base` (deep-copied once, up front); neither input is mutated. """
Suggestion:
"""
Recursively deep-merge `overlay` onto `base`, preserving explicit `null` values.
"""deployments/sequencer/src/config/native.py line 81 at r1 (raw file):
it is free to write into it. Two dicts merge key-by-key; any non-dict overlay value (including explicit `null`) replaces the target value outright. `overlay` is only read, never mutated. """
Suggestion:
"""
Recursively merge `overlay` into `target`, MUTATING `target` in place.
"""deployments/sequencer/src/config/native.py line 97 at r1 (raw file):
`cp -rf` cannot clobber one with the other across repos). They are merged in sorted order; layer files in the same dir must have disjoint leaf keys, so the order is immaterial (see `LAYER_GLOB`). """
Suggestion:
"""
Returns all *sequencer_config.jsonnet` override-layer file in `layer_dir`, in sorted order.
"""deployments/sequencer/src/config/native.py line 153 at r1 (raw file):
wins). Shared ancestors across multiple overlays are deduped, so the output is identical whether the caller passes only the leaf overlay or the leaf plus its explicit ancestors. """
Suggestion:
"""
Resolves the ordered list of existing `*sequencer_config.jsonnet` layer files.
"""deployments/sequencer/src/config/native.py line 231 at r1 (raw file):
Matches the Rust eval form (`crates/apollo_deployments/src/jsonnet.rs`): `build` is a method on the imported object (no extVar / TLA). The overrides object is injected inline as a JSON literal. """
Suggestion:
"""
Evaluates `(import 'lib/build.libsonnet').build(<layout>, <overrides>)` and return its JSON.
"""deployments/sequencer/src/constructs/configmap.py line 19 at r1 (raw file):
layout: str, overlays: list[str], config_format: str = "preset",
Suggestion:
config_format: str,deployments/sequencer/test/test_native_config.py line 24 at r1 (raw file):
assert merged == {"a": {"x": 1, "y": None}, "b": "keep"} # The null is a real value, present in the result. assert "y" in merged["a"] and merged["a"]["y"] is None
Suggestion:
def test_deep_merge_preserves_explicit_null():
"""A later layer setting a key to null overwrites an earlier non-null value (not skipped)."""
base = {"a": {"x": 1, "y": 2}, "b": "keep"}
overlay = {"a": {"y": None}}
merged = deep_merge_preserving_null(base, overlay)
assert merged == {"a": {"x": 1, "y": None}, "b": "keep"}
assert base == base (not changed)
assert overlay == overlaydeployments/sequencer/test/test_native_config.py line 47 at r1 (raw file):
deep_merge_preserving_null(base, overlay) assert base == {"a": {"x": 1}} assert overlay == {"a": {"y": 2}}
delete
deployments/sequencer/test/test_native_config.py line 56 at r1 (raw file):
@pytest.mark.parametrize("name", ["core", "gateway", "l1", "mempool", "committer"]) def test_service_name_to_build_key_identity(name): assert service_name_to_build_key(name) == name
delete
nimrod-starkware
left a comment
There was a problem hiding this comment.
@nimrod-starkware made 1 comment.
Reviewable status: 0 of 8 files reviewed, 10 unresolved discussions.
deployments/sequencer/src/config/native.py line 47 at r1 (raw file):
# Maps an overlay/deployment service name to the build key used by the jsonnet layout # (`lib/layouts/hybrid.libsonnet` `services::`). Only `sierracompiler` differs from the build key; # every other service name is identity.
clean these comments
Code quote:
# Repo root: this file is deployments/sequencer/src/config/native.py, root is 4 levels up.
REPO_ROOT = Path(__file__).resolve().parents[4]
# JPATH for the jsonnet evaluator, matching `crates/apollo_deployments/src/jsonnet.rs` (JSONNET_DIR).
# `import 'lib/build.libsonnet'` and build's internal relative imports resolve under this dir.
JSONNET_DIR = REPO_ROOT / "crates" / "apollo_deployments" / "jsonnet"
# Glob matching every override-layer file in an overlay layer directory. The pattern requires the
# `sequencer_config.jsonnet` suffix but allows a name prefix, so a directory may carry more than one
# layer file (e.g. the public repo's applicative `sequencer_config.jsonnet` and the devops-owned
# `common_sequencer_config.jsonnet`), authored with distinct names so the deploy's
# `cp -rf devops/overlays -> public/configs/overlays` (a file-level replace) cannot clobber one with
# the other. All matches in a dir are deep-merged in sorted order; layer files sharing a dir MUST
# have disjoint leaf keys (enforced by the cross-repo collision test) so the merge order is immaterial.
LAYER_GLOB = "*sequencer_config.jsonnet"
# Maps an overlay/deployment service name to the build key used by the jsonnet layout
# (`lib/layouts/hybrid.libsonnet` `services::`). Only `sierracompiler` differs from the build key;
# every other service name is identity.
nimrod-starkware
left a comment
There was a problem hiding this comment.
@nimrod-starkware resolved 7 discussions.
Reviewable status: 0 of 8 files reviewed, 3 unresolved discussions (waiting on dan-starkware).
nimrod-starkware
left a comment
There was a problem hiding this comment.
@nimrod-starkware resolved 3 discussions.
Reviewable status: 0 of 8 files reviewed, all discussions resolved (waiting on dan-starkware).
98a58a8 to
528af92
Compare
8e7c47b to
1a279e2
Compare
PR SummaryMedium Risk Overview Overlay directory resolution is factored into Unit tests cover null-preserving deep-merge only; full parity / overlay jsonnet data tests are deferred to a follow-up. Reviewed by Cursor Bugbot for commit e625740. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1a279e2. Configure here.
1a279e2 to
21f7e07
Compare
528af92 to
9b6dcf2
Compare
01a785b to
cd104c8
Compare
9b6dcf2 to
8032897
Compare
8032897 to
509c630
Compare
cd104c8 to
4aeaa5c
Compare
…gMap
Add src/config/native.py: it assembles the nested SequencerNodeConfig for a
service by deep-merging (null-preserving) the per-layer sequencer_config.jsonnet
override files and evaluating jsonnet build('<layout>', overrides), then selecting
the service's section. ConfigMapConstruct selects this native path when
config_format == "native"; preset remains the default. Add the jsonnet (_jsonnet)
Pipfile dependency and unit tests for the deep-merge and the overlay-service-name
-> build-key mapping. The override-layer data and the parity/mirror tests that
exercise the full build land in a follow-up.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
4aeaa5c to
e625740
Compare


Add src/config/native.py: it assembles the nested SequencerNodeConfig for a
service by deep-merging (null-preserving) the per-layer sequencer_config.jsonnet
override files and evaluating jsonnet build('', overrides), then selecting
the service's section. ConfigMapConstruct selects this native path when
config_format == "native"; preset remains the default. Add the jsonnet (_jsonnet)
Pipfile dependency and unit tests for the deep-merge and the overlay-service-name
-> build-key mapping. The override-layer data and the parity/mirror tests that
exercise the full build land in a follow-up.
Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com