Repository navigation
Conversation
kcmd init wrote no snapshot.aspects, so pull fetched only the aspects an entry's type requires. A knowledge base document's text lives in the overview aspect, so every document pulled with an empty body, and an author writing into it would replace text they never saw. Each scope now names the aspects init lists by default (CatalogSource.defaultAspects): the overview aspect for a knowledge base, an entry group and a BigQuery dataset, and none for a semantic model, which pulls by its own path. No system-generated aspect is listed, since push sends every local aspect back. Rerunning init on the same scope still keeps the existing catalog.yaml. The overview aspect name now lives once, in metadata.ts, for the documents layout and the scopes. Bug: b/576454554
6efafb0 to
0e422a8
Compare
feixufeixu
left a comment
There was a problem hiding this comment.
Approving with one required fix. A pull and a push with no edit now turn a knowledge base document's HTML overview into MARKDOWN. My comments on documents.ts and init_kb_pull_overview.yaml give the fix and a test. The other two comments are optional.
| entry.aspects[OVERVIEW_ASPECT_KEY].content = bodyTrimmed; | ||
| entry.aspects[OVERVIEW_ASPECT_KEY].contentType = 'MARKDOWN'; | ||
| entry.aspects[md.OVERVIEW_ASPECT].content = bodyTrimmed; | ||
| entry.aspects[md.OVERVIEW_ASPECT].contentType = 'MARKDOWN'; |
There was a problem hiding this comment.
Required. Pull drops contentType on line 110, and push sets MARKDOWN here, so a pull and a push turn an HTML overview into MARKDOWN. Now that init lists the overview, the first push after a pull does this to every HTML document in the knowledge base, because push sends every document. I'd keep any other content type in the front matter:
| entry.aspects[md.OVERVIEW_ASPECT].contentType = 'MARKDOWN'; | |
| entry.aspects[md.OVERVIEW_ASPECT].contentType ??= 'MARKDOWN'; |
On line 110, change delete aspect.contentType; to if (aspect.contentType === 'MARKDOWN') delete aspect.contentType;. A MARKDOWN document's file stays as it is today. My suggestion on init_kb_pull_overview.yaml adds a test for this.
| contentType: MARKDOWN | ||
|
|
||
| init: | ||
| kb: t.u.g | ||
|
|
||
| actions: | ||
| - action: pull | ||
|
|
||
| assert: | ||
| fileSystem: | ||
| catalog/doc.md: | ||
| contains: The text an author wrote in Knowledge Catalog. |
There was a problem hiding this comment.
Required, the test for my comment on documents.ts. With this document in HTML, a push and a second pull show whether the content type survives. The ---\n prefix also makes the first check match only when the text is the body.
| contentType: MARKDOWN | |
| init: | |
| kb: t.u.g | |
| actions: | |
| - action: pull | |
| assert: | |
| fileSystem: | |
| catalog/doc.md: | |
| contains: The text an author wrote in Knowledge Catalog. | |
| contentType: HTML | |
| init: | |
| kb: t.u.g | |
| actions: | |
| - action: pull | |
| - action: push | |
| - action: pull | |
| assert: | |
| fileSystem: | |
| catalog/doc.md: | |
| - contains: "---\nThe text an author wrote in Knowledge Catalog." | |
| - contains: "contentType: HTML" |
I'd also change line 2 to description: A knowledge base pulled right after init holds each document's overview text as its body, since init lists the overview aspect, and a push with no edit keeps its content type.
| // A built-in aspect type every Dataplex project can read, and one of the | ||
| // aspects `kcmd init` lists by default. | ||
| this.addMockAspectType( | ||
| {name: 'projects/dataplex-types/locations/global/aspectTypes/overview'} as gcp.AspectType); |
There was a problem hiding this comment.
Optional. Now that the mock registers the overview type for every test, the aspectTypes entries for it in push_kb.yaml lines 9-10, push_bq.yaml lines 14-15 and manifest_snapshot.yaml lines 7-8 have no effect. I'd delete those six lines.
| // The aspect types `kcmd init` lists under snapshot.aspects, so pull fetches | ||
| // them along with those each entry's type requires. System-generated aspects | ||
| // are left out, since push sends every local aspect back. |
There was a problem hiding this comment.
Optional. README.md lines 157-159 say pull fetches the aspects each entry's type requires, and that other aspects go under snapshot.aspects. They don't say init now lists the overview there. I'd add these two lines after line 159:
# kcmd init lists dataplex-types.global.overview under snapshot.aspects for
# knowledge base, entry group and BigQuery dataset scopes.
Part 1 of b/576454554.
Problem
kcmd initwrote nosnapshot.aspects, sokcmd pullfetched only the aspects an entry's type requires. A knowledge base document's text is itsoverviewaspect, so every document pulled with an empty body.Change
CatalogSource.defaultAspects: the aspect typesinitlists undersnapshot.aspectsfor each scope.dataplex-types.global.overview.CatalogManifest.initWith*builds the manifest through oneforNewSourcehelper that applies the list.initon the same scope still keeps the existingcatalog.yaml.metadata.ts, used by the documents layout and the scopes.Tests
init_kb,init_eg,init_bqdsscenarios check the default list incatalog.yaml.init_kb_pull_overviewscenario:init, thenpull, gives a document whose body is its overview text (the bug's repro).overviewaspect type, as every Dataplex project does.init_kbandinit_kb_pull_overviewfail.tscclean; scenarios 23, layouts 2, gcp 11, semantic 2068, tool 395, all pass.Follow-up
Part 2:
pullwarns once per aspect type it leaves out (excluding BigQuery system-generated aspects), forcatalog.yamlfiles written before this change.