diff --git a/.agents/rules/dresources.md b/.agents/rules/dresources.md index 351b19d172a..8bffe309df7 100644 --- a/.agents/rules/dresources.md +++ b/.agents/rules/dresources.md @@ -6,3 +6,5 @@ paths: --- **RULE: Before implementing or modifying a resource, read `bundle/direct/dresources/README.md`.** It covers constraints, field classification, update mask rules, async patterns, testing requirements, and state compatibility. + +**RULE: When adding a new resource type, follow `.agents/skills/author-new-resource/SKILL.md` and run its API readiness gate before writing code.** Composite list keys, `OverrideChangeDesc` in a new resource, hand-written rules that mirror missing proto annotations, and engine changes to accommodate an API shape are API problems; raise them upstream instead of encoding them here. diff --git a/.agents/skills/author-new-resource/SKILL.md b/.agents/skills/author-new-resource/SKILL.md new file mode 100644 index 00000000000..a54ad30c0dd --- /dev/null +++ b/.agents/skills/author-new-resource/SKILL.md @@ -0,0 +1,170 @@ +--- +name: author-new-resource +description: Add a new resource type to Declarative Automation Bundles (resources. in databricks.yml) on the direct deployment engine, for engineers who own an API but do not know the CLI codebase. Starts with an API readiness gate that sends API-shape problems back to the proto, then walks the wiring, generators, tests and PR in fixed phases with a verification command at each step. Use when asked to add, onboard or support a new bundle resource, or when an API team asks what DABs needs from their API. +--- + +# Author a new bundle resource + +One resource per PR, roughly 250 to 400 hand-written Go lines, an adapter of 60 to 130 lines with +no plan overrides, a fake of 70 to 100 lines, three to five acceptance directories. That is what the +last several resources looked like and what review expects. If your resource needs more, the API +is usually the reason, and the API is where to fix it. + +Work in phases. Each phase ends with a check. Do not start the next phase until the check passes. + +References in this directory: `references/api-readiness.md` (phase 0), `references/wiring.md` +(phases 2 and 5), `references/testing.md` (phase 4), `references/pitfalls.md` (read once before +phase 2, and again before phase 6). Repo rules under `.agents/rules/` and +`bundle/direct/dresources/README.md` apply as well; where this skill and the README disagree, the +README is the contract and this skill is the path through it. + +## Phase 0: API readiness gate + +Before writing code, run every check in `references/api-readiness.md` against your proto (or the +pinned `.codegen/cli.json` if you have no universe checkout). Write the verdict table into your +notes: one line per check, STOP, ASK or NOTE, with the field or RPC concerned. + +- Any STOP: send the note from the reference to your API owners and stop here. Do not build a + composite key, a plan override, or an engine change to compensate. Those will be rejected in + review and cost more than the proto change. +- Any ASK: file the proto change now, before the PR, and carry the smallest commented bridge the + reference allows. +- All NOTE: continue. + +Check: the verdict table exists and has no STOP. + +## Phase 1: establish the contract + +Decide and write down, in the PR description draft: + +1. **Names.** Bundle plural and singular (`resources.`, snake_case, matching the API's + collection name), Go type, SDK service and types: the resource message, the create request, the + get response. Confirm the SDK in `go.mod` has them: `grep -n " struct" $(go env GOMODCACHE)/github.com/databricks/databricks-sdk-go@*/service//model.go`. + If the SDK lacks the API, that is a separate SDK bump PR first; see `.agents/skills/bump-sdk`. + Embed the message GET returns in the config type, so state and remote share one type and + `RemapState` is not needed; server-only fields on it are covered by the generated output-only + rules. Two exceptions: the create request carries inputs the resource message lacks, or the + resource message marks a server-assigned field `REQUIRED` (it would become a required config + key). In both cases embed the create request, pin the resource type in `apitypes.yml` so the + generated rules still describe the read shape, and for the second case ASK for `OUTPUT_ONLY` + upstream. +2. **Identity.** Which field or fields the engine stores as the id, how `DoRead` gets from id to a + Get request, and whether changing an identity field recreates (`provided_id_fields`) or renames + (`updatable_id_fields`). +3. **Mutability.** Which fields update in place, which are immutable (should already be annotated + after phase 0), whether the update mask is `*` or a documented list, whether there is an update + at all. +4. **Asynchrony.** Does create, update or delete return an operation; is the id available before + completion; is the resource readable right after create. +5. **URL.** Does the workspace UI have a page for one instance; what is its path. +6. **Launch stage** and therefore PyDABs eligibility (GA and public preview only). +7. **Exemplar.** Pick the existing resource closest to your shape and copy its files, do not write + from scratch. Current good choices on `main`: + - server-assigned id, plain CRUD: `instance_pool.go` in `bundle/direct/dresources/` and + `bundle/config/resources/`, fake `libs/testserver/instance_pools.go`; + - full-name identity derived from parent fields: `feature.go` (three-line rules file); + - parent plus leaf AIP name: `mcp_service.go` with `configs/mcp_services.yml`; + - composed id from two user fields, no UI page: `data_quality_monitor.go` with + `configs/data_quality_monitors.yml`; + - asynchronous create with the id available at once: `database_instance.go` (returns the id + from the create response and polls in `WaitAfterCreate`). + If one of these does not exist at your base commit, take the newest resource that passes the + phase 2 rules: no `OverrideChangeDesc`, no inline wait loop, a static or `*` mask, a fake under + 150 lines. Never copy an adapter because it is recent; copy it because it is clean. + +Check: the seven items are written down and the exemplar's files are open side by side. + +## Phase 2: wire the resource + +Follow `references/wiring.md` sections 1 and 2 in order: config type, registrations, adapter, +`all.go`, `all_test.go` fixture, `configs/.yml` with only the rules the proto cannot express +(each with a one-line comment naming the API behaviour), fake server from section 4. + +Rules that review enforces on the adapter: + +- One SDK call per `DoCreate`, `DoUpdate`, `DoDelete`. Return SDK errors unchanged; the engine adds + the resource key and id. +- Asynchronous waits go in `WaitAfterCreate`, `WaitAfterUpdate`, `WaitAfterDelete`, so state is + persisted before polling. +- `RemapState` is a field copy; any remapping lives in `DoRead`. +- No nil checks on arguments the framework guarantees. `DoCreate` must return a non-empty id; if + the API can complete without one, return an error. +- No `OverrideChangeDesc`. If you believe you need it, go back to phase 0. +- Update mask `*` when the API accepts it, otherwise a static list with a comment. + +Check: + +```bash +go build ./... && go test ./bundle/config -count=1 +``` + +The adapter tests need the generated rules from phase 3 (input-only fields fail `TestAll` until +then), so they are part of the phase 3 check. + +## Phase 3: generate + +Run the generators in the order given in `references/wiring.md` section 3, fill the +`annotations.yml` placeholders for your type, run `./task generate-schema` again, then +`./task generate-check`. Read `configs/.generated.yml` back against the API docs (section 7 +of the wiring reference); anything you had to hand-write in `configs/.yml` that should have +been generated is an upstream annotation to request, if phase 0 did not already. + +Check: + +```bash +go test ./bundle/direct/dresources -run 'TestAll/^$|TestResourceConfig|TestConfigFiles|TestResourcesYML|TestInputSubset|TestRemoteSuperset|TestNameStableOrOptedOut|TestNoUpdateResourcesCoverAllFields' -v -count=1 +``` + +plus: `./task generate-check` is clean, `grep -c PLACEHOLDER bundle/internal/schema/annotations.yml` +did not grow, `acceptance/bundle/refschema/output/.txt` exists (the first +`generate-refschema` run reports the new file as unexpected; run it again), and +`python/databricks/bundles//` exists exactly when the resource's launch stage makes it +eligible (GA or public preview). A preview resource that produced a Python package needs the +launch-stage override from the readiness reference, then a regeneration that removes the package. + +## Phase 4: tests + +Follow `references/testing.md`. Add the canonical acceptance set (`basic`, `lifecycle` with +`Cloud = true`, `remote-delete`, `deployment/bind/`, the invariant template and its matrix +entry, the PyDABs support directory when eligible) and extend the completeness tests the wiring +reference lists. Copy the exemplar's directories and adapt them; keep `title` lines to one line and +let `print_requests.py`, `replace_ids.py` and `musterr` do the work. + +Then prove the tests ran, with the commands in testing reference section 4, and run the `lifecycle` +test against a real workspace (section 6). Record which environment. + +Check: every proof command lists your tests; `./task test` passes locally; the cloud run passed. + +## Phase 5: bundle-wide decisions + +Go through `references/wiring.md` section 5 and make each decision explicitly: run_as, presets, +permissions, UC dependencies, approval prompts in both deploy and destroy, workspace URL, state +fixture, deployment-history allowlist or exclusion. Write each decision and its reason into the PR +description. "Not applicable" is a valid decision when you say why. + +Check: the completeness tests named in that section pass; the PR description lists the decisions. + +## Phase 6: the PR + +- Scope: this resource and nothing else. No changes to other resources, no new config syntax, no + engine changes, and no changes to shared `libs/` beyond the fake server in `libs/testserver` and + the URL pattern in `libs/workspaceurls`. If you found something worth fixing elsewhere, open a + separate PR. +- Changelog fragment `.nextchanges/bundles/.md`, one line. +- Run `.agents/skills/pr-checklist`. +- Description, three sections from the PR template: + - Changes: the resource, its identity and update shape in two sentences, the decisions from + phase 5, the API changes requested in phase 0 and their status. + - Why: one sentence. + - Tests: the acceptance directories, which ran on cloud and where, the proof that the invariant + config ran. +- Read `references/pitfalls.md` once more against your diff. + +Check: the diff touches only the files in the wiring reference plus your acceptance directories, and +the hand-written Go line count is in the expected range. If it is not, say why in the description. + +## When the API changes under you + +If the proto changes while the PR is open (an annotation you asked for lands, or a field moves), +bump the SDK in a separate PR, rerun phase 3, and delete the hand-written rule the annotation now +covers. `TestResourcesYMLNoRedundantRules` fails until you do. diff --git a/.agents/skills/author-new-resource/references/api-readiness.md b/.agents/skills/author-new-resource/references/api-readiness.md new file mode 100644 index 00000000000..7a4ed0a3884 --- /dev/null +++ b/.agents/skills/author-new-resource/references/api-readiness.md @@ -0,0 +1,340 @@ +# API readiness gate + +Run this before writing any CLI code. The bundle engine derives most of its plan behaviour from +the API's proto annotations, so a gap in the API becomes hand-written resource code, and a gap in +the API's shape becomes engine workarounds. Both are expensive to carry and neither is accepted in +a new-resource PR. Fix the API first; the API is usually still in preview when this question comes +up and the proto is the cheapest place to change. + +Each check ends in one of three verdicts: + +- **STOP**: do not start the CLI work until the API changes. Send the note in section 4. +- **ASK**: file the API change, continue in parallel, and carry a small, commented hand-written rule + until the annotation lands. Say so in the PR. +- **NOTE**: record the decision in the PR description. No API change needed. + +## 1. How the CLI consumes the API + +- Proto `google.api.field_behavior` values reach `.codegen/cli.json` as `behaviors` on each field: + `IMMUTABLE`, `INPUT_ONLY`, `OPTIONAL`, `OUTPUT_ONLY`, `UNORDERED_LIST`. `REQUIRED` and + `IDENTIFIER` are not carried, so identity has to be read from the proto itself. +- `./task generate-direct` turns those into `bundle/direct/dresources/configs/.generated.yml`: + `OUTPUT_ONLY` and `INPUT_ONLY` become `ignore_remote_changes` (reasons `spec:output_only`, + `spec:input_only`); `IMMUTABLE` becomes `recreate_on_changes` (reason `spec:immutable`). A + behaviour on a container field applies to every child. +- Only fields that are part of the resource's state (listed in + `acceptance/bundle/refschema/output/.txt` as `STATE` or `ALL`) get rules. +- The generator picks the API type for a resource by field overlap and writes it to + `apitypes.generated.yml`. When the heuristic picks a request type instead of the resource type, + pin the right one in `apitypes.yml` with a one-line reason. +- Everything else is hand-written in `configs/.yml`: `provided_id_fields`, + `updatable_id_fields`, `stable_output_fields`, `backend_defaults`, `ignore_local_changes`. Each + hand-written rule is a fact about the API the proto could not express. Keep them few, and give + each a comment that names the API behaviour it stands in for. + +## 2. Checks + +### 2.1 Identity + +**Check.** The resource has one stable identifier the server returns on GET and accepts on GET, +Update and Delete. Best: a `name` field marked `IDENTIFIER` (AIP-122, AIP-203). If the client picks +the ID, the Create request carries `_id` and the entity's own ID field is `OUTPUT_ONLY` +(AIP-133). + +**Verify.** In the proto: `rg -n "IDENTIFIER|identifier: true|identifier_field_names" `; +identity may be declared on the field or in the file-level resource option +(`databricks.api.resource` with `identifier_field_names`). Read the Get request's path parameters. +The identifier is not visible in `cli.json`. + +**Verdicts.** +- One field, returned on GET: **NOTE**. Declare it in `provided_id_fields` (recreate when the user + changes it) or `updatable_id_fields` (rename in place, needs `DoUpdateWithID`). +- Parent plus leaf id on the Create request, with the resource carrying only the server-derived + `name` (the AIP-133 shape): **NOTE**. The config type wraps `parent` and `_id` around + the embedded resource, both go in `provided_id_fields`, and the engine id is the returned `name`. +- An id the client may set or leave for the server to assign: **NOTE**, treat it as user-provided + (`provided_id_fields`); if the user later sets a different value the resource recreates. +- Two or more fields together identify the resource (for example an object type plus an object id + in the URL): **ASK** for a single `name`, and in the meantime compose the engine ID from the + components with a small pair of helpers (`ID(...)`, `ParseID(...)`) on the + config type and list every component in `provided_id_fields`. This pattern exists in the repo and + needs no engine change. +- No field the client can predict and the server returns: **STOP**. + +### 2.2 Repeated fields of resource-like elements + +**Check.** Every `repeated` message field whose elements have their own lifecycle (they are created, +updated or deleted individually, carry server-assigned ids, or appear in other RPCs) is either keyed +by one client-settable field per element, or expressed as a list of resource names (AIP-144, +AIP-124). Lists of plain settings (tags, column names, notification addresses) need no key. + +**Verify.** For each `repeated` field in the resource message, open the element type. Does it have a +field the user sets that is unique within the list and echoed on GET? Is the list's order +preserved by GET? The API docs often say "order is not significant"; that is a key requirement, not +an excuse. + +**Why.** The engine pairs list elements by a key (`KeyedSlices`) and then looks elements up by +`field=value`. The key must be a real field present in config, state and the GET response. A key +composed from several fields, or a value the client cannot set, cannot be looked up, and the +resource then needs plan overrides that reimplement engine behaviour. + +**Verdicts.** +- Keyed by one user-set field, or a list of names: **NOTE**. Declare it in `KeyedSlices`. +- Elements with a lifecycle but no single key, order not preserved: **STOP**. Ask for a + client-settable key field on the element (the pattern of `task_key` on job tasks), or for the + list to become resource names. Do not build a composite key. Do not propose an engine change. +- Elements that only exist as part of this resource and have no identity at all (pure config): + **NOTE**, positional diff is fine. + +### 2.3 Inputs round-trip + +**Check.** Every field the client sends on Create or Update is returned by GET with the same value, +or is annotated `INPUT_ONLY` with a reason (write-only secrets are the legitimate case). + +**Verify.** Compare the Create body with the GET response in the proto or API docs. In `cli.json` +look for `INPUT_ONLY`. Quote the annotation line for every field you classify; a doc comment that +says "not returned" or "input only" on a field annotated `OPTIONAL` is an ASK, not a NOTE, because +the generator reads annotations, not prose. After generation, `configs/.generated.yml` must +list each non-echoed field under `ignore_remote_changes` with `spec:input_only`. + +**Verdicts.** +- All inputs echoed, or non-echoed ones annotated: **NOTE**. +- A field is not echoed and not annotated: **ASK** for the annotation (or for the field to be + echoed). Until then a hand-written `ignore_remote_changes` rule with a comment is acceptable for + a setting, never for the resource's main content: ignoring a content field disables drift + detection for exactly what the user cares about. +- The main content is echoed but re-serialized by the server (reformatted document, re-keyed ids): + **ASK** for a stable representation. Decide with the API team, before the PR, how drift is + detected: a canonical form the adapter produces in `DoRead`, a server-provided hash, or an + etag. A file reference for such a payload is part of correctness here, not convenience syntax. +- An input field the server rejects on Create or Update whenever it is set: **ASK** to remove it + from the public surface or to implement it; check the create and update validators, not only + the proto. +- Whole configuration blocks are not echoed (the server keeps the intent private): **ASK** loudly. + The engine cannot detect drift on them at all; say so in the PR. +- A field is returned only when a query flag asks for it: **NOTE**, and `DoRead` must pass the + flag. If the server also rewrites the value (reformats a document, re-keys ids), the round-trip + can never settle: **ASK** for a stable representation, and decide with the API team whether the + bundle or the server owns that content. + +### 2.4 Immutable fields + +**Check.** Every field the API rejects a change to is `IMMUTABLE` (AIP-203). Fields that are +immutable only in some states must not carry the annotation; document the condition instead. + +**Verify.** Compare `IMMUTABLE` in the proto with the Update method's documentation and, if you +have access, with the server's validation code. Internal proto comment markers such as +`(--[Create:REQ Update:IGN]--)` describe create-only behaviour the annotation does not carry; they +are evidence for an ASK, not a substitute for the annotation. After generation, each one appears in +`configs/.generated.yml` under `recreate_on_changes` with `spec:immutable`. + +**Verdicts.** +- Annotated: **NOTE**. +- Not annotated but documented as create-only: **ASK** for the annotation; carry a hand-written + `recreate_on_changes` rule with a comment naming the API behaviour, and remove it once the + generated file covers it. +- Immutable only in some states (settable once, or fixed only after a sub-object exists): no + annotation and no recreate rule, which would also fire on the allowed transition. **NOTE** when + the API documents the condition; **ASK** for documentation when the only evidence is a server + validator. Either way, say in the PR that a later change fails at deploy with the server's + error, and test the allowed transition in the `lifecycle` test. +- The API has a "cannot be changed" rule nobody has written down: **ASK**, and test it against a + live workspace before relying on it. + +### 2.5 Server-computed fields + +**Check.** Timestamps, status, server-assigned ids and derived values are `OUTPUT_ONLY`. A +user-settable field with a server default stays `OPTIONAL`; the default is documented. + +**Verify.** `cli.json` `behaviors`; generated `ignore_remote_changes` with `spec:output_only`. + +**Verdicts.** Annotated: **NOTE**. A computed field left unannotated: **ASK**; expect a phantom diff +on every plan until it is fixed, and cover it with a hand-written rule in the meantime. A field the +user may set that is wrongly marked `OUTPUT_ONLY`: **ASK**; the engine will ignore user edits to it. + +A server-filled field annotated `IMMUTABLE` instead of `OUTPUT_ONLY` (a derived `name`, a +server-assigned id) is an **ASK** for the right annotation, but as long as the config never sets it +no hand rule is needed: the planner treats "unset in config and state, set remotely" as a backend +default and skips it, so the generated recreate rule does not fire on its own. Confirm with a plan +against the fake after the first deploy. + +### 2.6 Update semantics + +**Check.** Update replaces the whole resource with `update_mask: "*"` (AIP-134), or documents a +closed list of updatable fields. Everything the user can set is either updatable or `IMMUTABLE`. + +**Verify.** The Update request has a `FieldMask`; the docs state whether `*` is accepted. Try it +against a live workspace once; some services accept the mask and ignore fields anyway. + +**Verdicts.** +- `*` works and every field is updatable or immutable: **NOTE**. The adapter sends `*`. +- A documented static field list: **NOTE**. The adapter sends that list, never a list derived from + the plan. +- `*` accepted only by an internal code path, or the public doc says it is unsupported: **ASK** + for public documentation of `*` or of the closed list, and read the server's mask validator, not + only the proto comment. +- A mask path the server rejects depending on the resource's state (a list field on a resource + that has no principal, for example): **ASK**; a static mask then fails for some resources, and + the PR must say which. +- A full-body PATCH or PUT with no mask: **NOTE** if the server treats absent fields as cleared (that + is full replacement). If absent fields keep their old value, removing a field from config can + never clear it: **ASK** for a mask or for documented clearing semantics, and test clearing in the + `lifecycle` test. +- No Update at all: **NOTE**. Do not write `DoUpdate`; declare every settable field immutable so the + planner recreates. A test in the repo checks this. +- Update exists but can change only a subset of what Create accepts, with no annotation telling + the two apart: **ASK** for `IMMUTABLE` on the rest (2.4). +- Membership changes only through custom add/remove methods, or an update that can remove members + but not add them: **ASK** for full replacement through the standard Update (AIP-144). Until it + lands, do not implement `DoUpdate`: every change recreates the resource, and the PR says so. If a + recreate is unacceptable for this resource (it drops data that takes hours to rebuild), that + turns the ASK into a **STOP**. Never classify changes in Go to work around it. + +### 2.7 Long-running operations + +**Check.** A Create or Update that returns an operation declares `operation_info` (AIP-151) and the +resource id is available from the Create response or the operation metadata before the operation +completes. + +**Verify.** `rg -n "operation_info" `; read the operation's metadata type. + +**Why.** The engine persists the id right after `DoCreate` returns and polls afterwards in +`WaitAfterCreate`. If the id exists only in the final response, the adapter has to wait inline and +an interrupted deploy leaves an untracked resource. + +**Verdicts.** Id available early: **NOTE**, use the wait hooks. Id only at completion: **ASK** for +the id in metadata; until then wait inline and say in the adapter comment why the hook does not fit. + +The wait hooks receive the id, not the operation, so they poll the resource, or poll the operation +endpoint with a short loop when the SDK waiter cannot be rebuilt from the id. Either is fine inside +the hook; neither belongs in `DoCreate`, `DoUpdate` or `DoDelete`. If the resource has no +`state` field (AIP-216) that reports a failed or pending operation, a failure is indistinguishable +from "still running" or, for a failed delete, from success: **ASK** for a `state` on the resource +that the hooks can read. Also read the operation message: an operation that reports `done` without +setting `error` when the work failed makes the SDK waiter return success; that is an **ASK** for a +correct `error` on the operation, and until then the adapter must inspect the resource state after +the wait. + +### 2.8 Delete semantics + +**Check.** Delete returns `NOT_FOUND` for a missing resource unless `allow_missing` is set, and +either completes synchronously or is an operation with a documented completion signal (AIP-135). +The declared error codes are on the RPC's `error_codes` list in the proto; a status enum with a +pending-delete value is the usual hint that delete is asynchronous or soft. If Delete is refused while child objects exist, the children must be bundle resources the engine +can order through `lifecycle.depends_on` or references; otherwise `bundle destroy` cannot succeed +without manual steps. Say which in the PR. + +**Verdicts.** Standard: **NOTE**. Async delete with no completion signal, or a soft delete that +keeps the resource readable for a while (GET still returns it, perhaps in a trashed or deleting +state): **ASK** for a documented completion contract; implement `IsGone` on the terminal state and +`WaitAfterDelete` as the repo's existing resources do, and cover delete-then-recreate in the +`lifecycle` test. + +### 2.9 Read after create + +**Check.** GET returns the resource as soon as Create (or its operation) completes. + +**Why.** The planner treats a 404 on read as "gone" and plans another create. + +**Verdicts.** Immediate: **NOTE**. Documented lag: **ASK**, and cover it in `WaitAfterCreate`. +Not documented either way: **ASK** the service to state it, and let the `lifecycle` test's +create-then-plan prove it against a real workspace. A 404 observed after create is a **STOP**. + +### 2.10 Launch stage + +**Check.** The resource, its methods and new fields carry a `stage`. PyDABs generates Python classes +only for `GA` and `PUBLIC_PREVIEW` resources; `PRIVATE_PREVIEW` and `PUBLIC_BETA` are excluded, and +fields at those stages are marked experimental. + +**Verify.** Eligibility is decided by the stage on the resource message, not on its fields. A +resource whose fields are all private preview but whose message carries no stage is generated as +GA. After `./task generate-schema`, check the `x-databricks-launch-stage` on the resource's entry +in `bundle/schema/jsonschema.json`, and `bundle/internal/annotation/preview.go` for CLI-side +overrides. + +**Verdicts.** Set on the resource and matching the fields: **NOTE** the stage and the resulting +PyDABs eligibility in the PR. Missing on the resource, or lower than its fields: **ASK**, and add +the CLI-side override so a preview resource does not ship as a public Python API in the meantime. + +### 2.11 ETag + +**Check.** If the resource is edited concurrently by people and automation, it has `string etag` +(AIP-154) and Update honours it. + +**Verdicts.** Present, and Update or Delete honour it as a precondition (a mismatch fails with +`ABORTED` or 409): **ASK** the team how the bundle should use it, and write in the PR which channel +detects drift if the adapter does not send it. Present but recomputed by the server on its own +schedule (content migrations, re-keying): **ASK** for a content-stable etag or a documented rule, and +note that the adapter omits it on update. Absent: **NOTE**; drift detection falls back to full read +comparison. + +### 2.12 References from existing resources + +**Check.** If an existing bundle resource already carries a field that names this new type (an app +naming its space, a job naming a pool), the two must deploy and destroy in the right order. The +engine orders by `${resources...}` references and `lifecycle.depends_on`; a plain string field +gives it nothing. + +**Verdicts.** The referrer's field can hold a `${resources...}` reference and +the API deletes the referenced object only after its referrers are gone: **NOTE**, and add an +acceptance test that deploys and destroys both. The referenced object's delete is refused or races +while referrers exist and nothing signals completion: **ASK** for a completion signal, and record +the destroy race in the PR. + +### 2.13 Singletons + +**Check.** A singleton (one per parent, no id) defines Get and Update only (AIP-156). + +**Verdicts.** Clean singleton: **NOTE**. Create means update and Delete means reset; say so in the +adapter and in the PR. Check `apitypes.generated.yml` picked the resource type and pin it in +`apitypes.yml` if it picked a request type. + +## 3. What the API linter already covers + +"Linter clean" in universe does not mean "ready for DABs". Blocking rule groups include resource Get +and List, output-only resource ids, Create and Update and Delete request shapes, declarative +friendly fields (`name`, `uid`, `display_name`, `create_time`, `update_time`, `delete_time`), and +`OUTPUT_ONLY` on state fields. Not blocking, so check them yourself: field behaviour on every field +(2.3 to 2.5), LRO annotations (2.7), etag (2.11), singleton method rules (2.12). Every +`method_exception`, `message_exception` or `field_exception` in the proto is a place to look +twice; exceptions mark exactly the deviations that cost the CLI hand-written code. + +## 4. Note to the API team + +Send this when any check is STOP or ASK. Keep each item to the field, the annotation, and the AIP. + +``` +Subject: API readiness for bundle support () + +We checked against what the bundle deployment engine derives from proto annotations. +The items below need an API change; the CLI does not work around them. + +1. Identity (AIP-122, AIP-203): has no single identifier returned on GET. Add `name` + marked IDENTIFIER, or add `_id` to Create (AIP-133). +2. Repeated field (AIP-144): elements have a lifecycle but no client-settable key. Add a + key field on the element, or make the field a list of resource names. +3. Inputs not echoed (AIP-203): are accepted on Create but not returned by GET. Return + them, or annotate INPUT_ONLY with the reason. +4. Immutable fields (AIP-203): cannot change after Create. Annotate IMMUTABLE. +5. Server-computed fields (AIP-203): . Annotate OUTPUT_ONLY. +6. Update (AIP-134): support update_mask "*", or document the updatable field list. +7. Long-running Create (AIP-151): return the resource id in the operation metadata. +8. Delete (AIP-135): NOT_FOUND unless allow_missing; document async completion. +9. Read after Create: state when GET returns the new resource. +10. Launch stage: set `stage` on the resource, methods and new fields. + +Items marked STOP block the bundle work; items marked ASK can land after the first PR. +``` + +## 5. Shapes that look unusual but are fine + +- A server-assigned id that is stable and returned on GET. Identity is that id. +- A user-chosen id that is `OUTPUT_ONLY` on the resource and supplied as `_id` on Create. +- Hierarchical names (`parents/{p}/things/{t}`) as long as `name` alone is what GET accepts. +- Read-only resources (Get and List only). They need no Create or Update rules. +- `IMMUTABLE` deliberately absent on a conditionally immutable field. Check the condition is + documented instead. +- `OPTIONAL` where you expected `INPUT_ONLY`, as long as the field is echoed. +- A repeated field of plain values or settings with no identity. Positional diffing is correct. +- A whole request-only block such as an initial sub-resource spec that is consumed on Create and + never echoed, annotated `INPUT_ONLY`. diff --git a/.agents/skills/author-new-resource/references/pitfalls.md b/.agents/skills/author-new-resource/references/pitfalls.md new file mode 100644 index 00000000000..d19049051f4 --- /dev/null +++ b/.agents/skills/author-new-resource/references/pitfalls.md @@ -0,0 +1,131 @@ +# Pitfalls + +Patterns that have cost resource PRs weeks of review. Each entry: what it looks like, why it is a +problem, what to do instead. Reviewers use the same list, so finding yourself here before you open +the PR saves a round trip. + +## Design + +**A list key built from several fields, or from a value that is not a field.** `KeyedSlices` returns +a field name and a value; the engine later looks elements up by that field. A key such as +`"member", "a=x|b=y"` pairs elements for the diff but can never be looked up, so every generated +rule under that list stops working and the resource grows plan overrides to compensate. What to do: +go back to the API readiness gate, section 2.2. The fix is a client-settable key field on the +element, in the API. Not a composite key, not an engine change. + +**`OverrideChangeDesc` in a new resource.** It exists for the rare case the yml rules cannot +express. In a new resource it almost always means one of: the key problem above, an update endpoint +that supports only part of the resource, or a rule that belongs in the proto. Each of those is an API +conversation. If you still need it, the PR description must say which yml rule it replaces and why +that rule could not be written. + +**`DoUpdate` for an endpoint that cannot update.** If the API's update can only remove list members, +or only touch two fields, do not implement `DoUpdate` and classify the rest; recreate on any change +like the resources that have no update at all. An update path that needs code to decide what is +updatable is a sign the API should mark the rest `IMMUTABLE`. + +**Hand-written rules that mirror annotations.** `recreate_on_changes` for a field the proto should +mark `IMMUTABLE`; `ignore_remote_changes` for a field that should be `OUTPUT_ONLY` or `INPUT_ONLY`. +Acceptable only as a bridge: one comment per rule naming the API behaviour, and the proto change +filed before the PR opens. + +**A mirror config struct instead of embedding the SDK type.** The bundle schema follows the API. +Embed the SDK resource type in the config type; add a wrapper only when the SDK hides identity +behind `json:"-"` fields, and keep the wrapper to identity plus the embedded body. + +**Convenience syntax in the resource.** Friendlier duration units, shorthand that expands one config +entry into several API objects, defaults the API does not have. These are bundle-wide features that +belong in the config phase for every resource, in their own PR, after a design discussion. The +resource PR speaks the API's language. The one exception is a file reference for a payload the +server re-serializes, which the readiness gate (check 2.3) treats as part of drift detection. + +## Adapter + +**Waiting inline in `DoCreate` when the id is already known.** State is persisted after `DoCreate` +returns; a poll loop inside it means an interrupted deploy leaves an untracked resource. Return the +id and poll in `WaitAfterCreate`. If the API only yields the id at completion, say so in a comment +and in the API readiness note. + +**Prefixing errors with the resource or id.** The engine already prints `cannot create +resources..: ...` and `updating id=...`. A second prefix repeats the same id twice in +one line. Return SDK errors as they are. + +**Checking what the framework guarantees.** Receivers, the arguments to `DoCreate`, `DoUpdate` and +`DoDelete`, and the id passed in are never nil or empty; a check "just in case" is dead code. The +id you return from `DoCreate` is your responsibility: an empty one is saved as state and the next +plan creates the resource again. + +**Logic in `RemapState`.** It is a field-by-field copy. Remapping a differently named field there +hides real drift. Put the remap in `DoRead` and make the field part of the remote type. + +**A custom update mask.** Send `*` when the API supports it, otherwise a static list with a comment. +Never derive the mask from the planned changes. + +## Fake server + +**Modelling backend validation.** Rules about which combinations of fields the service accepts, +cascade semantics between members, error-code aggregation. The CLI only surfaces these errors; it +does not branch on them. Each such rule is an untested claim about the backend that the goldens then +pin. Model create, read, update, delete and server-assigned fields; cover service behaviour with the +cloud test. + +**Failure triggered by a magic string in a fixture.** A feature name containing `fails` that makes +the fake return an error is invisible to the next reader. Use `fault.py` and `kill_after.py`. + +**Operations that are always already complete.** If the API is asynchronous, the fake should be +able to return a pending operation at least once, or the polling loop is never exercised locally. + +**Plain-text error bodies.** The SDK maps errors to typed sentinels only for JSON bodies with +`error_code`. Return JSON. + +## Tests + +**An invariant config that is not in the matrix.** Adding the template is half the step. Register it +under `EnvMatrix.INPUT_CONFIG`, exclude the legs it cannot run, and prove it ran. + +**Many directories, one behaviour each.** Five or more directories under `resources//` +usually means backend refusal cases got their own tests. The canonical set is `basic`, `lifecycle`, +`remote-delete`, plus one directory per plan behaviour the resource adds (an immutable field, a +dependency). Fold the rest. + +**Plan-only coverage of a recreate.** `bundle plan` showing `recreate` proves the classifier; only +`bundle deploy --auto-approve` proves the delete and create requests. Execute it. + +**Dumping an API response into the golden.** The backend adds fields; the golden breaks. Project to +the fields you assert. + +**Titles as paragraphs.** `title` takes one line. Explanation belongs in the PR description or a +code comment, not in `output.txt`. + +**Goldens with unresolved `${resources...}` references.** The script printed config, not state. +Print what the deploy produced. + +**A `[[Server]]` stub for a route the fake models.** Stubs are for routes the fake does not have. +Teach the fake instead. + +## Wiring + +**Approval prompt in deploy but not destroy.** Both groups must list the type, with a destroy +message. + +**A type missing from the deployment-history allowlist, silently.** Either add it (and coordinate +the backend allowlist) or exclude the `DMS=true` leg with a comment. Never leave the leg failing or +unexplained. + +**The bind test in the resource directory.** It lives under `acceptance/bundle/deployment/bind/`. + +**Placeholder descriptions left in `annotations.yml`.** Fill the resource-level and field +descriptions, then regenerate the schema; do not edit `jsonschema.json` by hand. + +## Scope and prose + +**Changes to another resource riding along.** Approval prompts for a related type, a mutator +tweak, a message rewording. Each is its own PR. + +**Comments that explain the engine.** A comment on the adapter should say what the API does that +the code is reacting to. How the planner matches patterns, or what `ignore_remote_changes` means, +belongs in the engine's README, once. + +**PR descriptions that describe design.** Say what the resource is, what shape its identity and +update have, which tests ran on cloud, and what API changes were requested. Leave the mechanism to +the code. diff --git a/.agents/skills/author-new-resource/references/testing.md b/.agents/skills/author-new-resource/references/testing.md new file mode 100644 index 00000000000..d9a9932a0bc --- /dev/null +++ b/.agents/skills/author-new-resource/references/testing.md @@ -0,0 +1,146 @@ +# Testing a new resource + +Every item here ends with a command that proves the test exists and ran. A test that is written but +never selected is the most common gap in resource PRs, so run the proof commands, not just the +tests. + +## 1. Go tests that must know about the type + +Most of these are completeness tests that fail until you add the type. That is intended: the +failure tells you which decision you have not made yet. + +| Test | What it enforces | Run alone | +|---|---|---| +| `TestAll` in `bundle/direct/dresources/all_test.go` | Create, read, update, delete through the adapter against the fake, round-trip of every field you set in `testConfig`. Add a `testConfig` entry with every field populated; a type without one runs with a zero config and proves nothing. Add `testDeps` if the fake needs a parent object first. | `go test ./bundle/direct/dresources -run 'TestAll/^$' -v -count=1` | +| `TestNoUpdateResourcesCoverAllFields` (same file) | For a type without `DoUpdate`: every settable field is covered by a recreate, id or ignore rule, so no change can plan an unsupported update. | `go test ./bundle/direct/dresources -run 'TestNoUpdateResourcesCoverAllFields/^$' -v -count=1` | +| `TestNameStableOrOptedOut` (same file) | A backend-assigned `name` on a type with `DoUpdate` is declared in `stable_output_fields`. | `go test ./bundle/direct/dresources -run TestNameStableOrOptedOut -v -count=1` | +| `TestInputSubset`, `TestRemoteSuperset` in `type_test.go` | Config fields exist in the state type; state fields exist in the remote type. Exceptions are explicit lists in that file and must have a comment. | `go test ./bundle/direct/dresources -run 'TestInputSubset\|TestRemoteSuperset' -v -count=1` | +| `TestResourceConfig`, `TestGeneratedResourceConfig`, `TestConfigFiles`, `TestResourcesYML*` in `all_test.go` and `config_test.go` | Every rule in `configs/.yml` resolves to a real field, no rule duplicates the generated file, the file name is a registered type. | `go test ./bundle/direct/dresources -run 'TestResourceConfig\|TestConfigFiles\|TestResourcesYML' -v -count=1` | +| `TestCustomMarshallerIsImplemented`, `TestResourcesAllResourcesCompleteness`, `TestSupportedResources` in `bundle/config/resources_test.go` | The config type has value-receiver `MarshalJSON` and pointer-receiver `UnmarshalJSON`; it is in `AllResources()` and `SupportedResources()`. | `go test ./bundle/config -run 'TestCustomMarshallerIsImplemented\|TestResourcesAllResourcesCompleteness\|TestSupportedResources' -v -count=1` | +| `TestBundleResourcePluralNamesResolveInWorkspaceURLs` (same file) | The type has a URL pattern in `libs/workspaceurls`, or is listed in the test's no-URL map with a comment saying why there is no page. | `go test ./bundle/config -run TestBundleResourcePluralNamesResolveInWorkspaceURLs -v -count=1` | +| `TestResourcesBindSupport` (same file) | `Exists` works for the type through a strict mock; add an instance to the fixture and a mock expectation for the GET call `Exists` makes. | `go test ./bundle/config -run TestResourcesBindSupport -v -count=1` | +| `TestAllResourcesMocked`, `TestAppropriateResourcesAreRenamed`, `TestProcessTargetMode*` in `bundle/config/mutator/resourcemutator/apply_target_mode_test.go` | The dev-mode fixture has an instance of the type; a user-facing `Name` gets the dev prefix unless the type is listed as not user-named with a reason. | `go test ./bundle/config/mutator/resourcemutator -run 'TestAllResourcesMocked\|TestAppropriateResourcesAreRenamed\|TestProcessTargetMode' -v -count=1` | +| `TestRunAs*` in `run_as_test.go` | The type appears in the sorted list of all resource types, and is either in the run_as allow list or produces the "not supported" diagnostic. | `go test ./bundle/config/mutator/resourcemutator -run TestRunAs -v -count=1` | +| `TestAllResourcesExplicitlyDefinedForPermissionsSupport` in `apply_bundle_permissions_test.go` | The type is in the permissions level map or in the unsupported list. | `go test ./bundle/config/mutator/resourcemutator -run TestAllResourcesExplicitlyDefinedForPermissionsSupport -v -count=1` | +| `TestStateToBundle*` in `bundle/statemgmt/state_load_test.go` | State with the type loads back into config with id and modified status. Nothing enumerates types here, so add the fixture yourself. | `go test ./bundle/statemgmt -run TestStateToBundle -v -count=1` | +| `cmd/experimental/workspace_open_test.go` | If you added a URL pattern: the completion list, help text and unknown-type error list it. | `go test ./cmd/experimental -run 'TestBuildWorkspaceURL\|TestWorkspaceOpenCommand' -v -count=1` | +| `TestInvariantConfigsCoverage` in `acceptance/invariant_test.go` | An invariant config file exists for the type. It does not check the matrix; see section 2. | `go test ./acceptance -run '^TestInvariantConfigsCoverage$' -v -count=1` | +| `test_python_support_coverage`, `test_has_all_test_cases`, `test_public_api` under `python/databricks_tests/core/` | PyDABs has a support fixture, a generated test case and an updated public API snapshot for the type (only when the launch stage makes it eligible). | `cd python && uv run pytest databricks_tests/core -q` | + +Use `-count=1` so Go does not report a cached result as a run. + +## 2. The acceptance set + +Directory layout under `acceptance/bundle/resources//` plus three directories elsewhere. +Copy the most recent resource's directories and adapt; do not invent new shapes. + +| Directory | Covers | Settings | +|---|---|---| +| `resources//basic/` | `bundle summary` before and after deploy with id and URL, the create request body via `print_requests.py`, a redeploy that plans nothing, an in-place update, an immutable change that plans a recreate and is then deployed with `--auto-approve`, destroy with the delete request. | Local only. `RecordRequests` is inherited from the parent `resources/test.toml`. | +| `resources//lifecycle/` | Create, update, plan clean, destroy against a real workspace. Uses `$UNIQUE_NAME` in every name, creates its own prerequisites, cleans up in a trap. | `Cloud = true`, `RecordRequests = false`. This is the live run the resource README requires. | +| `resources//remote-delete/` | Delete the object out of band with the CLI's own command, then `bundle plan` must detect it is gone and plan a create. | Local only. | +| `deployment/bind//` | Create the object outside the bundle with the CLI's own create command against the fake, `bundle deployment bind` it, summary, `unbind`, summary. | Local only. Older bind tests stub the object's GET with a `[[Server]]` block instead; do not copy that, the fake already models the route. | +| `invariant/configs/.yml.tmpl` | The generic no-drift, delete-idempotent and destroy-idempotent suites run your config. Add `-init.sh` and `-cleanup.sh` next to it if the resource needs a parent object the harness does not provide. | Register the file name under `EnvMatrix.INPUT_CONFIG` in `acceptance/bundle/invariant/test.toml`. Add an `EnvMatrixExclude` entry for the `DMS=true` leg if the type is not in the deployment-history allowlist, and for cloud if the config cannot run there. Add the type to the exclusion lists in `invariant/migrate` and `invariant/auto-migrate`, which seed state with an older engine. | +| `python/-support/` | The PyDABs model loads from YAML and from Python and a mutator runs. Six files; copy the newest sibling directory. | `EnvMatrix.PYDAB_VERSION = ["current"]` for a brand-new type. Only for types PyDABs generates (GA or public preview). | +| `refschema/output/.txt` | Generated listing of the type's fields and which are state. | Regenerated by `-update`; commit it. | + +Script conventions, all enforced by review: wrap every command in `trace`; one-line `title` per +phase; `musterr` for a command that must fail; `print_requests.py //` instead of inline +`jq` on the request log; `replace_ids.py` right after the first deploy; project any API read you +print to the fields you assert; commit static inputs instead of generating them in the script. + +Every `EnvMatrix` variant must produce the same `output.txt`. The bundle tree runs a `DMS=` and a +`DMS=true` leg by default; a type absent from `dmsResourceTypes` in +`libs/testserver/bundledeployments.go` fails the second leg on the first deploy, so either add it +there (the list mirrors the service's allowlist; coordinate the backend change) or exclude the leg +with a comment that says why. + +## 3. The fake server + +Scope rule: model what the CLI's plan and apply depend on, and nothing else. + +- Create returns the object with server-assigned fields filled (id, timestamps, derived names). +- Get returns the stored object; fields the real API never echoes are stripped only when the proto + marks them input-only. +- Update applies the mask the adapter sends (`*` means replace the writable fields and keep the + server-assigned ones). +- Delete removes the object. A missing object returns 404 with a JSON body carrying `error_code` + (`RESOURCE_DOES_NOT_EXIST`), a duplicate create 409 (`RESOURCE_ALREADY_EXISTS`), bad input 400 + (`INVALID_PARAMETER_VALUE`). The SDK maps these to typed errors only when the body is JSON. + +Mechanics: one file `libs/testserver/.go`, a map on `FakeWorkspace` keyed by id, every +handler including delete starts with `defer s.LockUnlock()()`, routes registered in `handlers.go` +next to the neighbouring service. For an asynchronous API, the fake returns a pending operation at +least once before completing, so the adapter's polling runs locally. `MapGet` and `MapDelete` cover get and delete when the stored body is the +request body. Expect 70 to 100 lines. A fake several times that size is modelling backend policy +the CLI does not branch on; move that coverage to the cloud test and cite the server code in the +PR instead. + +Do not encode failure triggers in fixture data. Transport and response faults come from +`acceptance/bin/fault.py` and `kill_after.py`, which the harness already provides. + +## 4. Proving a test ran + +```bash +# every subtest for the resource, with its matrix variants +go test ./acceptance -run 'TestAccept/bundle/resources/' -v -count=1 2>&1 | grep -E -- '--- (PASS|SKIP|FAIL): ' + +# the invariant suite actually selected your config +go test ./acceptance -run 'TestAccept/bundle/invariant/no_drift' -v -count=1 2>&1 | grep 'INPUT_CONFIG=' + +# the DMS leg runs (or is deliberately excluded); counts passing subtests, not log lines +go test ./acceptance -run 'TestAccept/bundle/resources/' -v -count=1 2>&1 | grep -c -- '--- PASS.*DMS=true' + +# effective settings after inheritance +cat acceptance/bundle/resources//*/out.test.toml +``` + +A test that does not appear in the first listing was not selected. Tests under `Cloud = true` also +run locally; the cloud run is additional. On the PR, the Integration Tests check links to the cloud +run; its job log contains a line `Selected N changed tests (...)` followed by `PASS` and `SKIP` +lines with reasons. Confirm your `lifecycle` test is in the `PASS` lines, and quote that in the PR. +Ask a maintainer for the log if you cannot open the run. + +## 5. Regenerating goldens + +```bash +go test ./acceptance -run 'TestAccept/bundle/resources/' -update -count=1 +go test ./acceptance -run 'TestAccept/bundle/resources/' -count=1 +``` + +Run without `-update` afterwards and require a clean pass. Never run `-update` while two matrix +variants produce different output; fix the variant first. `./task test-update` regenerates the +whole suite and can pick up unrelated drift; revert any output file you did not mean to change. +Regenerate `out.test.toml` files alone with +`go test ./acceptance -run '^TestAccept$' -only-out-test-toml`. + +## 6. The cloud run + +A new type needs at least one test executed against a real workspace before the PR is ready. +Locally: + +```bash +deco env run -i -n aws-prod-ucws -- go test ./acceptance -run 'TestAccept/bundle/resources//lifecycle' -v -count=1 +``` + +In the PR description, under Tests, state which directories are `Cloud = true`, which environment +they ran on, and link the run. If the cloud run did not happen, say so and why; do not describe the +local run as cloud coverage. + +## 7. Changelog fragment + +One new file `.nextchanges/bundles/.md`, one line, imperative, ending with the PR link: + +``` +* Add the `` resource for . ([#NNNN](https://github.com/databricks/cli/pull/NNNN)) +``` + +Never edit `CHANGELOG.md` or `.nextchanges/version`. + +## 8. Before opening the PR + +Follow `.agents/skills/pr-checklist/SKILL.md`: `./task fmt`, `./task checks`, `./task lint`, +`./task test`, the generators you touched (`./task generate-schema`, `./task generate-direct`, +`./task pydabs-codegen pydabs-test pydabs-lint pydabs-docs` when Python changed), then the cleanup +scan for debug prints and stray files. Use the PR template's three sections and keep the +description short; the diff carries the detail. diff --git a/.agents/skills/author-new-resource/references/wiring.md b/.agents/skills/author-new-resource/references/wiring.md new file mode 100644 index 00000000000..68b053e2dc8 --- /dev/null +++ b/.agents/skills/author-new-resource/references/wiring.md @@ -0,0 +1,103 @@ +# Wiring checklist + +Every place a new resource type is registered, generated or tested. For each row: what to add, and +what catches it when forgotten. Rows marked **nothing catches it** are the ones to verify by hand +with the command given; they are where resource PRs most often come back from review. + +Use `` and `` for the resource names, `` for the Go identifier. + +## 1. Config type and registration (`bundle/config`) + +| Add | Where | Caught by | +|---|---|---| +| `type struct { BaseResource; ID string \`json:"id,omitempty" bundle:"readonly"\`; . }` with value-receiver `MarshalJSON` and pointer-receiver `UnmarshalJSON` via the SDK `marshal` package | `bundle/config/resources/.go` (new) | `TestCustomMarshallerIsImplemented` in `bundle/config/resources_test.go` | +| `Exists(ctx, client, id)` using the service's Get and `apierr.IsMissing` | same file | `TestResourcesBindSupport`: add an instance to its fixture and a mock expectation for the Get call. The test binds with the id `"0"`; a composed id needs an entry in its per-type id map | +| `ResourceDescription()` with singular, plural and titles | same file | Empty plural panics at startup in `collectResourceMap` | +| `GetName()`, `InitializeURL(baseURL)`, `GetURL()` | same file | `TestBundleResourcePluralNamesResolveInWorkspaceURLs`: add a URL pattern (row below) or add the plural to that test's no-URL map with a comment saying the UI has no page | +| Map field ` map[string]*resources. \`json:",omitempty"\`` | `bundle/config/resources.go`, the `Resources` struct | Everything below keys off it | +| Entry in `AllResources()` | `bundle/config/resources.go` | `TestResourcesAllResourcesCompleteness` | +| Entry in `SupportedResources()` | `bundle/config/resources.go` | `TestSupportedResources` | +| `Grants` or `Permissions` field on the type only if the API exposes a public grants or permissions surface for it | `bundle/config/resources/.go` | Derives `.grants` or `.permissions` keys that then need rows in sections 2 and 6 | + +## 2. Direct engine adapter (`bundle/direct/dresources`) + +| Add | Where | Caught by | +|---|---|---| +| `Resource` with `New`, `PrepareState`, `DoRead`, `DoCreate`, `DoUpdate` (only if the API can update), `DoDelete`, and `WaitAfterCreate`/`WaitAfterUpdate`/`WaitAfterDelete` for asynchronous APIs. Helper methods unexported; one SDK call per method; SDK errors returned as they are | `bundle/direct/dresources/.go` (new) | `NewAdapter` rejects wrong signatures and extra exported methods; `TestAll` runs CRUD against the fake | +| Entry `"": (*Resource)(nil)` | `bundle/direct/dresources/all.go` | A config type without an adapter fails only at plan time with "resource type not supported". **Nothing catches it in unit tests**; verify with `go test ./bundle/direct/dresources -run 'TestAll/^$' -v -count=1` | +| `testConfig[""]` with every field populated; `testDeps` if a parent object must exist first | `bundle/direct/dresources/all_test.go` | A missing entry runs `TestAll` with a zero config and proves nothing. **Nothing catches it**; check the entry exists | +| `configs/.yml` with only the rules the proto cannot express: `provided_id_fields` for identity components, `stable_output_fields` for a server-assigned `name`, `backend_defaults` for server-filled defaults, and a commented `recreate_on_changes` or `ignore_remote_changes` only as a bridge until the upstream annotation lands | `bundle/direct/dresources/configs/.yml` (omit if empty) | `TestConfigFiles`, `TestResourceConfig`, `TestResourcesYMLNoRedundantRules` (a hand rule the generated file already produces fails), `TestNameStableOrOptedOut`, `TestNoUpdateResourcesCoverAllFields` (types without `DoUpdate`) | +| `KeyedSlices()` for repeated fields the API returns unordered, keyed by one real field of the element | adapter file | **Nothing catches a wrong key**; the readiness gate decides whether a key exists | +| `apitypes.yml` pin only when `apitypes.generated.yml` picked a request type instead of the resource type | `bundle/direct/dresources/apitypes.yml` | **Nothing catches a wrong or misspelled pin**; a typo yields an empty generated config. Verify `configs/.generated.yml` lists the behaviours you expect | + +## 3. Generated files + +Run in this order after the Go types compile; `./task generate` runs all of them. + +| Command | Writes | Check | +|---|---|---| +| `./task generate-refschema` | `acceptance/bundle/refschema/output/.txt` | File exists and lists the resource's fields with `STATE` or `ALL` | +| `./task generate-schema` | `bundle/schema/jsonschema.json`, and rewrites `bundle/internal/schema/annotations.yml` adding `PLACEHOLDER` entries | Fill every placeholder for the type (resource-level description, `lifecycle`, each field without an upstream description), then run again. `TestRequiredAnnotationsForNewFields` in `bundle/internal/schema` fails on leftovers | +| `./task generate-validation` | `bundle/internal/validation/generated/required_fields.go`, `enum_fields.go` | Entries for the type appear | +| `./task generate-direct` | `bundle/direct/dresources/apitypes.generated.yml`, `configs/.generated.yml` | The plural has an entry naming the resource type; the generated rules match the proto's `IMMUTABLE`, `OUTPUT_ONLY`, `INPUT_ONLY`. A missing entry means the heuristic found nothing: pin it | +| `./task pydabs-codegen` | `python/databricks/bundles//`, `core/_generated/`, docs, test cases | Only produced for GA and public-preview stages. Regenerate the public API snapshot: `cd python && UPDATE_SNAPSHOTS=1 uv run pytest databricks_tests/core/test_public_api.py`. The task deletes the generated packages before it fetches dependencies; if it fails on network access, restore with `git checkout -- python/`, sync the `python/` and `python/codegen/` environments first, and rerun | +| `./task generate-check` | nothing | Must be clean; CI runs it | + +Never edit `jsonschema.json`, `*.generated.yml`, the validation files or the Python packages by +hand. `annotations.yml` is the one hybrid file: generated structure, hand-written text. + +## 4. Fake server (`libs/testserver`) + +| Add | Where | Caught by | +|---|---|---| +| ` map[string].` on `FakeWorkspace`, initialised in `NewFakeWorkspace` | `libs/testserver/fake_workspace.go` | `TestAll` 404s | +| Create, get, update, delete handlers; `MapGet`/`MapDelete` where the stored body is the request body; a dedicated method where the server assigns fields or applies a mask | `libs/testserver/.go` (new), routes in `handlers.go` next to the neighbouring service | `TestAll` and every acceptance test | +| The plural in `dmsResourceTypes`, mirroring the deployment-history service's allowlist (a backend change you coordinate), or an `EnvMatrixExclude` for `DMS=true` with a comment in the resource's `test.toml` and in `acceptance/bundle/invariant/test.toml` | `libs/testserver/bundledeployments.go` | **Nothing catches it in Go**; the `DMS=true` acceptance leg fails on the first deploy | + +## 5. Bundle-wide behaviour (`bundle/config/mutator/resourcemutator`, `bundle/phases`, `libs/workspaceurls`) + +Each row is a decision, not a default. Write the decision in the PR description. + +| Decision | Where | Caught by | +|---|---|---| +| `run_as`: does deploying as another principal make sense? If not, add a "not supported" block | `run_as.go`; test lists in `run_as_test.go` (`allResourceTypes` exact list, `allowList`) | `TestRunAs*` fail until the type is in the exact list and either allowed or blocked | +| Development-mode presets: name prefix, tags, paused schedules. Check the API's name length and character constraints against the `[dev user]` prefix before deciding to prefix | `apply_presets.go`; fixture and carve-out list in `apply_target_mode_test.go` | `TestAllResourcesMocked`, `TestAppropriateResourcesAreRenamed` (a user-facing `Name` must be prefixed or carved out with a reason); the preset itself needs an acceptance test, nothing enforces that | +| Permissions: level map entry if the API has permissions, else the unsupported list | `apply_bundle_permissions.go`, `apply_bundle_permissions_test.go` | `TestAllResourcesExplicitlyDefinedForPermissionsSupport` | +| UC dependencies: if config names a catalog or schema by name that may be a bundle resource, rewrite it to `${resources.schemas...}`. If the API takes UUIDs instead of names, there is nothing to capture; document that users reference `${resources.schemas..schema_id}` explicitly | `capture_uc_dependencies.go` | **Nothing catches it**; add a case to the implicit-deps acceptance test | +| Approval prompt for deletion or recreation that loses data, is expensive to rebuild, drops child objects, or loses an identity the API minted (a service principal, grants on the old object) | The approval table in `bundle/phases` (older trees: separate lists in `deploy.go` and `destroy.go`), message in `messages.go`. Deploy and destroy both, unless destroy's prompt comes from somewhere else or deletion is harmless while recreation is not; say which in a comment on the entry | **Nothing catches a missing entry**; the resource's `basic` golden shows the prompt | +| Workspace URL: pattern if the UI has a page | `libs/workspaceurls/urls.go`; `cmd/experimental/workspace_open_test.go` lists; `acceptance/experimental/open/output.txt` | `TestBundleResourcePluralNamesResolveInWorkspaceURLs`; the `workspace_open_test.go` lists are exact and fail until updated | +| State fixtures | `bundle/statemgmt/state_load_test.go`: one entry per `TestStateToBundle*` fixture | The tests assert full resource coverage and fail for a type without a fixture | +| `bundle run`: only if the resource has a run action | `bundle/run/runner.go` | Not applicable to most types | + +Automatic through `BaseResource`: `lifecycle.prevent_destroy`, `lifecycle.depends_on`, telemetry, +`bundle summary`, `bundle open` (refuses when `GetURL` returns false), bind and unbind through +`Exists`. + +## 6. Tests and fixtures + +See `testing.md` for the content of each. Registration points only: + +| Add | Where | Caught by | +|---|---|---| +| `acceptance/bundle/resources//{basic,lifecycle,remote-delete}/` | new directories | Documented requirement, **not enforced**; reviewers check | +| `acceptance/bundle/deployment/bind//` | new directory | same | +| `acceptance/bundle/invariant/configs/.yml.tmpl` (+ `-init.sh`, `-cleanup.sh` if needed) | new files | `TestInvariantConfigsCoverage` checks the file exists | +| The file name under `EnvMatrix.INPUT_CONFIG`, plus `EnvMatrixExclude` for `DMS=true` or cloud when needed, plus exclusions in `invariant/migrate/test.toml` and `invariant/auto-migrate/test.toml`; `invariant/continue_293` seeds every config with an old CLI that does not know the type, so exclude it there if that leg fails | `acceptance/bundle/invariant/test.toml` and the subdirectories | **Nothing catches a missing matrix entry**; verify with `go test ./acceptance -run 'TestAccept/bundle/invariant/no_drift' -v -count=1 2>&1 \| grep 'INPUT_CONFIG='` | +| `acceptance/bundle/python/-support/` when PyDABs generates the type | new directory, six files | `test_python_support_coverage` | +| `.nextchanges/bundles/.md` | new file | `task check-changelog` once the PR exists; reviewers otherwise. The line ends with the PR link, so fill the number in right after opening the PR | + +## 7. Reading the generated rules back + +After `./task generate-direct`, open `configs/.generated.yml` and compare with the API docs: + +- Every field GET does not return should be under `ignore_remote_changes` with `spec:input_only`. +- Every server-computed field should be there with `spec:output_only`. +- Every create-only field should be under `recreate_on_changes` with `spec:immutable`. + +Anything missing from that list and present in your hand-written `configs/.yml` is an +annotation the proto lacks. File it upstream and keep the hand rule commented as a bridge. + +If the generated file is absent or says there are no field behaviours for the resource, the +generator did not find a matching API type: either the config type is a hand-written mirror (embed +the SDK type instead) or the heuristic picked nothing (pin the type in `apitypes.yml`). Do not +predict generated rules in a plan; read them back after `./task generate-direct`. diff --git a/.agents/skills/review-new-resource/SKILL.md b/.agents/skills/review-new-resource/SKILL.md new file mode 100644 index 00000000000..82c9069ff60 --- /dev/null +++ b/.agents/skills/review-new-resource/SKILL.md @@ -0,0 +1,226 @@ +--- +name: review-new-resource +description: Review a branch or PR that adds a new bundle resource type to the direct engine, as a maintainer would. Checks scope, API shape and plan semantics first, then adapter contract, wiring completeness, fake server scope, test coverage and whether the tests actually ran, generated-file hygiene and user-facing text. Use when asked to review a change that adds a resources. type, or when a review of such a PR should start from the design rather than from line comments. +--- + +# Review a new resource PR + +A new resource should be one PR, about 250 to 400 hand-written Go lines, an adapter of 60 to 130 +lines with no plan overrides, a fake of 70 to 100 lines, and three to five acceptance directories. +Deviations are where review time goes. Judge the change against the repo's precedents and the API +readiness gate, not against the author's design; a gap in the API is fixed in the API, not in the +resource and never in the engine. + +Read `../author-new-resource/references/api-readiness.md`, `wiring.md`, `testing.md` and +`pitfalls.md` once; they are the standards this review applies. + +## 1. Collect the change + +```bash +TARGET=HEAD # or the commit you were given +BASE=$(git merge-base "$TARGET" origin/main) +git diff --stat "$BASE" "$TARGET" +git diff "$BASE" "$TARGET" -- . ':!*.generated.yml' ':!bundle/schema/jsonschema.json' ':!python/' ':!acceptance/**/output.txt' ':!acceptance/**/out.*' +``` + +Read every changed non-generated file in full at `TARGET` (`git show "$TARGET":path`). Skim +generated files only to confirm they follow from the source. Do not read existing review comments +before forming your view; read the PR description for intent if you have it. + +Identify the resource: its plural, the config type under `bundle/config/resources/`, the adapter +under `bundle/direct/dresources/`, its `configs/.yml` and `.generated.yml`, the fake under +`libs/testserver/`, and the acceptance directories. + +## 2. Triage before line comments + +Compute and write down: + +```bash +git diff --numstat "$BASE" "$TARGET" -- '*.go' ':!*_test.go' ':!*generated*' ':!python/' | awk '{a+=$1} END {print a}' +git show "$TARGET":bundle/direct/dresources/.go | wc -l +git show "$TARGET":libs/testserver/.go | wc -l +git ls-tree --name-only "$TARGET" acceptance/bundle/resources// +``` + +Then compare with the two most recent resource additions on `origin/main`. A number more than twice +the precedent is the first comment, asked as a question about the cause, before anything else. + +## 3. Required passes + +Every pass produces its table for the worksheet, even when empty. A finding needs code you read; +drop anything you cannot point to. + +### P1. Scope + +List every changed file that is not part of adding this resource: another resource's behaviour, +generic mutators, engine code under `bundle/direct/*.go` or `libs/structs/`, messages for other +types, bundle-wide syntax features. Each is a split request, made once, before reviewing its +content. +Table: `file | belongs to the new resource? | if not, what it changes` + +### P2. API shape and plan semantics + +Apply the readiness gate to the result. For each row, cite the file and line. + +- Identity: how the engine id is formed; `provided_id_fields` or `updatable_id_fields` present + and matching; a composed id has a composer and a parser on the config type. +- `KeyedSlices`: every key function returns a real field of the element. A key built from several + fields or from a value the client does not send is a redesign comment, pointing at the gate. +- `OverrideChangeDesc`: present? For each branch, which yml rule it stands in for and why that rule + could not be written. Default position: it should not exist in a new resource. +- `configs/.yml`: every hand-written rule has a comment naming the API behaviour; rules + that mirror `IMMUTABLE`, `OUTPUT_ONLY` or `INPUT_ONLY` have an upstream annotation request + mentioned in the PR. +- `configs/.generated.yml` and `apitypes.generated.yml`: the type picked is the resource + type, not a request type; input-only and output-only fields match the API docs. +- `DoUpdate`: present only if the API can update what the config can change; a removal-only or + partial update is a design comment. For a `*` mask, find the evidence that this API accepts it: + the API doc line in `.codegen/cli.json`, or a cloud test that performs an update. No evidence is + a question, not a pass. +- Every ignore rule, hand-written or generated: what does ignoring this field cost the user, and + how do they restore intended state if it drifts? An `ignore_remote_changes` on a field the user + sets, or an `ignore_local_changes` anywhere, needs that answer in a comment. +- Sibling resources: if another resource manages the same kind of object or the same service + (an older API generation, a related type in the same package), list where this one behaves + differently (approvals, presets, URL, fix-ups) and ask whether each difference is intended. +- Long-running calls: id returned before polling, polling in the wait hooks; inline waits carry a + comment that explains why the hook does not fit. +Table: `item | finding (file:line) | verdict: fine / ask upstream / redesign at API` + +### P3. Adapter contract + +Check against `bundle/direct/dresources/README.md` and the `IResource` interface in `adapter.go`: +one SDK call per `DoCreate`/`DoUpdate`/`DoDelete`; SDK errors returned unwrapped; no nil checks on +arguments the framework guarantees; `DoCreate` cannot return an empty id; `RemapState` is a trivial copy; remapping lives in `DoRead`; +`Exists` uses `apierr.IsMissing`; `GetURL` returns `"", false` with a comment only when there is no +UI page; marshal methods on the config type have the right receivers. +Table: `method | contract point | ok? (file:line)` + +### P4. Wiring completeness + +Walk the checklist in `../author-new-resource/references/wiring.md`. For each row: present, absent +with a justifying comment, or absent. Pay attention to the rows that nothing enforces: state load +fixture, invariant matrix registration, deployment-history allowlist or exclusion, deploy and +destroy approval groups together, URL plus its command tests, UC dependency capture when the +resource names a catalog or schema, run_as and presets decisions. +Table: `checklist row | present? (file:line) | if absent: justified?` + +### P5. Fake server + +Size against precedent; scope limited to create, read, update, delete and server-assigned fields; +JSON error bodies with `error_code`; no failure triggers in fixture data; a pending operation path +if the API is asynchronous; redaction only of fields the proto marks input-only. Every modelled +backend rule beyond CRUD needs either a server source reference or a `Cloud = true` test that +exercises it. +Table: `behaviour modelled | needed by the CLI? | evidence (server ref or cloud test)` + +### P6. Tests and whether they ran + +- Canonical set present: `basic`, `lifecycle` with `Cloud = true`, `remote-delete`, + `deployment/bind/`, invariant template, PyDABs support dir when eligible. +- Invariant template registered in `acceptance/bundle/invariant/test.toml`; migrate and + auto-migrate exclusions added. +- `out.test.toml` files show the expected `Cloud` and `DMS` values; a `DMS=true` exclusion has a + comment. +- The recreate is executed, not only planned; `print_requests.py` shows the delete and create. +- Scripts: `trace`, one-line `title`, `musterr`, `print_requests.py`, `replace_ids.py`, projection + of API output; no inline `jq` on the request log; no `[[Server]]` stub for a route the fake models. +- Goldens: no unresolved `${resources...}`; no full API dumps. A golden that shows a failed + operation reported as success, an object left behind after destroy, or an error swallowed is a + finding even when the cause is pre-existing engine behaviour: the test must not enshrine it. +- Deployment history: if the `DMS=true` leg is excluded anywhere, the type is absent from + `dmsResourceTypes` in `libs/testserver/bundledeployments.go`; ask whether the backend allowlist + change is tracked and say so in the review body. An exclusion without that is a comment. +- Unit tests: `testConfig` entry populates every field; completeness-list tests updated rather than + special-cased (`run_as_test.go`, `apply_bundle_permissions_test.go`, `workspace_open_test.go`, + `all_test.go`). A branch on the resource name inside a shared harness is a comment. +- Evidence of a cloud run in the PR description, with the environment. +- Bundle-wide behaviour the resource opted into (a dev-mode preset such as a prefix or a paused + schedule, run_as rejection, UC dependency capture, approval prompts) has an acceptance test that + shows it; a unit test alone is a comment. +- Any claim in this review that a test exists, or that a file does something, cites `path:line` + you read. Verify counts with a command before stating them. +Table: `expected test | present? | ran? (out.test.toml / description) | note` + +### P7. Generated files and descriptions + +`annotations.yml` has real descriptions for the resource and its fields and was regenerated, not +pasted; `jsonschema.json`, validation files, refschema output and PyDABs models follow from a +generator run (`./task generate-schema`, `generate-direct`, `pydabs-codegen`). Launch stage in the +schema matches the proto and PyDABs eligibility. +Table: `generated file | consistent with source? | note` + +### P8. User-facing text + +Changelog fragment under `.nextchanges/bundles/` in the required one-line form; approval messages +short and actionable; error strings without trailing punctuation; PR description in the template's +three sections, stating cloud coverage and any API changes requested. +Table: `text (file:line) | issue | suggested wording` + +### P9. Comments and size + +Comments on the adapter explain what the API does, not how the engine works. Multi-paragraph +comments, comments that restate code, and comments describing planner internals get a "delete" or +"move to the README" comment. Prose titles in scripts get the same. +Table: `location | what the comment explains | keep / delete / move` + +Skip formatting, identifier naming, anything a linter catches, and defensive-check debates beyond +what P3 lists. + +## 4. Red flags + +Any of these is a top-of-review comment and usually a redesign at the API: + +| Seen in the diff | Means | +|---|---| +| `KeyedSlices` key assembled from several fields or a non-field name | Elements have no client identity; ask the API for a key field | +| `OverrideChangeDesc` in a new resource | A yml rule or an API annotation is missing | +| `DoUpdate` whose mask excludes most of the config, or whose endpoint only removes | API update semantics do not fit declarative config | +| Hand-written `recreate_on_changes` or `ignore_remote_changes` without a comment and upstream ask | Annotation missing in the proto | +| A mirror config struct rather than the embedded SDK type | Schema drifts from the API | +| Fake over ~150 lines, or validating field combinations | Backend policy modelled locally, unverified | +| More than five acceptance directories for one resource | Backend refusal matrix in the tests | +| Invariant template without a matrix entry | Dead test | +| Approval group in `deploy.go` only | Destroy loses the warning | +| Engine or `libs/structs` changes in a resource PR | Workaround for an API gap; split and discuss | + +## 5. Write the comments + +Every confirmed problem gets a comment; no target count. Each comment points at one line, states +the behaviour with a concrete input and outcome, names the precedent or rule it relies on, and ends +with the change or the question. One to three sentences, plain, no headings or severity labels +inside the comment. Prefer a question where the author may know something you do not. No praise, +no verdict, no summary. + +Order: redesign and scope first, then correctness, then wiring and tests, then generated files and +wording. Raise each issue once and list other locations inside that comment. The review body +carries cross-cutting asks only: the API changes to request, the split, the design decision to +make explicit. + +Before printing, re-check every comment against the code, drop any that misread it, and make sure +every worksheet problem row is either a comment or listed under "Not raised" with the reason. + +## 6. Output + +Print, do not post: + +``` +## Inline comments + +### path/to/file.go:LINE + + +## Review body + + +## Evidence (not for posting) +- path/to/file.go:LINE: + +Not raised: +- : + +## Worksheet (not for posting) + +``` + +Use line numbers from `TARGET`. Only post if asked, and never to a PR you were told to dry-run. diff --git a/AGENTS.md b/AGENTS.md index 02b63df8169..9ea0a5adf34 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -90,6 +90,7 @@ GIT_EDITOR=true GIT_SEQUENCE_EDITOR=true VISUAL=true GIT_PAGER=cat git rebase or - Use `./task test-update` to regenerate acceptance test outputs after changes. - The CLI binary supports both `databricks` and `pipelines` command modes based on executable name. +- Adding a new bundle resource type: follow `.agents/skills/author-new-resource/SKILL.md`, which starts with an API readiness gate and ends with a one-PR checklist. Reviewing such a PR: use `.agents/skills/review-new-resource/SKILL.md`. **Stale golangci-lint cache across worktrees.** Triggered by `task lint` or a direct `go tool ... golangci-lint` run reporting issues in files under a *different* or deleted worktree, often with `no such file or directory` warnings. Fix — clear the cache and re-run, don't investigate the reported issues: diff --git a/bundle/direct/dresources/README.md b/bundle/direct/dresources/README.md index 5487042c965..537d4640539 100644 --- a/bundle/direct/dresources/README.md +++ b/bundle/direct/dresources/README.md @@ -1,12 +1,16 @@ # Guidelines on implementing a resource +Adding a new resource type? Follow `.agents/skills/author-new-resource/SKILL.md`: it starts with an +API readiness check, then walks every registration point, generator and test with a verification +command. Reviewers use `.agents/skills/review-new-resource/SKILL.md`. This README is the contract +those skills apply. + ## Core constraints - See adapter.go on what methods are needed and what constraints are present. - Return SDK errors directly, no need to wrap them. Things like current operation, resource key, id are already added by the caller and will be part of the error message. - Although the arguments to resource methods like DoCreate are pointers, they are never nil, so nil checks are not needed. The passed id argument is never empty string. - - When returning id from DoCreate() and from DoUpdateWithID() there is no need to check that returned id is non-empty, this will be done by the framework and converted to error. - An exception could be made if default error message lacks the necessary context. + - DoCreate() and DoUpdateWithID() must return a non-empty id. An empty id would be saved as state and the next plan would create the resource again. Take the id from the create response or operation metadata; if the API can complete without returning one, return an error that says so. - The arguments point to actual struct that will be persisted in state, any changes to it will affect what is stored in state. Usually there is no need to change it, but if there is, there should always be detailed explanation. - Each Create/Update/Delete method should correspond to one API call. We persist state right after, so there is minimum chance of having orphaned resources. - We should calculate the update type during plan phase. This means it should be configured via the resource's YAML file as much as possible, falling back to OverrideChangeDesc(). The DoUpdate() implementation should be as predictable as possible based on the plan. In particular, avoid reading remote state in DoUpdate() to decide what kind of update to dod. @@ -41,12 +45,22 @@ If a resource has fields that must not be sent in updates (deploy-only, lifecycl ## Async APIs: WaitAfterCreate / WaitAfterUpdate -For resources whose create or update is asynchronous (the resource is not immediately ready after the call returns), implement `WaitAfterCreate` and/or `WaitAfterUpdate` instead of polling inline inside DoCreate/DoUpdate. These are the correct extension points in the framework, and polling inline bypasses state persistence timing. +For resources whose create or update is asynchronous (the resource is not immediately ready after the call returns), implement `WaitAfterCreate` and/or `WaitAfterUpdate` instead of polling inline inside DoCreate/DoUpdate. These are the correct extension points in the framework, and polling inline bypasses state persistence timing: state is saved when DoCreate returns, so an interrupted inline wait leaves an untracked resource. + +Some older resources wait inline because their API returns the id only when the operation completes. Do not copy them. If the API you are onboarding has that shape, ask for the id in the operation metadata, and if you must wait inline in the meantime, say so in a comment on the method. + +## No update, or a partial update + +If the API has no update call, do not implement `DoUpdate`; declare every settable field under `recreate_on_changes` (or `provided_id_fields`) so any change plans a recreate. `TestNoUpdateResourcesCoverAllFields` enforces this. + +If the API's update can change only a subset of what create accepts, the rest must be `IMMUTABLE` upstream so the generated rules classify them. An update path that needs code to decide which changes it can apply is a sign the annotations are missing; fix them rather than classifying in Go. ## Slice ordering: KeyedSlices If the API may return a slice's elements in a different order between calls (e.g., `depends_on` in job tasks, `privileges` in grants), implement `KeyedSlices` to compare elements by a natural key rather than by index. Without this, every deploy after any reordering shows phantom diffs. +The key function returns `(fieldName, value)`, and `fieldName` must be a real field of the element: the planner resolves the resulting paths (`tasks[task_key='x'].field`) by looking that field up in the state and remote values. A key assembled from several fields, or a field name that does not exist on the element, pairs elements for the diff but can never be looked up, so every generated rule under that list stops applying. If the API gives list elements no single client-settable identity, the fix is a key field in the API (the pattern of `task_key` on job tasks), not a composite key here and not an engine change. + ## Empty states: IsEmptyState If a desired state describes no resource at all (e.g. an empty grants list), implement `IsEmptyState`. The planner omits such a node instead of planning a create, and apply drops its state entry instead of persisting one. @@ -107,6 +121,8 @@ in normal drift detection and is no longer subject to the `missing_in_remote` su Use `OverrideChangeDesc` only as a last resort when the `.yml` settings cannot express the needed logic. Skipping an action with `change.Action = deployplan.Skip` in `OverrideChangeDesc` creates a silent no-op: the plan shows no change even if the user's config differs from remote. Document the skip reason clearly in both the comment and `change.Reason`. +A new resource should not need it. When one seems to, the cause is almost always upstream: a missing `IMMUTABLE`, `OUTPUT_ONLY` or `INPUT_ONLY` annotation, an update endpoint that cannot replace the resource, or list elements without a key field. Raise those with the API team before writing an override, and if an override still lands, the PR description must say which yml rule it replaces and why that rule could not be written. + ## Nice to have - Add link to corresponding API documentation before each method. - Add link to corresponding terraform resource implementation at the top of the file. @@ -114,14 +130,15 @@ Use `OverrideChangeDesc` only as a last resort when the `.yml` se ## Testing - Make sure to implement CRUD for testserver in libs/testserver + - Model what the CLI's plan and apply depend on: create with server-assigned fields, get, update with the mask the adapter sends, delete, and JSON error bodies with `error_code`. Do not model backend validation policy; cover service behaviour with the cloud test. Recent resource fakes are 70 to 100 lines. - Test first with go test ./bundle/direct/dresources - - You might need to add test fixture in all\_test.go + - Add a `testConfig` fixture in all\_test.go with every field populated; without one the CRUD test runs with a zero config and proves nothing. - Implement acceptance tests in acceptance/bundle/resources// - - See acceptance/bundle/resources/volumes - - Prefer smaller tests for each operation. - - Make sure bundle deploy/plan/debug plan/summary/destroy are covered + - Canonical set: `basic`, `lifecycle` (with `Cloud = true`), `remote-delete`, plus one directory per plan behaviour the resource adds. Copy the most recent resource's directories. + - Make sure bundle deploy/plan/debug plan/summary/destroy are covered, and that an immutable change is deployed with `--auto-approve`, not only planned. - Add an invariant test config in acceptance/bundle/invariant/configs/.yml.tmpl - See existing configs in that directory for the format. + - Register the file under `EnvMatrix.INPUT_CONFIG` in acceptance/bundle/invariant/test.toml, add exclusions for legs it cannot run (`DMS=true` if the type is not in the deployment-history allowlist, cloud if prerequisites are missing), and exclude it in `invariant/migrate` and `invariant/auto-migrate`. A config that is not in the matrix never runs; prove it ran with `go test ./acceptance -run 'TestAccept/bundle/invariant/no_drift' -v -count=1 | grep INPUT_CONFIG=`. - Add bind/unbind tests in acceptance/bundle/deployment/bind// - These verify that binding an existing resource and then deploying/destroying works correctly. - - For new resource types, run at least one test on a live cloud environment. + - For new resource types, run at least one test on a live cloud environment, and say in the PR which test and which environment. diff --git a/bundle/direct/dresources/adapter.go b/bundle/direct/dresources/adapter.go index 80ece24b2be..0d946968d8f 100644 --- a/bundle/direct/dresources/adapter.go +++ b/bundle/direct/dresources/adapter.go @@ -70,6 +70,9 @@ type IResource interface { DoDelete(ctx context.Context, id string, state any) error // [Optional] OverrideChangeDesc can implement custom logic to update a given ChangeDesc; it is run last after built-in classifiers and field triggers. + // It is a last resort for behaviour the rules in configs/.yml cannot express. A new resource + // should not need it: when it seems to, the cause is usually a missing proto annotation (IMMUTABLE, + // OUTPUT_ONLY, INPUT_ONLY) or an API shape the engine cannot diff, and the fix belongs upstream. OverrideChangeDesc(ctx context.Context, path *structpath.PathNode, changedesc *ChangeDesc, remoteState any) error // DoCreate creates a new resource from the newState. Returns id of the resource and optionally remote state. @@ -96,6 +99,8 @@ type IResource interface { DoResize(ctx context.Context, id string, newState any, entry *PlanEntry) error // [Optional] WaitAfterCreate waits for the resource to become ready after creation. Returns optionally updated remote state. + // State is persisted between DoCreate and this call, so an interrupted wait leaves a tracked resource. + // Polling inside DoCreate instead delays that persistence and can orphan the resource. // TODO: wait status should be persisted in the state. WaitAfterCreate(ctx context.Context, id string, newState any) (remoteState any, e error) @@ -109,6 +114,11 @@ type IResource interface { WaitAfterDelete(ctx context.Context, id string) error // [Optional] KeyedSlices returns a map from path patterns to KeyFunc for comparing slices by key instead of by index. + // A KeyFunc returns (fieldName, value). fieldName must be a real field of the element: the planner + // later resolves paths like tasks[task_key='x'] by looking that field up in state and remote values, + // so a key composed from several fields, or named after a field that does not exist, cannot be + // resolved and silently disables the rules under that path. If the API gives list elements no + // single client-settable identity, that is an API gap to raise upstream, not something to encode here. // Example: func (*ResourcePermissions) KeyedSlices(state *PermissionsState) map[string]any KeyedSlices() map[string]any