Skip to content

feat(mdcode): push every model that lists deployments or has a sibling profile file in two phases, and report its errors - #563

Open
feixufeixu wants to merge 1 commit into
mainfrom
feat/push/tpp-3-two-phase-every-model
Open

feixufeixu wants to merge 1 commit into
mainfrom
feat/push/tpp-3-two-phase-every-model

Conversation

@feixufeixu

Copy link
Copy Markdown
Collaborator

A plain kcmd push now takes the two-phase path for a model that lists
deployments:, in the Google flavor or in a vanilla GOOGLE block, and for a
model that has a sibling profile file, <model>.profile.<name>.yaml, so
Knowledge Catalog receives every profile. A bare --profile takes two phases
too, since cac gives it the value of an absent flag. For a model with no
deployments list, --profile <name>, --all-profiles and --no-profile keep
today's path, and so does a model whose only profile files are in
<model>.profiles/. Push no longer calls deploymentProfileError, which
refused a deployment that names a profile, and loader.ts loses it.

On this path default_profile chooses no binding, but the runtime reads it.
So defaultProfileCheck applies it with its actions, as the runtime does, and
runs push's own action checks on it. A legacy wrapper must merge and load. Push
warns when default_profile is set and no deployment uses it, or when a
selected deployment names no profile.

For a model that lists deployments, kcmd profiles runs checkBeforeWrites
and prints the errors it returns, so it stops where a plain push stops. With
KC_V2_ASPECTS=1, it prints once each catalog warning push gives, such as one
about a custom ai_context member push leaves out of the catalog's model.
b/570200480 makes kcmd keep the author's text for those values, which removes
the warning, so this PR only keeps the warning visible. The report also gives
the error for an aspect too large to store, under the model. Push refuses that
aspect in its catalog preflight, before any write. Each error is a LocalCheckError that
belongs to the whole push, to a model or to one of its profiles. A deployment's
errors go under the profile it uses, which is default when it names none.
Push and the report print each distinct error once. When push gives no error, a
profile that does not apply gets a not shown: line that says why.

A plain push of either kind of model now differs from today's path:

  1. Push refuses --no-kc. For a model that lists deployments it also refuses
    --profile, --all-profiles and --no-profile. The help text of the four
    flags says so.
  2. Knowledge Catalog receives the model file as written, with every profile
    attached, before the graphs deploy. A Spanner graph that fails to deploy
    now leaves the catalog written.
  3. A deployment that names a profile deploys with that profile. Two graph
    deployments both deploy. An AlloyDB deployment gets a note and deploys no
    graph.
  4. A Spanner deployment that names no profile, in a model with metrics, is
    refused. Today's path deployed it.
  5. With default_profile naming a legacy wrapper, today's path deployed the
    wrapper's table. This PR deploys the model file's table and warns.

Until the next PR:

  1. Push accepts a Spanner or AlloyDB deployment whose tables are in another
    database, as store_agreement's 'a deployment for a declared profile, under
    default' case expects. A BigQuery deployment that reads a Spanner table
    passes the local checks and fails the DDL dry run.
  2. A deployment that names a profile is not held to its engine's metric rules.
    The generator warns about each metric they would refuse and leaves it out.
  3. Push does not warn that default is unbound when every deployment names a
    profile.
  4. Main's call to validateDeployments in modelFileBindingErrors stays,
    though no model that lists deployments reaches it.

Main's tests that pushed a model that lists deployments with --no-kc,
--profile, --all-profiles or --no-profile now use a plain push or
--no-deployment, expect the refusal, or are removed where another test covers
the code. A test about today's path drops deployments:, or moves its target
into a legacy wrapper's deployment_target: or a GOOGLE block's
deploymentTargets. Tests of the refusal of a deployment that names a profile
now expect the two-phase result, or are removed. Main's pruning-note table
becomes one two-phase test of its source-check row. Its two KC_V2_ASPECTS
rows become one today's-path test that deploys through a legacy wrapper's
deployment_target:. Where a profiles.test.ts
case needs today's path for a model with a sibling profile file, it adds
--profile default. The other cases take two phases and pass with no change to
what they expect. The exception is the aspect size case, which now expects its
error under the model. The AlloyDB error on today's path drops "push uses exactly
one deployment for now". Main's --transpile test of a vendor-only metric now
reads the model from the catalog preflight, since its model lists deployments
and pushes in two phases.

Points to check:

  1. takesTwoPhasePath. Only a value other than true counts as passing
    --profile, and localProfileFiles gives a profile name only to a sibling
    profile file. listsDeployments reads the places the loader reads: the
    native key, and each GOOGLE block whose data is a string. Like the
    loader, it matches GOOGLE in any letter case.
  2. In profiles(), the !twoPhase guards keep main's report for a scope where
    no model lists deployments. With --profile <name>, the report prints the
    other profiles' errors above the profiles, since any of them stops the push.

…g profile file in two phases, and report its errors
@feixufeixu
feixufeixu requested a review from merr1988 October 24, 2026 20:37
@feixufeixu
feixufeixu added this pull request to stack #565 October 24, 2026 20:38
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.

1 participant