Skip to content

apollo_deployments: assert every config param is set in each service's app configs - #15083

Merged
matanl-starkware merged 1 commit into
main-v0.14.4from
matanl/deployment-config-schema-coverage-test
Sep 1, 2026
Merged

apollo_deployments: assert every config param is set in each service's app configs#15083
matanl-starkware merged 1 commit into
main-v0.14.4from
matanl/deployment-config-schema-coverage-test

Conversation

@matanl-starkware

@matanl-starkware matanl-starkware commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Stacked on #15082 — rebase onto main-v0.14.4 once it merges. Fails without it.

The node loads with ignore_default_values, so a param missing from a service's app configs is either a startup failure or a silent default. The test asserts each service sets every schema param, minus secrets, pointer targets and disabled subtrees.

TODO at deployment_definitions_test.rs:46 stays open — value-level validation. It wouldn't have caught this: central_sync_client_config.#is_none defaults to true.

@cursor

cursor Bot commented Sep 1, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
Test-only changes in deployment definitions; no runtime deployment or config loading behavior is modified.

Overview
Adds all_config_params_are_set_in_service_configs, which walks every node service’s mounted app config files and fails if any non-private schema param with an explicit value is missing (accounting for optional subtrees disabled via #is_none).

The duplicate-key test is refactored to share application_config_files and config_file_params helpers so both tests read deployment-listed JSON the same way as a pod would.

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

@reviewable-StarkWare

Copy link
Copy Markdown

This change is Reviewable

@matanl-starkware
matanl-starkware force-pushed the matanl/require-explicit-batch-config branch from c04294e to a86c5b5 Compare September 1, 2026 06:53
@matanl-starkware
matanl-starkware force-pushed the matanl/deployment-config-schema-coverage-test branch 2 times, most recently from 899c0ab to b36bf75 Compare September 1, 2026 06:54
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

Artifacts upload workflows:

Base automatically changed from matanl/require-explicit-batch-config to main-v0.14.4 September 1, 2026 07:30
@matanl-starkware
matanl-starkware force-pushed the matanl/deployment-config-schema-coverage-test branch from b36bf75 to a337af0 Compare September 1, 2026 07:31

@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.

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 a337af0. Configure here.

Comment thread crates/apollo_deployments/src/deployment_definitions_test.rs Outdated

@yoavGrs yoavGrs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@yoavGrs made 2 comments.
Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on matanl-starkware).


crates/apollo_deployments/src/deployment_definitions_test.rs line 105 at r1 (raw file):

/// Test that every config param a service needs is set explicitly in its application config
/// files: the node loads with `ignore_default_values`, so an absent param either fails

Where does the test set ignore_default_values=true?

Code quote:

ignore_default_values

crates/apollo_deployments/src/deployment_definitions_test.rs line 132 at r1 (raw file):

    for node_type in NodeType::iter() {
        for node_service in node_type.all_service_names() {
            let deployment_file = File::open(node_service.replacer_deployment_file_path()).unwrap();

Can you reuse the production's loading code?

…s app configs

The node loads with ignore_default_values, so a param missing from a service's app
configs is either a startup failure or a silent default. Nothing caught that:
duplicate_config_entries opens the same files but only checks duplicates,
deployment_files_are_up_to_date compares presets against themselves, and the hybrid
system test runs the one overlay where the missing subtree is disabled.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@matanl-starkware
matanl-starkware force-pushed the matanl/deployment-config-schema-coverage-test branch from a337af0 to 97a1b57 Compare September 1, 2026 08:45
@matanl-starkware

Copy link
Copy Markdown
Collaborator Author

Both addressed in 97a1b57.

line 105 — the test doesn't set it; the deployed pod does. Reworded to name the call site: SequencerNodeConfig::load_and_process (node_config.rs:484) passes ignore_default_values = true.

line 132 — partially. The full loader can't run on these files: the replacer app configs hold $$$_..._$$$ placeholders and "" required-param markers that only get real values from the env overlay at deploy time — the same wall as the TODO at line 46. And the merge step, update_config_map_by_custom_configs (apollo_config/src/loading.rs:198), is pub(crate).

What I reused instead: the file list comes from production's own replacer_deployment_file_path() — the same configList cdk8s mounts — and the read/merge is now extracted into helpers shared with duplicate_config_entries, which was doing the same thing by hand.

If you'd rather have real reuse of the merge step I can make update_config_map_by_custom_configs pub and build the test on it. WDYT?

@matanl-starkware

Copy link
Copy Markdown
Collaborator Author

Followed up in #15085, stacked on this one.

Making update_config_map_by_custom_configs pub turned out to be the wrong route — update_config_map type-checks every value, and 116 values across the app configs are $$$_..._$$$ placeholders that would fail it with ChangeRequiredParamType. Its signature also takes clap::parser::Values<PathBuf>, so it isn't callable from outside the arg parser as it stands.

Stubbing the placeholders first turned out to unlock the whole entry point instead: substitute a replacer placeholder with the schema default and the six pointer targets with a well-formed dummy, and all 19 layout/service configs load through SequencerNodeConfig::load_and_process — the same call a pod makes, no visibility changes anywhere.

That catches stale params (UnexpectedParam), wrong value types and optional-pruning errors, none of which the coverage test sees. Full validation stays out of reach: validate_node_config_without_urls fails on components.* for every service, since component urls and ports come from the env overlay rather than the schema. That half of the TODO at line 46 stays open.

@matanl-starkware matanl-starkware left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@matanl-starkware made 2 comments.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on yoavGrs).


crates/apollo_deployments/src/deployment_definitions_test.rs line 105 at r1 (raw file):

Previously, yoavGrs wrote…

Where does the test set ignore_default_values=true?

Fixed the comment. The caller site passes this param.


crates/apollo_deployments/src/deployment_definitions_test.rs line 132 at r1 (raw file):

Previously, yoavGrs wrote…

Can you reuse the production's loading code?

Can't reuse it directly — update_config_map type-checks, and 116 app-config values are $$$_..._$$$ placeholders that fail it. Stubbed those instead and used the full loader: PR 15085

@yoavGrs yoavGrs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@yoavGrs reviewed 1 file and all commit messages, and resolved 2 discussions.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on matanl-starkware).

@matanl-starkware
matanl-starkware added this pull request to the merge queue Sep 1, 2026
Merged via the queue into main-v0.14.4 with commit 0ee373a Sep 1, 2026
28 checks passed
@matanl-starkware
matanl-starkware deleted the matanl/deployment-config-schema-coverage-test branch September 3, 2026 14:41
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.

3 participants