Skip to content

apollo_config,apollo_node_config,apollo_deployments,deployment: jsonnet-native node config - #15096

Open
itamar-starkware wants to merge 1 commit into
itamar/ci-pin-remaining-actionsfrom
itamar/jsonnet-stack-squash
Open

apollo_config,apollo_node_config,apollo_deployments,deployment: jsonnet-native node config#15096
itamar-starkware wants to merge 1 commit into
itamar/ci-pin-remaining-actionsfrom
itamar/jsonnet-stack-squash

Conversation

@itamar-starkware

Copy link
Copy Markdown
Contributor

No description provided.

@cursor

cursor Bot commented Sep 3, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
This changes how every node loads and merges configuration at startup; mistakes in file layout, secret keys, or Jsonnet output could misconfigure or partially apply secrets across many components.

Overview
Moves node configuration off the old flattened dump / SerializeConfig / per-flag CLI model to nested JSON configs plus flat dotted-key secret overlays, aligned with Jsonnet-generated deployments.

apollo_config drops dumping (pointers, #is_none flags, ser_param, generated defaults in dumps). Startup CLI now only accepts --config_file (repeatable); values come from files, not env/CLI per path. Loading expects two config files: nested base, then secrets merged by dotted paths (skips overrides under null optionals; can add missing leaves when the parent exists). Config presentation/redaction takes an explicit private path set instead of metadata baked into dumps. Duration fields gain matching serialize_with helpers for round-trip JSON.

Component config crates shed SerializeConfig impls and often drop unused apollo_config deps; a few comments/docs note fields mirrored or validated at the node level (e.g. validation_only).

apollo_deployments evaluates Jsonnet via jrsonnet-evaluator / jrsonnet-stdlib (replacing prior compile-time config machinery in that crate). Workspace Cargo.lock picks up jrsonnet and routine transitive bumps.

CI: hybrid CDK8s synth no longer passes the hybrid.common.dummy_for_testing overlay.

Reviewed by Cursor Bugbot for commit dfa82ea. Bugbot is set up for automated code reviews on this repo. Configure here.

@reviewable-StarkWare

Copy link
Copy Markdown

This change is Reviewable

@itamar-starkware
itamar-starkware marked this pull request as ready for review September 3, 2026 13:44
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown

Artifacts upload workflows:

itamar-starkware commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

// `SequencerNodeConfig::default()` does not have internally consistent pointer-group values
// (those are only reconciled by pointer resolution at load time), so normalize them before
// exercising the validation_only logic this test targets.
normalize_pointer_groups(&mut config);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test clears validation only flag

Low Severity

validation_only_with_tx_ingestion_disabled_succeeds sets validation_only: true, then calls normalize_pointer_groups, which forces the node-level and batcher validation_only fields to false. The test therefore never runs validate_validation_only_config in the mode it claims to cover, so regressions in validation-only boot checks could pass CI unnoticed.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 068fbf9. Configure here.

@itamar-starkware
itamar-starkware changed the base branch from main to graphite-base/15096 September 3, 2026 16:14
@itamar-starkware
itamar-starkware force-pushed the itamar/jsonnet-stack-squash branch from 068fbf9 to dfa82ea Compare September 3, 2026 16:14
@itamar-starkware
itamar-starkware changed the base branch from graphite-base/15096 to itamar/ci-pin-remaining-actions September 3, 2026 16:14

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

Fix All in Cursor

❌ 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 dfa82ea. Configure here.

pub fn dump_config_file_changes(&self) {
self.base_app_config.dump_config_file(&self.node_config_path);
self.base_app_config.dump_native_config_file(&self.node_config_path);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified configs fail secret reload

High Severity

modify_config rewrites only the nested base file. Custom Sensitive deserializers such as deserialize_optional_sensitive_map accept a string, but serde emits null for None. After a later change that introduces a new secret field (for example enabling central_sync_client_config), the stale secrets overlay has no override and native load fails.

Additional Locations (2)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit dfa82ea. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants