Skip to content

fix(config): a profile's [apps.<name>] table merges into the app definitions, so an overlay can retune an app and not only the gateway - #9052

Open
MusabMahmoodh wants to merge 2 commits into
jaseci-labs:mainfrom
MusabMahmoodh:fix/profile-app-overlay
Open

fix(config): a profile's [apps.<name>] table merges into the app definitions, so an overlay can retune an app and not only the gateway#9052
MusabMahmoodh wants to merge 2 commits into
jaseci-labs:mainfrom
MusabMahmoodh:fix/profile-app-overlay

Conversation

@MusabMahmoodh

Copy link
Copy Markdown
Contributor

Closes #8995.

What this changes

merge_from_toml_file (which reads jac.<profile>.toml and jac.local.toml) and apply_profile (which reads [environments.<name>]) each handle project, check, storage, the capability sections, dependencies, environments and scripts, and had no apps branch at all. So a profile's [apps.worker_sv.scale] was parsed and thrown away, while [scale.gateway] three lines below it in the same file was applied, because scale is a capability section and gets deep-merged.

reference/apps.md documents the profile as layer 3 and the app overlay as layer 2, later layers winning, so the profile's table is supposed to override the base one. That asymmetry is what the report describes.

Both paths now call one new JacConfig.merge_apps_table, which deep-merges the overlay's [apps] onto _base_data's and re-parses the result through parse_apps. Two consequences worth stating, because they are why this is one function rather than two merge sites:

  • the merged table gets the same validation the base one gets (unknown keys, missing kind, absolute paths, route collisions), instead of a second, laxer path into config.apps;
  • config.apps stays derived from one table. Readers needed no change: app_scale_overrides and fleet_from_config both already resolve config.apps[name].overlays.

The re-parse runs _interpolate_recursive over the merged table before parse_apps, mirroring the load path (_parse_toml_data stores raw in _base_data and parses the interpolated copy). Without it, _base_data being the un-interpolated store means a base app's ${VAR} would come back literal the moment any overlay declared [apps] - the fix would have introduced that. There is a test for it.

_deep_merge replaces lists rather than concatenating them, so an overlay that sets [apps.<n>.scale] exempt_paths restates the whole list. That is the existing behaviour for every other section, not new here.

Behavior change

An overlay that declares [apps] against a jac.toml with no [apps] table is now rejected:

an overlay declares [apps] but jac.toml does not; declare every app in
jac.toml and let the overlay retune it

Previously it was silently ignored, which is the same class of silence as the bug itself. A single-app project that had a stray [apps] in jac.local.toml will now fail loud instead of quietly not applying it.

Tests

One new test each for the two properties, both in scale/tests/apps/test_app_scale_overrides.jac - the file whose docstring already claims this surface - reusing its _project() helper and reset_scale_config discipline. No new file, no mocks: real jac.toml and jac.e2e.toml on disk, read through the real JAC_PROFILE path.

Both fail on unmodified main:

assert orders["replicas"] == 1 , orders;
E   AssertionError: {'rpc_timeout': 120.0, 'replicas': 2, 'cpu_limit': '2000m', ...}
assert cfg.apps["orders"].overlays["scale"]["replicas"] == 1
E   KeyError: 'scale'

and with only the _interpolate_recursive call removed and the rest of the fix in place, the second one fails on the half it exists to protect:

assert cfg.apps["orders"].route == "/orders"
E   AssertionError: /${ORDERS_ROUTE_SEG}

Validation

Run on a Linux x86_64 dev build from this checkout (jac dev mode - using compiler source at ~/jaseci-w2/jac confirmed in each run), rebased onto 9b02a4b2d9:

suite result
jac/tests/project/ (owns JacConfig) 323 passed
jac/jaclang/scale/tests/apps/ 27 passed
jac/jaclang/scale/tests/misc/ 206 passed

Pre-fix runs: 25 passed / 1 failed / 1 error with both source files reverted, and 26 passed / 1 failed with only the interpolation call removed.

Not run: jac/jaclang/scale/tests/deploy/, the k8s e2e lanes, and any real cluster deploy. This changes config parsing only and touches no manifest or runtime path, but a deploy that reads config.apps under a profile is the case I did not exercise on a cluster. CI covers the deploy suite.

…nitions, so an overlay can retune an app and not only the gateway

merge_from_toml_file and apply_profile handled project, check, storage, the
capability sections, dependencies and scripts, and had no branch for apps.
A jac.<profile>.toml or jac.local.toml carrying [apps.<name>.scale] was read
and discarded, while [scale.gateway] three lines below it in the same file
was applied, because scale is a capability section and gets deep-merged.
reference/apps.md documents the profile as layer 3 and the app overlay as
layer 2 with later layers winning, so the profile's table should override
the base one.

Both paths now deep-merge the overlay's [apps] table onto _base_data's and
re-parse through parse_apps, so the merged table gets the same validation
the base one does (unknown keys, kinds, route collisions) and config.apps
stays derived from one table rather than from two merge implementations.
An overlay that declares [apps] against a non-workspace jac.toml is
rejected rather than silently starting a workspace.

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

Reviewed 9f5980cc27bd82daec3fc747658f301b9b54cdc7, including the full diff, both profile merge paths, app parsing/validation, and effective per-app configuration. No blocking findings.

Independent macOS validation using this checkout's compiler source: all 27 tests in jac/jaclang/scale/tests/apps passed. Two additional probes also passed: inherited [environments] app settings retain base fields, and jac.local.toml app settings take precedence over the selected file profile. git diff --check passes. I did not run a real cluster deployment.

The shared merge helper preserves the existing nested settings and reuses parse_apps validation; re-interpolating the raw base app table also preserves the base route scenario covered by the new regression test. The follow-up 9f5980cc2 commit only adds the release note. No fixes were needed or made during review.

I rechecked the remote head: unchanged, non-draft, and conflict-free. Current-head CI and pre-commit.ci are green, including the Scale deploy and Kubernetes checks. Status: approved and ready to merge from this review.

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.

A profile's [apps.<name>.<section>] overlay is dropped, while [scale.gateway] in the same profile is applied

2 participants