bundle: fix genie_space permissions URL prefix (stacked on #5282)#5327
Closed
longchass wants to merge 21 commits into
Closed
bundle: fix genie_space permissions URL prefix (stacked on #5282)#5327longchass wants to merge 21 commits into
longchass wants to merge 21 commits into
Conversation
Adds first-class support for Genie spaces as a bundle resource,
complete with CRUD via direct-mode deploy, `bundle generate genie-space`
to import an existing space, permissions handling, and acceptance tests.
The resource configuration follows the dashboards pattern: a `file_path`
field points to a local `.genie.json` file whose contents are inlined
into `serialized_space` during deployment. The parent_path defaults to
`${workspace.resource_path}` and is normalized to the `/Workspace`
prefix, matching the API's expected form.
Co-authored-by: Isaac
…ng-parent-path errors Genie surfaces a missing parent folder inconsistently across environments: some workspaces return a standard 404 missing-resource error, while others return 400 INVALID_PARAMETER_VALUE with a NOT_FOUND "Tree node ... does not exist" message embedded in the text. Treat both forms as "create the parent directory and retry once". Co-authored-by: Isaac
…sitor VisitGenieSpacePaths existed but was never called by VisitPaths, so NormalizePaths did not rewrite genie_space file_path values from "relative to YAML location" to "relative to bundle root" before applyGenieSpaceTranslations resolved them. The result was that generator output like "../src/<name>.genie.json" failed on deploy with "path ... is not contained in sync root path". Co-authored-by: Isaac
Inline YAML serialized_space stayed as a structured value (map[string]any with int leaves) in the config struct, while state held the JSON string that was sent to the API. structdiff compared an `any` field with reflect.DeepEqual, which reports map != string, so every plan after deploy showed a false update for the genie_space. Marshal inline serialized_space to its JSON string in ConfigureGenieSpaceSerializedSpace, mirroring the file_path code path, so config-side and state-side carry the same type. The genie_space_complex validate test is updated to reflect that serialized_space is now a string regardless of input form, and a new acceptance test under resources/genie_spaces/inline asserts that a deploy + plan cycle is drift-free for inline serialized_space. Co-authored-by: Isaac
Databricks workspaces do not expose a permissions endpoint for Genie Spaces (PUT /permissions/genie/spaces/<id> returns 404 ENDPOINT_NOT_FOUND). Without an upfront check the deploy creates the space first and then errors when applying permissions, leaving partial state behind. Add ValidateGenieSpacePermissions to the PreDeployChecks pipeline so both per-resource permissions and bundle-level permissions propagated by ApplyBundlePermissions surface a clear validation error before any API call is made. Co-authored-by: Isaac
Two minor follow-ups to the genie_spaces work: - ConfigureGenieSpaceSerializedSpace silently let file_path win when a user also set serialized_space inline. Emit a warning that points at the inline block so the user knows their YAML is being dropped on the floor. - ValidateDirectOnlyResources only mentioned the DATABRICKS_BUNDLE_ENGINE env var as a way to opt into direct mode, even though 'bundle.engine: direct' in databricks.yml is the more common entry point. Mention both. Co-authored-by: Isaac
When the Genie API returns parent_path on GetSpace, propagate it through bundle generate genie-space so the produced YAML deploys back to the same workspace folder. The testserver is updated to mirror that response shape so the acceptance fixture exercises the new path. Filter ParentPath out of ForceSendFields in DoRead and responseToGenieSpaceConfig: we deliberately clear ParentPath in the returned GenieSpaceConfig because the GET API does not reliably include it, but the SDK still surfaces it in ForceSendFields when the field appeared on the wire. Without this filter, deploy state serialization force-emits parent_path: "" even though the field is logically unset, producing spurious output diffs. Co-authored-by: Isaac
- Replace switch-with-fallthrough on dyn.Kind with a guard clause to satisfy the exhaustive linter without listing every Kind variant. - Use http.StatusBadRequest in isMissingGenieParentPathError instead of a magic 400 (auto-fix from golangci-lint). Co-authored-by: Isaac
…cally serialized_space is in ignore_remote_changes because we cannot diff a structured local YAML body against a remote JSON string. That makes UI edits invisible at plan time, but the unconditional UpdateSpace request was still sending the local body, so any later update to title or description would silently overwrite UI changes. Use the plan entry to detect whether the user actually changed serialized_space locally; only include it in the update request when the change is an Update action (not a Skip from ignore_remote_changes). Co-authored-by: Isaac
The previous implementation polled w.Workspace.GetStatusByPath using resource.FilePath, which is the local relative path (e.g. "src/foo.genie.json"). Both lookups (with and without the "/Workspace" prefix) were invalid for the workspace API, so currentModified stayed at 0 and the file never updated past the first iteration. Genie has no remote modification timestamp on the response, so use content comparison instead: canonicalize the just-fetched serialized_space and compare against the on-disk body, re-saving only when they differ. The first iteration still always saves, preserving the prior unconditional initial sync. Co-authored-by: Isaac
The parent generate command exposes --key as a persistent flag, but the genie-space subcommand was always deriving the key from the remote title. Read the flag value and fall back to the title-derived key only when not provided. Co-authored-by: Isaac
Calling json.Unmarshal on an empty serialized_space surfaces a confusing "unexpected end of JSON input" error and writes nothing useful. Bail out early with a clear message that names the target file. Co-authored-by: Isaac
DoRead duplicated the field copy and the ParentPath-drop comment that already lives in responseToGenieSpaceConfig. Reuse the helper directly so the two stay in sync. Co-authored-by: Isaac
The user-facing fields (title, description, warehouse_id, parent_path, file_path, serialized_space) had PLACEHOLDER descriptions, leaving the generated reference and resources docs blank. Fill them in with short descriptions and regenerate the schema and docs output. Co-authored-by: Isaac
Lowercase the genie_space error message to satisfy ST1005 and let the
linter convert an empty []string{} to a nil slice.
Co-authored-by: Isaac
The simple acceptance test fixture was a v1 serialized_space sample that
the Genie backend now rejects with 409 ABORTED ("The export format has
changed since this export was taken"). Bumps version to 2 and replaces
get_example_values / build_value_dictionary with the v2-equivalent
enable_format_assistance / enable_entity_matching, matching the format
that bundle generate genie-space now produces.
Co-authored-by: Isaac
The state DB API gained context, withRecovery and withWrite arguments on origin/main; mirror the dashboard generate command and use the same arguments. Also regenerates the simple acceptance plan output to pick up the WAL-implementation serial increment. Co-authored-by: Isaac
Commit e5af0e6 bundled an unrelated change to ValidateDirectOnlyResources alongside the genie_space dual-source warning. The error-message tweak doesn't belong in this PR; reverting those three files (incl. the catalog acceptance output that reflected the old text) so the PR stays scoped to genie work. The tweak can land in a separate PR if still desired. Co-authored-by: Isaac
The genie_space permissions prefix was registered as "/genie/spaces/",
which parsePermissionsID interprets as 4 segments and routes to
PUT /api/2.0/permissions/genie/spaces/<id>. That endpoint does not
exist; the workspace API returns 404 ENDPOINT_NOT_FOUND, which is what
motivated the earlier rejection mutator.
The actual permissions path is /api/2.0/permissions/genie/<id>
(3 segments, RequestObjectType="genie"). This matches:
- the SDK iam path builder at service/iam/impl.go (uses
"/api/2.0/permissions/%v/%v")
- the CLI's own "databricks permissions get/set genie <id>" command,
which lists "genie" as a valid REQUEST_OBJECT_TYPE
(cmd/workspace/permissions/permissions.go)
Fix the bundle prefix and the matching testserver whitelist entry,
which both had the same wrong path and therefore masked the bug in
acceptance tests. Regenerate the affected acceptance fixtures.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Approval status: pending
|
Author
|
@janniklasrose Hi, just proposing this fix on top of your PR, cheers. |
janniklasrose
force-pushed
the
janniklasrose/genie
branch
from
d633f4a to
a595ca4
Compare
Contributor
|
Thanks for the note. Already addressed upstream |
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
2 participants
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.
Summary
Five-line fix on top of #5282. The genie_space permissions prefix was registered as
/genie/spaces/— a 4-segment path thatparsePermissionsIDreads asRequestObjectType="genie/spaces", routing to:That endpoint does not exist (404 ENDPOINT_NOT_FOUND, which is what motivated
41b19bb5b"reject genie_space permissions during plan"). The actual production path is 3-segment:with
RequestObjectType="genie". This matches:service/iam/impl.go(uses"/api/2.0/permissions/%v/%v")databricks permissions get/set genie <id>command —genieis listed as a validREQUEST_OBJECT_TYPEincmd/workspace/permissions/permissions.go:100databricks permissions set genie <id> --json …(I run this in a Genie-space Git-versioning tool against a real workspace daily)The acceptance test passes today only because
libs/testserver/permissions.gohas the same"genie/spaces": "genie-space"entry — both halves of the wire format agree on the wrong path, so production breakage is invisible in CI.If #5282 ships with
/genie/spaces/, any deploy withpermissions:on a genie_space will 404 against production. Sincee4809d65a"Agent review" already deleted the rejection mutator and added the positive test, this just makes the positive test exercise the right URL.Changes (5 lines)
bundle/direct/dresources/permissions.go"/genie/spaces/"→"/genie/"libs/testserver/permissions.go"genie/spaces"→"genie"bundle/direct/dresources/all_test.goObjectIDmatches new prefixacceptance/.../current_can_manage/out.requests.deploy.direct.json-update:PUT /api/2.0/permissions/genie/[FOO_ID]acceptance/.../current_can_manage/out.plan.direct.json/genie/${...id}Test plan
go test ./bundle/... ./libs/testserver/...— passgo test ./acceptance -run 'TestAccept/bundle/resources/permissions/genie_spaces'— pass after-update./task lint-q— 0 issuesDrive-by observation (out of scope for this PR)
Independent of this fix, two acceptance fixtures on #5282 reproduce as stale without my changes:
acceptance/bundle/resources/genie_spaces/simple/out.plan.json—serial: 7vs actual1acceptance/bundle/refschema/out.fields.txt— missingresources.genie_spaces.*.etagBoth look like they need
./task test-updatefrom a clean rebase. Flagging here rather than fixing — out of scope.Closes when
#5282 lands. Either cherry-pick
6767037directly, or this becomes a normal PR againstmainafter #5282 merges.