Repository navigation
Terraform conformance test, UC grants, workspace-admin belt, legacy SQL - #19
Merged
Merged
Conversation
…acy SQL
Coverage 17.8% -> 22.9% of the endpoints the databricks-sdk calls (208 -> 267).
CLI command groups working end to end: 21 -> 30.
The headline number understates things, and the roadmap now says so: 604 of
the missing endpoints are in blocks this project deliberately skips (MLflow,
Account API, identity v2, marketplace, Genie, Lakebase, vector search,
serving, clean rooms, apps). Against the reachable surface it is 267/564,
about 47%.
Terraform conformance
---------------------
docs/terraform.md promised the provider worked against minilake. Nothing
verified it. tests/terraform/ now runs the real provider through
apply -> plan -> destroy over a 15-resource config, asserting the second plan
is empty, with terraform and a pinned provider mirror baked into
Dockerfile.test so the suite still needs no network.
Its first run found three defects every SDK-driven test had missed, because
they are in behaviour only the provider exercises:
- The provider posts `"entitlements": [{}]` for a group with none. minilake
echoed the empty entry back and broke the provider on its own input
(`Invalid address to set: []string{""}`). Real Databricks drops valueless
entries from multi-valued SCIM attributes; now so does minilake.
- SQL warehouses never reported auto_stop_mins, enable_photon,
max_num_clusters or odbc_params, so every `terraform plan` showed a
permanent in-place update. They are now stored and returned with the real
API's defaults. None of them changes how a statement executes.
- SCIM ignored the `attributes` projection the provider asks for.
Permanent drift is worse than a 501: it looks like it works until someone
runs plan twice. That is what the empty-plan assertion is for.
New endpoints
-------------
- UC grants (w.grants.*, databricks_grants) with real inheritance: a grant on
a catalog is effective on its schemas and tables, tagged with its source.
Without that, get_effective would just be get under another name.
- The workspace-admin belt: git credentials, IP access lists, global init
scripts, notification destinations, instance profiles, workspace-conf.
Individually dull, collectively what a terraform apply hits before it
reaches anything interesting.
- Legacy /api/2.0/preview/sql/*, as adapters over the modern stores rather
than a second copy — an object created through either surface is visible
from the other. Includes the operator translation (a legacy ">" that
reached the evaluator untranslated would leave an alert that never fires)
and the correct trash semantics: DELETE trashes, POST .../trash/{id}
restores.
Legacy list endpoints are page-numbered because they have to be: the SDK
walks them by incrementing `page` until a response comes back empty, so a
handler that ignores the parameter never terminates.
Nothing added here is enforced. Grants, secret ACLs, IP access lists and
cluster policies are stored and read back faithfully and checked nowhere. A
terraform plan that applies cleanly says the shape of a config is right, not
that its access control is.
Coverage harness fix
--------------------
It scored all 8 Files API endpoints as missing. The SDK writes
/api/2.0/fs/files{path} with the parameter carrying its own leading slash,
and the normalizer produced a path matching no route. Those two paths are the
only ones in the SDK shaped that way.
Tests: 378 passing, up from 338.
Owner
Author
|
LGTM! 🚀 |
A002
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #17 and #18, and a change of measure.
Coverage: 17.8% → 22.9% of the endpoints the
databricks-sdkcalls (208 → 267).CLI command groups working end to end: 21 → 30.
First: the headline number was misleading, including in my own roadmap
604 of the "missing" endpoints are in blocks this project has deliberately decided not
to emulate — MLflow, the Account API, identity v2, marketplace, Genie, Lakebase, vector
search, serving, clean rooms, apps. Counting those as gaps makes progress look
permanently hopeless.
Against the reachable surface, coverage is 267 of 564 — about 47%. The roadmap
now leads with that and says why.
The main event: Terraform is now tested, not asserted
docs/terraform.mdpromised the provider worked against minilake. Nothing verifiedit — there was no Terraform test anywhere in the repo, and the services I added in #17
(SCIM, tokens, policies, pools) were specifically Terraform-facing resources I had no
way to check.
tests/terraform/now runs the real provider throughapply→plan→destroyovera 15-resource config, asserting the second plan is empty.
terraformand a pinnedprovider mirror are baked into
Dockerfile.test, so the suite still needs no network.Its first run found three defects that every SDK-driven test in the suite had missed,
because they live in behaviour only the provider exercises:
"entitlements": [{}]for a group with none; minilake echoed the empty entry back and broke the provider on its own input (Invalid address to set: []string{""})[{}]. Real Databricks drops valueless entries from multi-valued SCIM attributes; now so does minilakeauto_stop_mins,enable_photon,max_num_clustersorodbc_params, so everyterraform planshowed a permanent in-place updateattributesprojection the provider requestsPermanent drift is worse than a 501 — it looks like it works until someone runs
plantwice. That is exactly what the empty-plan assertion is for, and why it is the sharp
assertion in that file rather than the resource-by-resource ones.
New endpoints
w.grants.*,databricks_grants) with real inheritance: a granton a catalog is effective on its schemas and tables, tagged with the ancestor it came
from. Without that,
get_effectivewould just begetunder another name — worsethan not implementing it, because it would look right while hiding the one behaviour
it exists for.
notification destinations, instance profiles,
workspace-conf. Individually dull;collectively what a realistic
terraform applytouches before it reaches anythinginteresting, where one 501 fails the whole plan.
/api/2.0/preview/sql/*as adapters over the modern stores, not asecond copy: an object created through either surface is visible and editable from
the other. A separate implementation would have passed every single-surface test and
still been wrong.
Three things 8000 that look like details and are not
The legacy list endpoints must paginate. The SDK walks them by incrementing
pageuntil a response comes back empty. A handler that ignores the parameter and returns
everything never terminates — the symptom is a hung test suite, not a failing one.
DELETEtrashes andPOST .../trash/{id}restores, not the other way round. I hadit backwards first; implemented that way, a client asking to restore would destroy.
The SDK sends
securable_typeas anEnumthat is not astrsubclass, so thewire carries
SecurableType.CATALOG, notCATALOG. My local smoke test passed a plainstring and hid this; the Docker run caught it as three silent failures — grants stored
under an unrecognised key, inheritance finding nothing, validation skipped. Both forms
are now accepted.
What this does not give you
Nothing added here is enforced. Grants, secret ACLs, IP access lists and cluster
policies are stored and read back faithfully and checked nowhere; no IP is blocked, no
init script runs, no notification is sent. A
terraform planthat applies cleanlyagainst minilake says the shape of your config is right, not that its access control
is.
docs/terraform.mdnow says this explicitly, because a table of working resourcesinvites the opposite inference.
Also: a bug in my own coverage harness
It scored all 8 Files API endpoints as missing — they are implemented and tested. The
SDK writes
/api/2.0/fs/files{path}with the parameter carrying its own leading slash,and my normalizer produced
fs/files__probe__, matching no route. Those two paths arethe only ones in the SDK shaped that way. Fixed, and
docs/CLI_COVERAGE.mdregenerated.Testing
378 passing, up from 338. Docker suite green, ruff clean.
Running pytest on the host without
terraforminstalled skips the conformance testsrather than failing.