Repository navigation
feat(mdcode): deploy the deployments that --deployment names, after every check - #554
Conversation
7bf16f0 to
d9e9f60
Compare
d9e9f60 to
2d4504b
Compare
mmotoyama
left a comment
There was a problem hiding this comment.
AI only found one issue, so LGTM if the finding is valid and addressed!
| } | ||
|
|
||
| return {kcModels, fileCopies, overviews}; | ||
| const plans: DeploymentPlan[] = []; |
There was a problem hiding this comment.
AI found only one medium urgency issue:
When a profile is selected by --deployment, validateProfileCompleteness skips several checks on that profile—including physical column-name checks on keys and joins, field-expression reference checks, and profile-introduced hierarchy errors—so they are not reported twice once deploymentPlan validates the applied model.
However, checkBeforeWrites returns early whenever profileErrors or kcErrors is non-empty (see line 1872), before deploymentPlan runs. In addition, the deploymentPlan loop returns on the first failing deployment rather than collecting errors across all selected deployments. As a result, whenever a push has any error in profileErrors, kcErrors, or an earlier deployment plan, the checks that were deferred to deploymentPlan are silently dropped instead of being reported in the same run.
Here is a test case illustrating the problem:
test(
'reports deferred profile errors alongside other profile errors when a deployment selects that profile',
async () => {
// Setup: A model with a 'bq' deployment using profile 'bq'. In 'profiles/bq.yaml',
// entity 'Customer' has a non-physical primary_key ('UPPER(c_key)') and entity
// 'Order' is missing its 'source'.
const model = `
semantic_model:
- name: retail
deployments:
- name: bq
target: bigquery/project/proj/dataset/ds
profile: bq
entities:
- name: Customer
primary_key: [c_key]
fields:
- name: c_key
expression:
dialects:
- dialect: ANSI_SQL
expression: c_key
- name: Order
primary_key: [o_key]
fields:
- name: o_key
expression:
dialects:
- dialect: ANSI_SQL
expression: o_key
`;
const bqProfile = `
name: bq
entities:
- name: Customer
source: proj.ds.customers
primary_key: [UPPER(c_key)]
- name: Order
primary_key: [o_key]
`;
writeWorkspace(
tempDir, {'models/retail.yaml': model},
{'profiles/bq.yaml': bqProfile});
// Current behavior on this PR: checkBeforeWrites passes applied=true for
// profile 'bq' to namedProfileErrors (which skips the physical key column check
// on Customer so deploymentPlan will report it), but then returns early on
// profileErrors before deploymentPlan runs—dropping the Customer error.
// Expected behavior: Both the missing source on 'Order' and the non-physical
// key column 'UPPER(c_key)' on 'Customer' are reported in the same run, just as
// they are when --no-deployment is used.
const out = await runPush(['--deployment', 'bq']);
expect(out.code).toBe(1);
expect(out.errors).toEqual([
expect.stringContaining(
`[profiles/bq.yaml] profile 'bq': entity 'Order' has no source in this profile`),
expect.stringContaining(
`'UPPER(c_key)' is not a physical column name`),
]);
});There was a problem hiding this comment.
You're right. With --deployment, the profile check skipped the key, join, field expression and hierarchy rules for each selected deployment's profile and left them to that deployment's own checks. Those checks run only after every other local check passes, so these errors showed up on the next run instead of the same one. Push wrote nothing in either case.
I changed the profile check to apply every rule to every profile, as --no-deployment already does. It now leaves only one thing to a selected deployment's checks: the warning about SQL kcmd cannot read, which those checks give themselves. With a valid graph target, your example now prints both errors in one run, and I added a case like it to the two-phase path tests.
The deployment loop still stops at the first deployment that fails, as the live checks after it do.
The warning about SQL kcmd cannot read still prints only when those checks run, so a push that fails earlier shows it on a later run.
…ther errors A push with --deployment left the key, join, expression and hierarchy checks of each selected deployment's profile to that deployment's own checks. Those run only after every other local check passes, and stop at the first deployment that fails. A push that failed an earlier check reported these errors only on the next run. The profile check now gives every error of every profile, as it does under --no-deployment. It leaves to a selected deployment's checks only the warning about SQL in its profile that kcmd cannot read.
kcmd push --deployment <names>publishes the model to Knowledge Catalog, thendeploys each named deployment in the order given, with the profile that
deployment names. Names are comma-separated, or the flag repeats. Every check of
every destination runs before the first write.
After the checks of the
--no-deploymentpath, the first phase builds eachselected deployment's model in
deploymentPlan:refused, since a Spanner graph cannot hold metrics.
applyProfile. A profile file isread again first, with its actions.
remove the field they are about.
deploymentsis narrowed to the deployment's own entry, which keeps eachdeployment to its own target. The model is then validated and its DDL
generated offline.
After the file-copy type lookup and the catalog preflight,
liveErrorsrunseach deployment's checks that call a service. It reads the BigQuery dataset
through
datasetLocation, which the graph deploy already uses, or the Spannerdatabase through the new
SpannerClient.getDatabase. It probes each BigQuerysource, dry-runs the BigQuery DDL in the dataset's location and checks each
action's DML. When the dataset's location cannot be read, push prints that
warning if a check fails or under
--validate-only. A push that deploys printsit with the graph, as today. The second phase writes Knowledge Catalog, then
deploys each deployment, under its own header when there are several. An
AlloyDB deployment deploys no graph and prints a note.
Push refuses
--deploymentwith--no-deploymentin either order, a bare orempty
--deployment, a name the model lacks and--deploymentin a scope withno model.
typedDeploymentreads both flags from the command line as typed,since cac turns a value that looks like a number into a number and drops
--deploymentwhen--no-deploymentfollows it.Today's path accepts and refuses what it did before. Its refusal of a graph push
of a named profile no longer says that a deployment cannot name a profile yet.
pruningNote,printGraphandgenerateOfflinemove out of it, and itsoutput does not change. Comments in runtime.ts, store.ts and skills.ts no
longer say that push refuses several deployments, since
--deploymentdeploysseveral.
namedProfileErrorsnow takes a callback that says what push's own checks cover for each profile. Today's path leaves the profile it pushes to its own checks, as before. A--deploymentpush checks every profile in full, and leaves to each selected deployment's checks only the warning about SQL in its profile that kcmd cannot read. It also takes the profile whose join push's own keycheck reports, which the profile consistency check leaves out. The two-phase
path passes
default, and today's path the profile it pushes.This version differs from the finished stack in these ways:
deploys to, or hold a deployment that names a profile to the metric rules
of its engine. A Spanner deployment whose profile leaves a metric available
deploys its graph without that metric. A BigQuery deployment whose profile
leaves a metric that is not a single SUM, AVG, COUNT, MIN or MAX deploys
its graph without that measure. The generator warns about each metric it
leaves out. Push does not yet warn that
defaultis unbound when everydeployment names a profile. A later PR in this stack adds these checks.
--deploymentdoes not yet apply catalog.yaml'sdefault_profilethe way the runtime does, or warn that it chooses nobinding. A later PR in this stack adds both.
kcmd profilesstill reports the errors of a plain push, until a later PRin this stack runs every model that lists deployments in two phases.
generator cannot build are tested in a later PR in this stack, beside the
report that prints them. That PR also tests that a deployment's error prints
once.
through two phases, the test harness passes every deployment's name to
--deploymentwhen a case with a Google-flavor model gives none. So thetwo-phase tests already state what the final plain push does.
Point to check: Spanner cannot dry-run DDL, so a Spanner deployment whose DDL
Spanner rejects fails in the second phase, after Knowledge Catalog is written.
reference.md says so.