Added Databricks SQL Warehouses API actions #18148
Conversation
- Added create SQL warehouse action - Added list SQL warehouses action - Added get SQL warehouse action - Added edit SQL warehouse action - Added delete SQL warehouse action - Added start/stop SQL warehouse actions - Added get/set SQL warehouse config actions - Added get/set SQL warehouse permissions actions Implements 11 SQL Warehouses endpoints as discussed in the enhancement issue.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
WalkthroughAdds Databricks SQL Warehouse support: a Changes
Sequence Diagram(s)sequenceDiagram
actor U as User
participant A as Action
participant D as Databricks App
participant API as Databricks HTTP API
U->>A: Provide props (e.g., name, warehouseId, config)
A->>A: Validate & build payload (if applicable)
A->>D: Call app method (data/payload, $)
D->>API: HTTP request to /sql/warehouses, /sql/config/..., or /permissions/warehouses/...
API-->>D: Response
D-->>A: Response
A->>U: Export "$summary" and return response
sequenceDiagram
rect rgba(230,255,230,0.12)
note right of A: Create SQL Warehouse flow
actor U as User
participant A as Create Action
participant D as Databricks App
participant API as POST /sql/warehouses
U->>A: name, clusterSize, optional params
A->>A: validate autoStop/min/max, build payload
A->>D: createSQLWarehouse({ data: payload, $ })
D->>API: POST /sql/warehouses
API-->>D: { id, name, ... }
D-->>A: API response
A->>U: $summary "Successfully created SQL Warehouse: <name>" and return response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you so much for submitting this! We've added it to our backlog to review, and our team has been notified. |
|
Thanks for submitting this PR! When we review PRs, we follow the Pipedream component guidelines. If you're not familiar, here's a quick checklist:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/databricks/databricks.app.mjs (1)
49-51: Base URL hardcodes AWS host; will fail for Azure and GCP workspacesThe
_baseUrl()assumes<subdomain>.cloud.databricks.com, which breaks for:
- Azure:
adb-<ws-id>.<rand>.azuredatabricks.net- GCP:
<ws-id>.<last-digit>.gcp.databricks.comRecommend accepting a full host from auth and only falling back to the AWS pattern if a bare instance name is provided.
Apply this diff to make the host flexible across clouds and avoid double-host bugs:
- _baseUrl() { - return `https://${this.$auth.domain}.cloud.databricks.com/api/2.0`; - }, + _baseUrl() { + const domainOrHost = this.$auth.host || this.$auth.domain; + if (!domainOrHost) { + throw new Error("Databricks host/domain is not configured"); + } + const hasScheme = /^https?:\/\//i.test(domainOrHost); + const isFqdn = domainOrHost.includes("."); + // Accept full host (any cloud) if it looks like a URL or FQDN; else treat as AWS short name. + const base = hasScheme + ? domainOrHost.replace(/\/+$/, "") + : (isFqdn ? `https://${domainOrHost}` : `https://${domainOrHost}.cloud.databricks.com`); + return `${base}/api/2.0`; + },
- Azure per-workspace URL format (azuredatabricks.net). (learn.microsoft.com)
- GCP workspace URL format (gcp.databricks.com). (docs.databricks.com, docs.gcp.databricks.com)
🧹 Nitpick comments (19)
components/databricks/databricks.app.mjs (1)
158-172: Consider adding “update” permissions (PATCH) supportYou added GET/PUT on
/permissions/warehouses/{warehouseId}. Databricks also supports incremental “update-permissions” (PATCH), which is useful for partial changes without replacing the ACL. Suggest adding aupdateSQLWarehousePermissionsmethod and a corresponding action.
- CLI includes both set-permissions and update-permissions. (docs.databricks.com)
Possible addition:
+ updateSQLWarehousePermissions({ warehouseId, ...args }) { + return this._makeRequest({ + path: `/permissions/warehouses/${warehouseId}`, + method: "PATCH", + ...args, + }); + },Also, your current permissions path
/permissions/warehouses/{id}matches Databricks guidance. (kb.databricks.com)components/databricks/actions/create-sql-warehouse/create-sql-warehouse.mjs (3)
6-6: Docs URL likely unstable; point to a durable referenceThe link
https://docs.databricks.com/api/workspace/warehouse/createmay redirect or 404 depending on doc versioning. Prefer linking to the SQL Warehouses API or the CLI reference section that consistently lists operations.
- General SQL Warehouses docs and admin settings pages (stable entry points). (docs.databricks.com)
- CLI “warehouses” command group (lists create/edit/start/stop, etc.). (docs.databricks.com)
16-20: Constrain Cluster Size to known values to prevent API errorsCluster size accepts a fixed set of strings (e.g., 2X-Small, X-Small, Small, Medium, Large, X-Large, 2X-Large, 3X-Large, 4X-Large). Expose these as
options()to reduce typos.- clusterSize: { - type: "string", - label: "Cluster Size", - description: "Size of the cluster (e.g., `Small`, `Medium`, `Large`)", - }, + clusterSize: { + type: "string", + label: "Cluster Size", + description: "Size of the cluster", + options() { + return [ + "2X-Small", "X-Small", "Small", "Medium", + "Large", "X-Large", "2X-Large", "3X-Large", "4X-Large", + ]; + }, + },Reference size table. (docs.databricks.com)
21-27: Don’t send unset values; align with workspace defaults
- Defaulting
auto_stop_minsto 10 may override workspace defaults (UI defaults differ by type). Consider omitting the field when the user leaves it blank.Apply this diff to only include fields when provided, and to support serverless and type selection:
props: { databricks, name: { ... }, clusterSize: { ... }, - autoStopMinutes: { + autoStopMinutes: { type: "integer", label: "Auto Stop (minutes)", description: "Number of minutes of inactivity before the warehouse auto-stops", - optional: true, - default: 10, + optional: true, }, + warehouseType: { + type: "string", + label: "Warehouse Type", + description: "CLASSIC or PRO", + optional: true, + options: ["CLASSIC", "PRO"], + }, + enableServerlessCompute: { + type: "boolean", + label: "Enable Serverless Compute", + description: "When true, set type to PRO and enable serverless (if supported in your region)", + optional: true, + }, }, async run({ $ }) { - const payload = { - name: this.name, - cluster_size: this.clusterSize, - auto_stop_mins: this.autoStopMinutes, - }; + const payload = { + name: this.name, + cluster_size: this.clusterSize, + ...(this.autoStopMinutes != null && { auto_stop_mins: this.autoStopMinutes }), + ...(this.warehouseType && { warehouse_type: this.warehouseType }), + ...(this.enableServerlessCompute != null && { enable_serverless_compute: this.enableServerlessCompute }), + }; + // If serverless is enabled and type not set, default to PRO per Databricks guidance + if (payload.enable_serverless_compute && !payload.warehouse_type) { + payload.warehouse_type = "PRO"; + }Serverless parameter guidance. (docs.databricks.com)
Also applies to: 30-34
components/databricks/actions/get-sql-warehouse-config/get-sql-warehouse-config.mjs (1)
6-6: Docs link nit“Get SQL Warehouse Config” page locations can vary; consider linking to the warehouse admin settings page (which refers readers to the API) or the CLI
get-workspace-warehouse-configcommand reference.
- Admin settings overview and CLI get/set workspace warehouse config. (docs.databricks.com, learn.microsoft.com)
components/databricks/actions/stop-sql-warehouse/stop-sql-warehouse.mjs (1)
11-15: Add dropdown options to reduce ID entry errorsConsider providing
options()that lists warehouses (id → name) forwarehouseId. This reduces manual ID mistakes and mirrors other Pipedream actions’ UX.warehouseId: { type: "string", label: "Warehouse ID", description: "The ID of the SQL Warehouse to stop", + async options() { + const { warehouses } = await this.databricks.listSQLWarehouses({ $: this }); + return (warehouses || []).map(({ id, name }) => ({ value: id, label: `${name} (${id})` })); + }, },The list endpoint is
/api/2.0/sql/warehouses. (kb.databricks.com)components/databricks/actions/delete-sql-warehouse/delete-sql-warehouse.mjs (1)
11-15: Add dropdown options for Warehouse IDSame UX suggestion as “stop”: expose an options list from
listSQLWarehousesto minimize ID copy/paste errors.warehouseId: { type: "string", label: "Warehouse ID", description: "The ID of the SQL Warehouse to delete", + async options() { + const { warehouses } = await this.databricks.listSQLWarehouses({ $: this }); + return (warehouses || []).map(({ id, name }) => ({ value: id, label: `${name} (${id})` })); + }, },components/databricks/actions/start-sql-warehouse/start-sql-warehouse.mjs (1)
9-16: Consider propDefinition with autocomplete for Warehouse ID.Reusing a shared propDefinition improves UX (autocomplete) and consistency across actions.
Apply this diff if your app exposes a reusable definition:
props: { - databricks, - warehouseId: { - type: "string", - label: "Warehouse ID", - description: "The ID of the SQL Warehouse to start", - }, + databricks, + warehouseId: { + propDefinition: [ + databricks, + "warehouseId", + ], + description: "The ID of the SQL Warehouse to start", + }, },components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs (2)
16-20: Guard against empty edits.Calling the API with an empty body will likely 400. Add a quick guard to fail fast with a useful message.
async run({ $ }) { + if (!this.body || Object.keys(this.body).length === 0) { + throw new Error("Please provide at least one field in 'Edit Configuration' to update the warehouse."); + } const response = await this.databricks.editSQLWarehouse({ warehouseId: this.warehouseId, data: this.body, $, });
11-15: Adopt shared Warehouse ID prop for consistency and UX.Same suggestion as other actions: leverage a
propDefinitionwith autocomplete, if available.- warehouseId: { - type: "string", - label: "Warehouse ID", - description: "The ID of the SQL Warehouse to edit", - }, + warehouseId: { + propDefinition: [databricks, "warehouseId"], + description: "The ID of the SQL Warehouse to edit", + },components/databricks/actions/get-sql-warehouse/get-sql-warehouse.mjs (1)
23-24: Enrich the summary with the warehouse name when available.Minor UX improvement so summaries are identifiable at a glance.
- $.export("$summary", `Retrieved details for SQL Warehouse ID ${this.warehouseId}`); + const name = response?.name || this.warehouseId; + $.export("$summary", `Retrieved details for SQL Warehouse ${name}`);components/databricks/actions/list-sql-warehouses/list-sql-warehouses.mjs (3)
20-22: Prefer nullish coalescing over||to respect explicit 0.If 0 is ever valid,
||would override it.??handles only null/undefined.- const params = { - limit: this.maxResults || 50, - }; + const params = { + limit: this.maxResults ?? 50, + };
19-37: Consider pagination options (page/offset or fetch-all).Right now, only the first page is returned. If the app supports
offset(orpage) or an internal paginator, expose it to users, or add afetchAlltoggle to iterate and accumulate results.Example (if the app honors
offset):- async run({ $ }) { - const params = { - limit: this.maxResults ?? 50, - }; + props: { + databricks, + maxResults: { /* …as above… */ }, + page: { + type: "integer", + label: "Page", + description: "0-based page index (translates to offset = page * limit).", + optional: true, + default: 0, + min: 0, + }, + }, + async run({ $ }) { + const params = { + limit: this.maxResults ?? 50, + offset: (this.page ?? 0) * (this.maxResults ?? 50), + };Or, if you prefer
fetchAll: boolean, I can sketch a loop that collects all pages until an empty page is returned.
11-17: ConstrainmaxResultsto Databricks API limitsWe’ve confirmed that the Databricks REST API caps the
limit(i.e.maxResults) at 100 per request. To prevent client-side errors and align with the API’s behavior, please addminandmaxproperties:• File:
components/databricks/actions/list-sql-warehouses/list-sql-warehouses.mjs(around lines 11–17)maxResults: { type: "integer", label: "Max Results", description: "Maximum number of warehouses to return", default: 50, optional: true, + min: 1, + max: 100, // Matches the API’s hard cap per-page limit },This ensures callers can’t request fewer than 1 or more than the API allows (100), avoiding unnecessary round-trip errors and relying on built-in pagination for additional pages.
components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs (2)
11-16: Add a default and clarify expectations for the config object.A default avoids nullish input in UI and clarifies that a non-empty object is required.
Apply this diff:
config: { type: "object", label: "Configuration", - description: "The configuration object for SQL Warehouses. Example: `{ \"enable_serverless_compute\": true }`", + description: "The configuration object for SQL Warehouses. This may overwrite prior global settings. Example: `{ \"enable_serverless_compute\": true }`", + default: {}, },
17-25: Guard against empty updates and emit a more informative summary.Prevents accidental no-op (or unintended overwrite if the API treats empty objects specially) and surfaces which keys were updated.
Apply this diff:
async run({ $ }) { - const response = await this.databricks.setSQLWarehouseConfig({ - data: this.config, - $, - }); - - $.export("$summary", "Successfully updated SQL Warehouse configuration"); - return response; + if (!this.config || Object.keys(this.config).length === 0) { + throw new Error("Configuration cannot be empty. Provide at least one field."); + } + const response = await this.databricks.setSQLWarehouseConfig({ + data: this.config, + $, + }); + + const keys = Object.keys(this.config); + $.export( + "$summary", + `Updated SQL Warehouse config (${keys.length} field${keys.length === 1 ? "" : "s"}): ${keys.slice(0, 5).join(", ")}${keys.length > 5 ? ", …" : ""}` + ); + return response; },components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs (3)
16-21: Validate ACL shape and set clear expectations.“Set” likely overwrites the entire ACL. Add a default skeleton and caution in the description to reduce destructive misconfigurations.
Apply this diff:
permissions: { type: "object", label: "Permissions", - description: "The permissions object. Example: `{ \"access_control_list\": [{ \"user_name\": \"[email protected]\", \"permission_level\": \"CAN_MANAGE\" }] }`", + description: "The permissions object to set (overwrites existing ACL). Example: `{ \"access_control_list\": [{ \"user_name\": \"[email protected]\", \"permission_level\": \"CAN_MANAGE\" }] }`", + default: { access_control_list: [] }, },
22-31: Add safety and a more informative summary; prevent accidental ACL replacement.Require explicit confirmation before overwriting a non-empty ACL and surface how many ACEs were applied. This mirrors common “set permissions” guardrails.
Apply this diff:
export default { key: "databricks-set-sql-warehouse-permissions", name: "Set SQL Warehouse Permissions", @@ props: { databricks, warehouseId: { type: "string", label: "Warehouse ID", description: "The ID of the SQL Warehouse to update permissions for", }, permissions: { type: "object", label: "Permissions", description: "The permissions object. Example: `{ \"access_control_list\": [{ \"user_name\": \"[email protected]\", \"permission_level\": \"CAN_MANAGE\" }] }`", }, + confirmOverwrite: { + type: "boolean", + label: "Confirm overwrite", + description: "Set to true to replace existing permissions using the 'set' API. If false and the target ACL is non-empty, the action aborts.", + optional: true, + default: false, + }, }, async run({ $ }) { - const response = await this.databricks.setSQLWarehousePermissions({ + const acl = this.permissions?.access_control_list; + if (!Array.isArray(acl) || acl.length === 0) { + throw new Error("permissions.access_control_list must be a non-empty array."); + } + + if (!this.confirmOverwrite) { + const existing = await this.databricks.getSQLWarehousePermissions({ + warehouseId: this.warehouseId, + $, + }); + const existingCount = Array.isArray(existing?.access_control_list) ? existing.access_control_list.length : 0; + if (existingCount > 0) { + throw new Error("Existing permissions detected. To overwrite, set 'Confirm overwrite' to true."); + } + } + + const response = await this.databricks.setSQLWarehousePermissions({ warehouseId: this.warehouseId, data: this.permissions, $, }); - $.export("$summary", `Successfully updated permissions for SQL Warehouse ID ${this.warehouseId}`); + const count = acl.length; + $.export("$summary", `Updated permissions for SQL Warehouse ${this.warehouseId} (${count} ACE${count === 1 ? "" : "s"})`); return response; },
11-15: Suggest usingpropDefinitions.warehouseIdif availableCurrently,
databricks.app.mjsdoes not export awarehouseIdpropDefinition, so this change is purely optional. Once you’ve added awarehouseIddefinition underpropDefinitions, you can improve UX by letting users select from a dropdown instead of free-text input.• Location:
components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjslines 11–15
• Note: nowarehouseIdincomponents/databricks/databricks.app.mjspropDefinitions blockApply this diff only after defining
warehouseIdindatabricks.app.mjs:- warehouseId: { - type: "string", - label: "Warehouse ID", - description: "The ID of the SQL Warehouse to update permissions for", - }, + // If available in databricks.app.mjs + warehouseId: databricks.propDefinitions?.warehouseId ?? { + type: "string", + label: "Warehouse ID", + description: "The ID of the SQL Warehouse to update permissions for", + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
components/databricks/actions/create-sql-warehouse/create-sql-warehouse.mjs(1 hunks)components/databricks/actions/delete-sql-warehouse/delete-sql-warehouse.mjs(1 hunks)components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs(1 hunks)components/databricks/actions/get-sql-warehouse-config/get-sql-warehouse-config.mjs(1 hunks)components/databricks/actions/get-sql-warehouse-permissions/get-sql-warehouse-permissions.mjs(1 hunks)components/databricks/actions/get-sql-warehouse/get-sql-warehouse.mjs(1 hunks)components/databricks/actions/list-sql-warehouses/list-sql-warehouses.mjs(1 hunks)components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs(1 hunks)components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs(1 hunks)components/databricks/actions/start-sql-warehouse/start-sql-warehouse.mjs(1 hunks)components/databricks/actions/stop-sql-warehouse/stop-sql-warehouse.mjs(1 hunks)components/databricks/databricks.app.mjs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
components/databricks/actions/stop-sql-warehouse/stop-sql-warehouse.mjs (2)
components/databricks/actions/get-sql-warehouse/get-sql-warehouse.mjs (1)
response(18-21)components/databricks/actions/start-sql-warehouse/start-sql-warehouse.mjs (1)
response(18-21)
components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs (3)
components/databricks/actions/create-sql-warehouse/create-sql-warehouse.mjs (1)
response(36-39)components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs (1)
response(23-27)components/databricks/actions/get-sql-warehouse-permissions/get-sql-warehouse-permissions.mjs (1)
response(18-21)
components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs (8)
components/databricks/actions/create-sql-warehouse/create-sql-warehouse.mjs (1)
response(36-39)components/databricks/actions/get-sql-warehouse-config/get-sql-warehouse-config.mjs (1)
response(13-13)components/databricks/actions/get-sql-warehouse-permissions/get-sql-warehouse-permissions.mjs (1)
response(18-21)components/databricks/actions/get-sql-warehouse/get-sql-warehouse.mjs (1)
response(18-21)components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs (1)
response(18-21)components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs (1)
response(23-27)components/databricks/actions/start-sql-warehouse/start-sql-warehouse.mjs (1)
response(18-21)components/databricks/actions/stop-sql-warehouse/stop-sql-warehouse.mjs (1)
response(18-21)
components/databricks/actions/list-sql-warehouses/list-sql-warehouses.mjs (1)
components/databricks/databricks.app.mjs (1)
params(29-32)
components/databricks/actions/create-sql-warehouse/create-sql-warehouse.mjs (8)
components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs (1)
response(23-27)components/databricks/actions/get-sql-warehouse-config/get-sql-warehouse-config.mjs (1)
response(13-13)components/databricks/actions/get-sql-warehouse-permissions/get-sql-warehouse-permissions.mjs (1)
response(18-21)components/databricks/actions/get-sql-warehouse/get-sql-warehouse.mjs (1)
response(18-21)components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs (1)
response(18-21)components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs (1)
response(23-27)components/databricks/actions/start-sql-warehouse/start-sql-warehouse.mjs (1)
response(18-21)components/databricks/actions/stop-sql-warehouse/stop-sql-warehouse.mjs (1)
response(18-21)
components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs (3)
components/databricks/actions/create-sql-warehouse/create-sql-warehouse.mjs (1)
response(36-39)components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs (1)
response(23-27)components/databricks/actions/get-sql-warehouse-config/get-sql-warehouse-config.mjs (1)
response(13-13)
components/databricks/actions/get-sql-warehouse-config/get-sql-warehouse-config.mjs (3)
components/databricks/actions/get-sql-warehouse-permissions/get-sql-warehouse-permissions.mjs (1)
response(18-21)components/databricks/actions/get-sql-warehouse/get-sql-warehouse.mjs (1)
response(18-21)components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs (1)
response(18-21)
components/databricks/actions/get-sql-warehouse/get-sql-warehouse.mjs (7)
components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs (1)
response(23-27)components/databricks/actions/get-sql-warehouse-config/get-sql-warehouse-config.mjs (1)
response(13-13)components/databricks/actions/get-sql-warehouse-permissions/get-sql-warehouse-permissions.mjs (1)
response(18-21)components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs (1)
response(18-21)components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs (1)
response(23-27)components/databricks/actions/start-sql-warehouse/start-sql-warehouse.mjs (1)
response(18-21)components/databricks/actions/stop-sql-warehouse/stop-sql-warehouse.mjs (1)
response(18-21)
components/databricks/actions/start-sql-warehouse/start-sql-warehouse.mjs (8)
components/databricks/actions/create-sql-warehouse/create-sql-warehouse.mjs (1)
response(36-39)components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs (1)
response(23-27)components/databricks/actions/get-sql-warehouse-config/get-sql-warehouse-config.mjs (1)
response(13-13)components/databricks/actions/get-sql-warehouse-permissions/get-sql-warehouse-permissions.mjs (1)
response(18-21)components/databricks/actions/get-sql-warehouse/get-sql-warehouse.mjs (1)
response(18-21)components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs (1)
response(18-21)components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs (1)
response(23-27)components/databricks/actions/stop-sql-warehouse/stop-sql-warehouse.mjs (1)
response(18-21)
components/databricks/actions/get-sql-warehouse-permissions/get-sql-warehouse-permissions.mjs (2)
components/databricks/actions/get-sql-warehouse/get-sql-warehouse.mjs (1)
response(18-21)components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs (1)
response(23-27)
🔇 Additional comments (15)
components/databricks/databricks.app.mjs (3)
93-99: SQL Warehouses endpoints and methods look correct
- Create/List: POST/GET /api/2.0/sql/warehouses
- Start/Stop: POST /api/2.0/sql/warehouses/{id}/start and /stop
These align with Databricks REST API behavior.
- Start/Stop endpoints reference. (learn.microsoft.com)
Also applies to: 114-119, 127-133, 135-141
120-126: Confirm edit endpoint semantics (POST /edit vs PATCH)
/sql/warehouses/{id}/editwithPOSTis consistent with the long-standing pattern, but official pages rarely spell out the HTTP verb. The current Databricks CLI exposes a “warehouses edit” command, which indicates the endpoint exists; please double-check thatPOSTto/editis still preferred in your target regions (some APIs have moved to PATCH for partial updates in recent iterations).
- CLI “warehouses edit” command. (learn.microsoft.com)
143-157: Workspace-level Warehouse Config: ensure required fields are passedPUT
/sql/config/warehousesoften requiresenable_serverless_computein the request body; omitting it can produce 400s. Consider documenting this in the action and validating the payload (or providing a sane default) to prevent user errors.Example server response requiring
enable_serverless_compute. (github.com)components/databricks/actions/create-sql-warehouse/create-sql-warehouse.mjs (1)
36-43: LGTM — action wiring and summaryThe request wiring to
databricks.createSQLWarehouseand the exported$summarylook good.components/databricks/actions/get-sql-warehouse-config/get-sql-warehouse-config.mjs (1)
13-16: LGTM — correct endpoint usage and clean UXThe action simply calls the app’s method and exports a concise summary. Matches the documented GET
/sql/config/warehouses.
- GET config reference via CLI docs; same path used in SDK/issue logs. (learn.microsoft.com, github.com)
components/databricks/actions/stop-sql-warehouse/stop-sql-warehouse.mjs (1)
17-25: LGTM — correct call and user-facing summaryStop action calls POST
/sql/warehouses/{id}/stopappropriately and exports a helpful summary.
- Stop endpoint reference. (learn.microsoft.com)
components/databricks/actions/delete-sql-warehouse/delete-sql-warehouse.mjs (1)
17-25: LGTM — correct deletion flowRuns
DELETE /sql/warehouses/{id}and returns a simple success payload; summary message is clear.components/databricks/actions/start-sql-warehouse/start-sql-warehouse.mjs (1)
17-25: Confirmed startSQLWarehouse implementation
ThestartSQLWarehousemethod is defined in components/databricks/databricks.app.mjs with the correct casing and signature ({ warehouseId, …args }), matching the action’s usage. No changes required.components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs (1)
22-27: LGTM on the core call.The request assembly looks correct, passing
warehouseId,data, and$to the app method.components/databricks/actions/get-sql-warehouse/get-sql-warehouse.mjs (1)
17-25: LGTM on request/response handling.The call signature and summary export follow established action patterns.
components/databricks/actions/get-sql-warehouse-permissions/get-sql-warehouse-permissions.mjs (1)
17-25: LGTM on the permissions retrieval.The request shape is consistent with other actions and exports a clear summary.
components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs (2)
3-8: Solid action scaffold and consistent naming.Key, name, version, and type align with existing actions. Description link is helpful.
1-25: No changes needed—setSQLWarehouseConfigexists and matches the contract.
Verified that incomponents/databricks/databricks.app.mjs,setSQLWarehouseConfig(args = {})is implemented just aftergetSQLWarehouseConfig, targeting the correct/sql/config/warehousesendpoint with HTTP PUT, and accepts arbitraryargs(includingdataand$) as intended.components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs (2)
3-8: Consistent action wiring and metadata.Structure matches the rest of the new actions; versioning and naming are clear.
22-27: Everything checks out: method contracts match usageBoth
setSQLWarehousePermissionsandgetSQLWarehousePermissionsare defined incomponents/databricks/databricks.app.mjswith signatures that accept{ warehouseId, ...args }, and they correctly map to the REST endpoints/permissions/warehouses/${warehouseId}using PUT and GET respectively, so the action’s usage of{ warehouseId, data, $ }and{ warehouseId, $ }is valid.
components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs
Outdated
Show resolved
Hide resolved
components/databricks/actions/get-sql-warehouse-permissions/get-sql-warehouse-permissions.mjs
Outdated
Show resolved
Hide resolved
components/databricks/actions/get-sql-warehouse/get-sql-warehouse.mjs
Outdated
Show resolved
Hide resolved
components/databricks/actions/list-sql-warehouses/list-sql-warehouses.mjs
Outdated
Show resolved
Hide resolved
components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs
Outdated
Show resolved
Hide resolved
components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs
Outdated
Show resolved
Hide resolved
components/databricks/actions/start-sql-warehouse/start-sql-warehouse.mjs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs (1)
6-6: Fix docs URL and clarify “set” semantics (replaces entire ACL).The link likely 404s or points to a non-canonical page. Also, “set” replaces all direct ACL entries; this deserves an explicit warning in the description to prevent accidental revocations.
Apply this diff:
- description: "Updates the permissions for a specific SQL Warehouse. [See docs](https://docs.databricks.com/api/workspace/warehouses/setpermissions)", + description: "Sets (replaces) the direct ACL for a specific SQL Warehouse. Principals omitted from the list will be revoked. [See docs](https://docs.databricks.com/api/workspace/permissions)",To double-check the precise deep link for SQL Warehouses:
Please provide the canonical Databricks REST Permissions API docs URL for setting permissions on a SQL Warehouse (the operation that replaces the access_control_list). Also confirm the accepted identity keys and the current list of valid permission levels as of August 2025.
🧹 Nitpick comments (5)
components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs (5)
12-15: Consider using a propDefinition or async options for Warehouse ID.For better UX and fewer copy/paste errors, prefer a dropdown populated from the API (when available in databricks.app.mjs).
Proposed change (only if the app exposes this prop definition):
- warehouseId: { - type: "string", - label: "Warehouse ID", - description: "The ID of the SQL Warehouse to update permissions for", - }, + warehouseId: { + propDefinition: [databricks, "warehouseId"], + description: "The ID of the SQL Warehouse to update permissions for", + },If not available, consider adding an async options loader to the app to list SQL Warehouses and reuse it here.
16-29: Enforce “exactly one identity” per ACL entry at runtime.The UI description calls this out, but there’s no guard in run(). Add a small validation to fail fast with actionable errors instead of a 400 from the API.
Apply this diff:
async run({ $ }) { - const response = await this.databricks.setSQLWarehousePermissions({ - warehouseId: this.warehouseId, - data: { - access_control_list: this.accessControlList, - }, - $, - }); + // Validate ACL entries: exactly one identity field + permission_level + const entries = this.accessControlList ?? []; + const identityKeys = ["user_name", "group_name", "service_principal_name"]; + const errors = []; + entries.forEach((e, idx) => { + const present = identityKeys.filter((k) => e?.[k]); + if (present.length !== 1) { + errors.push(`Entry ${idx + 1}: expected exactly one of ${identityKeys.join(" | ")}, got ${present.length ? present.join(", ") : "none"}`); + } + if (!e?.permission_level) { + errors.push(`Entry ${idx + 1}: missing permission_level`); + } + }); + if (errors.length) { + throw new Error(`Invalid accessControlList:\n- ${errors.join("\n- ")}`); + } + + const response = await this.databricks.setSQLWarehousePermissions({ + warehouseId: this.warehouseId, + data: { + access_control_list: entries, + }, + $, + });
24-27: Verify permission level enums; consider avoiding over-restriction in UI.Permission levels for SQL Warehouses vary across editions/regions and over time. If “CAN_MONITOR” isn’t valid for Warehouses in some workspaces, hardcoding it may block users.
Options:
- Option A (safest): don’t constrain with options.
- permission_level: { - type: "string", - options: ["CAN_VIEW", "CAN_MONITOR", "CAN_USE", "CAN_MANAGE"] - }, + permission_level: { + type: "string", + description: "Permission level for this principal (e.g. CAN_VIEW, CAN_USE, CAN_MANAGE).", + },
- Option B (keep enums minimal until confirmed): drop CAN_MONITOR pending docs confirmation.
- options: ["CAN_VIEW", "CAN_MONITOR", "CAN_USE", "CAN_MANAGE"] + options: ["CAN_VIEW", "CAN_USE", "CAN_MANAGE"]If the app layer exposes a helper to fetch permission levels for a given resource type, consider dynamic options instead.
31-41: Improve the summary to reflect “set” semantics and entry count.Useful feedback to users, especially given set = replace behavior.
Apply this diff:
- $.export("$summary", `Successfully updated permissions for SQL Warehouse ID ${this.warehouseId}`); + $.export("$summary", `Set ${this.accessControlList?.length ?? 0} ACL entr${(this.accessControlList?.length ?? 0) === 1 ? "y" : "ies"} for SQL Warehouse ${this.warehouseId} (replaced existing direct ACL).`);
1-43: Overall: solid action; minor polish + safeguards recommended.Functionally complete and consistent with the other actions in this PR. The main tweaks are docs link, a runtime ACL validator, and a clearer summary. The rest are optional UX refinements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
components/databricks/actions/create-sql-warehouse/create-sql-warehouse.mjs(1 hunks)components/databricks/actions/delete-sql-warehouse/delete-sql-warehouse.mjs(1 hunks)components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs(1 hunks)components/databricks/actions/get-sql-warehouse-config/get-sql-warehouse-config.mjs(1 hunks)components/databricks/actions/get-sql-warehouse-permissions/get-sql-warehouse-permissions.mjs(1 hunks)components/databricks/actions/get-sql-warehouse/get-sql-warehouse.mjs(1 hunks)components/databricks/actions/list-sql-warehouses/list-sql-warehouses.mjs(1 hunks)components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs(1 hunks)components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs(1 hunks)components/databricks/actions/start-sql-warehouse/start-sql-warehouse.mjs(1 hunks)components/databricks/actions/stop-sql-warehouse/stop-sql-warehouse.mjs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- components/databricks/actions/get-sql-warehouse/get-sql-warehouse.mjs
🚧 Files skipped from review as they are similar to previous changes (9)
- components/databricks/actions/start-sql-warehouse/start-sql-warehouse.mjs
- components/databricks/actions/list-sql-warehouses/list-sql-warehouses.mjs
- components/databricks/actions/get-sql-warehouse-config/get-sql-warehouse-config.mjs
- components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs
- components/databricks/actions/create-sql-warehouse/create-sql-warehouse.mjs
- components/databricks/actions/delete-sql-warehouse/delete-sql-warehouse.mjs
- components/databricks/actions/get-sql-warehouse-permissions/get-sql-warehouse-permissions.mjs
- components/databricks/actions/stop-sql-warehouse/stop-sql-warehouse.mjs
- components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs
🧰 Additional context used
🧬 Code graph analysis (1)
components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs (2)
components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs (1)
response(23-27)components/databricks/actions/get-sql-warehouse-permissions/get-sql-warehouse-permissions.mjs (1)
response(18-21)
🔇 Additional comments (2)
components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs (2)
4-8: Metadata looks good (key/name/type/version).Naming follows the Pipedream convention, action type is correct, and semantic version starts at 0.0.1. No blockers here.
31-38: Verified Permissions API configurationI’ve confirmed that:
- The
setSQLWarehousePermissionsmethod incomponents/databricks/databricks.app.mjsinvokes HTTPPUTon the permissions endpoint:
- Path:
/permissions/warehouses/${warehouseId}- Method:
PUT- This aligns with the underlying Databricks Permissions API “set” operation for SQL Warehouses (which is a
PUTat/api/2.0/permissions/warehouses/{warehouseId}).No changes needed—everything matches the expected HTTP verb and resource path.
|
Branch updated with latest master. Addressed CodeRabbit’s feedback. Ready for maintainer review. |
|
Hi @dylburger , I’ve updated the branch with the latest master and addressed CodeRabbit’s feedback. The PR is ready for review whenever you get a chance. Thanks! |
vunguyenhung
moved this to Ready for PR Review
in Component (Source and Action) Backlog
|
Hi @jcortes |
There was a problem hiding this comment.
Hi @Lokeshchand33 thanks for this great contribution, I've just left a few suggestions and in addition to that please make sure you increase the version in package.json to "version": "0.2.0",
Thanks again and let me know if you need any help to move forward!
components/databricks/actions/create-sql-warehouse/create-sql-warehouse.mjs
Outdated
Show resolved
Hide resolved
components/databricks/actions/delete-sql-warehouse/delete-sql-warehouse.mjs
Outdated
Show resolved
Hide resolved
components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs
Outdated
Show resolved
Hide resolved
components/databricks/actions/get-sql-warehouse/get-sql-warehouse.mjs
Outdated
Show resolved
Hide resolved
components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs
Outdated
Show resolved
Hide resolved
components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs
Outdated
Show resolved
Hide resolved
components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs
Outdated
Show resolved
Hide resolved
components/databricks/actions/start-sql-warehouse/start-sql-warehouse.mjs
Outdated
Show resolved
Hide resolved
components/databricks/actions/stop-sql-warehouse/stop-sql-warehouse.mjs
Outdated
Show resolved
Hide resolved
jcortes
moved this from Ready for PR Review
to Changes Required
in Component (Source and Action) Backlog
|
Hi @Lokeshchand33 I've just seen you've made this change however the version should be |
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (3)
components/databricks/actions/list-sql-warehouses/list-sql-warehouses.mjs (1)
6-6: Docs URL fix looks good (plural “warehouses”).The description now points to the correct docs path. Thanks for addressing the earlier note.
components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs (1)
6-6: Fix docs URL to official SQL Warehouses “update” endpointLink points to the wrong path. Use the pluralized sql-warehouses/update URL.
- description: "Edits the configuration of an existing SQL Warehouse. [See docs](https://docs.databricks.com/api/workspace/warehouses/edit)", + description: "Edits the configuration of an existing SQL Warehouse. [See docs](https://docs.databricks.com/api/workspace/sql-warehouses/update)",components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs (1)
6-6: Clarify full-replace semantics and point to canonical docs.This endpoint is a PUT that replaces the entire workspace SQL Warehouse config. The description should warn users to GET → modify → PUT to avoid resets, and link to stable docs.
Apply:
- description: "Updates the global configuration for SQL Warehouses. [See docs](https://docs.databricks.com/api/workspace/warehouses/setworkspacewarehouseconfig)", + description: "Updates the workspace-level SQL Warehouse configuration. This is a full-replace operation (PUT /api/2.0/sql/config/warehouses): fetch the current config first, modify desired fields, then submit the complete payload to avoid accidental resets. See: [CLI docs](https://docs.databricks.com/gcp/en/dev-tools/cli/reference/warehouses-commands) and [Python SDK](https://databricks-sdk-py.readthedocs.io/en/stable/workspace/sql/warehouses.html).",Rationale: CLI and SDK reference this operation and its behavior; PUT requires the full payload. (docs.databricks.com, databricks-sdk-py.readthedocs.io, github.com)
🧹 Nitpick comments (9)
components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs (1)
6-6: Fix docs URL and reference the correct endpointUse the Workspace Permissions API page and mention the warehouses path; the current link path doesn’t exist.
- description: "Updates the permissions for a specific SQL Warehouse. [See docs](https://docs.databricks.com/api/workspace/warehouses/setpermissions)", + description: "Updates the permissions for a specific SQL Warehouse. [See docs](https://docs.databricks.com/api/workspace/permissions) — PUT /api/2.0/permissions/warehouses/{warehouse_id}.",This matches the documented pattern for Set workspace object permissions (PUT …/permissions/{object_type}/{object_id}). (docs.databricks.com)
components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs (3)
41-46: Align prop name with other actions (“autoStopMinutes”)Create action uses autoStopMinutes; this action exposes autoStopMins. Standardize for UX consistency.
- autoStopMins: + autoStopMinutes: { type: "integer", label: "Auto Stop (minutes)", description: "Minutes of inactivity before auto-stop. 0 disables autostop. Must be 0 or ≥ 10.", optional: true, },If you adopt this, update the run mapping accordingly:
- if (this.autoStopMins !== undefined) payload.auto_stop_mins = this.autoStopMins; + if (this.autoStopMinutes !== undefined) payload.auto_stop_mins = this.autoStopMinutes;To preserve backward-compat if needed, you can also support both:
+ const _autoStop = this.autoStopMinutes ?? this.autoStopMins; + if (_autoStop !== undefined) payload.auto_stop_mins = _autoStop;
93-98: Confirm “tags” schema matches API expectationsDatabricks often nests tags under custom_tags; some endpoints accept a flat map. Verify expected shape and normalize if needed.
I can add a small helper to transform a flat object into the required schema if the API expects nesting. Do you want me to draft that?
21-21: Nit: tighten description wordingMinor copy edit.
- description: "Logical name for the warehouse. Must be unique within an org and under 100 characters.", + description: "Logical name for the warehouse. Must be unique within an org and less than 100 characters.",components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs (5)
34-39: Definechannelschema (name/version) to guide users and prevent malformed objects.Apply:
- channel: { - type: "object", - label: "Channel", - description: "Channel details. Example: `{ \"name\": \"CHANNEL_NAME_PREVIEW\", \"dbsql_version\": \"2023.35\" }`", - optional: true, - }, + channel: { + type: "object", + label: "Channel", + description: "Channel selection details for all warehouses (optional).", + properties: { + name: { + type: "string", + label: "Channel Name", + options: [ + "CHANNEL_NAME_CURRENT", + "CHANNEL_NAME_PREVIEW", + ], + }, + dbsql_version: { + type: "string", + label: "DBSQL Version", + optional: true, + }, + }, + optional: true, + },Reference: SDK parameter “Channel” for workspace config. (databricks-sdk-py.readthedocs.io)
40-60: Warn about disabling types and favor GET→modify→PUT.Consider amending the description to avoid accidental disabling of a type, which may convert existing warehouses. Also recommend retrieving current config first.
- description: "Specify which warehouse types are enabled. Example item: `{ \"warehouse_type\": \"PRO\", \"enabled\": true }`", + description: "Specify which warehouse types are enabled. Disabling a type may convert existing warehouses to another type. GET the current config first, then modify. Example item: `{ \"warehouse_type\": \"PRO\", \"enabled\": true }`",SDK notes similar behavior for
enabled_warehouse_types. (databricks-sdk-py.readthedocs.io)
61-92: MarkconfigParamandglobalParamas deprecated; steer users tosqlConfigurationParameters.Apply:
- description: "General config key/value pairs. Example item: `{ \"key\": \"some.config\", \"value\": \"true\" }`", + description: "[Deprecated by Databricks] Prefer using “SQL Configuration Parameters”. Example item: `{ \"key\": \"some.config\", \"value\": \"true\" }`", ... - description: "Global config key/value pairs applied to all warehouses.", + description: "[Deprecated by Databricks] Prefer using “SQL Configuration Parameters”.",Per SDK,
config_paramandglobal_paramare deprecated;sql_configuration_parametersshould be used. (databricks-sdk-py.readthedocs.io)
109-124: Clarify secret reference support in data access config.Add hint so users don’t leak secrets:
- description: "Key/value pairs for data access configuration (e.g., credentials passthrough, external storage access).", + description: "Key/value pairs for data access config (e.g., external metastore properties). Supports secret references like `{{secrets/<scope>/<name>}}`.",Reference: Databricks guidance on secret reference syntax. (docs.databricks.com)
158-165: LGTM on API call/summary. Consider including which fields changed.Optional: compute and include a list of keys overridden in
$summaryto aid auditability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
components/databricks/actions/create-sql-warehouse/create-sql-warehouse.mjs(1 hunks)components/databricks/actions/delete-sql-warehouse/delete-sql-warehouse.mjs(1 hunks)components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs(1 hunks)components/databricks/actions/get-sql-warehouse-permissions/get-sql-warehouse-permissions.mjs(1 hunks)components/databricks/actions/get-sql-warehouse/get-sql-warehouse.mjs(1 hunks)components/databricks/actions/list-sql-warehouses/list-sql-warehouses.mjs(1 hunks)components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs(1 hunks)components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs(1 hunks)components/databricks/actions/start-sql-warehouse/start-sql-warehouse.mjs(1 hunks)components/databricks/actions/stop-sql-warehouse/stop-sql-warehouse.mjs(1 hunks)components/databricks/databricks.app.mjs(2 hunks)components/databricks/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- components/databricks/actions/stop-sql-warehouse/stop-sql-warehouse.mjs
- components/databricks/actions/create-sql-warehouse/create-sql-warehouse.mjs
- components/databricks/actions/get-sql-warehouse-permissions/get-sql-warehouse-permissions.mjs
- components/databricks/actions/get-sql-warehouse/get-sql-warehouse.mjs
- components/databricks/package.json
- components/databricks/actions/delete-sql-warehouse/delete-sql-warehouse.mjs
- components/databricks/databricks.app.mjs
- components/databricks/actions/start-sql-warehouse/start-sql-warehouse.mjs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-15T15:07:38.361Z
Learnt from: GTFalcao
PR: PipedreamHQ/pipedream#13961
File: components/fakturoid/actions/create-invoice/create-invoice.mjs:95-99
Timestamp: 2024-10-15T15:07:38.361Z
Learning: An "object array" type is not yet supported for Pipedream props.
Applied to files:
components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs
🧬 Code graph analysis (3)
components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs (3)
components/databricks/actions/create-sql-warehouse/create-sql-warehouse.mjs (1)
response(41-44)components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs (1)
response(135-139)components/databricks/actions/get-sql-warehouse-permissions/get-sql-warehouse-permissions.mjs (1)
response(20-23)
components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs (1)
components/databricks/actions/get-sql-warehouse-config/get-sql-warehouse-config.mjs (1)
response(13-15)
components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs (5)
components/databricks/actions/create-sql-warehouse/create-sql-warehouse.mjs (2)
payload(35-39)response(41-44)components/databricks/actions/get-sql-warehouse/get-sql-warehouse.mjs (1)
response(20-23)components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs (1)
response(35-41)components/databricks/actions/start-sql-warehouse/start-sql-warehouse.mjs (1)
response(20-23)components/databricks/actions/stop-sql-warehouse/stop-sql-warehouse.mjs (1)
response(20-23)
🔇 Additional comments (9)
components/databricks/actions/list-sql-warehouses/list-sql-warehouses.mjs (2)
12-27: Pagination: endpoint returns full list; no pagination parameters.The Databricks CLI’s “warehouses list” exposes no page/limit flags and states “List all SQL warehouses that a user has manage permissions on,” which implies the underlying REST list is not paginated. Your current implementation that returns the full array is appropriate. (docs.databricks.com)
If you want to be extra-safe, please confirm in your environment that the REST response lacks page tokens.
9-16: Optional: support “run as user” to match CLI capability
In components/databricks/actions/list-sql-warehouses/list-sql-warehouses.mjs, add an optionalrunAsUserIdprop and pass it through:props: { databricks, + runAsUserId: { + type: 'integer', + label: 'Run As User ID', + description: 'Service principal user ID to fetch the list as (maps to run_as_user_id)', + optional: true, + }, }, async run({ $ }) { - const { warehouses } = await this.databricks.listSQLWarehouses({ $ }); + const { warehouses } = await this.databricks.listSQLWarehouses({ + $, + runAsUserId: this.runAsUserId, + });Then update
listSQLWarehouses(args = {})in components/databricks/databricks.app.mjs to forwardrun_as_user_id(or transform camelCase keys) via_makeRequestparams—confirm the mapping supports this key.components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs (3)
18-19: Good choice using string[] for JSON rowsUsing string[] here aligns with Pipedream’s limitation around “object array” props.
3-17: Overall action scaffold looks solidKey, name, propDefinition for warehouseId, parsing JSON rows, summary export, and returning the API response are consistent with other Databricks actions.
35-41: Verified correct HTTP verb and endpoint
ThesetSQLWarehousePermissionsmethod incomponents/databricks/databricks.app.mjsusesPUTon/permissions/warehouses/${warehouseId}, matching the documented API.components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs (1)
24-40: Good enum UX for cluster sizesPredefined options match common Databricks sizes and improve input quality.
components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs (3)
7-7: Confirm action versioning policy (0.0.1 vs. 0.2.0).Maintainer requested version bumps for new components. Verify whether action files must align to 0.2.0 as well or keep 0.0.1 per repo conventions.
If alignment is required, update:
- version: "0.0.1", + version: "0.2.0",
93-108: LGTM onsqlConfigurationParametersshape (RepeatedEndpointConfPairs).This matches the SDK’s RepeatedEndpointConfPairs contract for workspace config.
(databricks-sdk-py.readthedocs.io)
1-166: Verification complete: Databricks methods and payload schema are correct
getSQLWarehouseConfig issues GET to/api/2.0/sql/config/warehouses, setSQLWarehouseConfig issues PUT to the same endpoint, and theconfiguration_pairsschema aligns with the SDK.
components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs
Outdated
Show resolved
Hide resolved
components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs
Outdated
Show resolved
Hide resolved
components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs
Outdated
Show resolved
Hide resolved
components/databricks/actions/list-sql-warehouses/list-sql-warehouses.mjs
Show resolved
Hide resolved
components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs
Show resolved
Hide resolved
components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs
Outdated
Show resolved
Hide resolved
components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs
Show resolved
Hide resolved
components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs
Show resolved
Hide resolved
components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs
Outdated
Show resolved
Hide resolved
|
Hi @jcortes , I’ve addressed all the requested changes. Please let me know if anything else is needed. Thanks. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs (2)
6-6: Fix docs URL to the correct Databricks endpointUse the pluralized path and “update” action. This was flagged earlier and still needs correction.
- description: "Edits the configuration of an existing SQL Warehouse. [See docs](https://docs.databricks.com/api/workspace/warehouses/edit)", + description: "Edits the configuration of an existing SQL Warehouse. [See docs](https://docs.databricks.com/api/workspace/sql-warehouses/update)",
105-110: Remove read-only creatorName from props and payload
creator_nameis read-only; keeping it confuses users and can cause 400s.- creatorName: { - type: "string", - label: "Creator Name", - description: "Warehouse creator name.", - optional: true, - }, @@ - if (this.creatorName) payload.creator_name = this.creatorName;Also applies to: 138-138
🧹 Nitpick comments (2)
components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs (2)
93-98: Validate tag count (< 45) as describedEnforce the documented limit to fail fast client-side.
if (this.tags) payload.tags = this.tags; @@ if (!Object.keys(payload).length) { throw new Error("No fields to update. Provide at least one property."); } + if (this.tags && typeof this.tags === "object" && Object.keys(this.tags).length >= 45) { + throw new Error("Tags must be fewer than 45."); + }Also applies to: 141-157
168-169: Enhance summary with optional warehouse nameSmall UX win in the run output.
- $.export("$summary", `Successfully edited SQL Warehouse ID ${this.warehouseId}`); + $.export("$summary", `Successfully edited SQL Warehouse${this.name ? ` "${this.name}"` : ""} (ID ${this.warehouseId})`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs (8)
components/databricks/actions/create-sql-warehouse/create-sql-warehouse.mjs (2)
payload(35-39)response(41-44)components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs (2)
payload(127-127)response(158-161)components/databricks/actions/get-sql-warehouse-permissions/get-sql-warehouse-permissions.mjs (1)
response(20-23)components/databricks/actions/get-sql-warehouse/get-sql-warehouse.mjs (1)
response(20-23)components/databricks/actions/stop-sql-warehouse/stop-sql-warehouse.mjs (1)
response(20-23)components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs (1)
response(35-41)components/databricks/actions/start-sql-warehouse/start-sql-warehouse.mjs (1)
response(20-23)components/databricks/actions/get-sql-warehouse-config/get-sql-warehouse-config.mjs (1)
response(13-15)
🔇 Additional comments (2)
components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs (2)
7-7: Confirm correct versioning per maintainer guidanceMaintainer requested bump to 0.2.0 for new components. Verify if action versions should align or remain at 0.0.1 for new actions.
41-46: Standardize autoStop property naming
RenameautoStopMinstoautoStopMinutesinedit-sql-warehouse.mjsto match the create action; verify consistency across all Databricks SQL warehouse actions.
components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs
Outdated
Show resolved
Hide resolved
|
Hi @Lokeshchand33 can you plese fix these eslint errors |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs (2)
119-121: Missing prop for enableServerlessCompute referenced in run()run() overlays
this.enableServerlessCompute, but no prop exists, so users cannot set it.Add the prop (place near other top-level props):
props: { databricks, + enableServerlessCompute: { + type: "boolean", + label: "Enable Serverless Compute", + description: "Maps to `enable_serverless_compute`. Required by the API when setting workspace config; if omitted we preserve the current value via GET→PUT.", + optional: true, + },
6-6: Clarify full-replace semantics and update docs URL--- a/components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs +++ b/components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs @@ -6,1 +6,1 @@ - description: "Updates the global configuration for SQL Warehouses. [See docs](https://docs.databricks.com/api/workspace/warehouses/setworkspacewarehouseconfig)", + description: "Updates the workspace-wide SQL Warehouse configuration. This is a full-replace (PUT) operation — retrieve the current config first, modify desired fields, then submit the complete config to avoid resets. [See docs](https://learn.microsoft.com/sql/endpoints/warehouses/set-workspace-warehouse-config)",Confirmed: this endpoint is PUT /api/2.0/sql/config/warehouses with all fields required.
🧹 Nitpick comments (3)
components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs (3)
34-39: Type the channel object to avoid arbitrary shapesDefine explicit fields to reduce invalid payloads.
channel: { type: "object", label: "Channel", - description: "Channel details. Example: `{ \"name\": \"CHANNEL_NAME_PREVIEW\", \"dbsql_version\": \"2023.35\" }`", + description: "Channel details.", + properties: { + name: { type: "string", label: "Channel Name" }, + dbsql_version: { type: "string", label: "DBSQL Version" }, + }, optional: true, },
137-154: Harden pair arrays: trim, drop empty keys, and dedupe last-write-winsPrevents malformed pairs and duplicate keys.
- if (Array.isArray(this.configParam) && this.configParam.length) { - payload.config_param = { - configuration_pairs: this.configParam, - }; - } + if (Array.isArray(this.configParam) && this.configParam.length) { + const norm = this.configParam + .map(({ key, value }) => ({ key: String(key ?? "").trim(), value: String(value ?? "") })) + .filter(({ key }) => key.length); + const map = new Map(norm.map(({ key, value }) => [key, value])); + payload.config_param = { configuration_pairs: Array.from(map, ([key, value]) => ({ key, value })) }; + }Apply similarly to
globalParamandsqlConfigurationParameters. I can push a full patch if you want.
160-161: Optional: include changed fields in $summaryImproves UX when running the action.
Example:
- $.export("$summary", "Successfully updated SQL Warehouse configuration"); + const touched = Object.keys(payload).sort().join(", "); + $.export("$summary", `Updated SQL Warehouse configuration (${touched})`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs(1 hunks)components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-01T10:09:43.747Z
Learnt from: Lokeshchand33
PR: PipedreamHQ/pipedream#18148
File: components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs:40-45
Timestamp: 2025-09-01T10:09:43.747Z
Learning: Pipedream schema doesn't support nested properties in array elements, causing "unsupported array element type" errors when trying to use object[] with properties defined for array items.
Applied to files:
components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs
📚 Learning: 2024-10-15T15:07:38.361Z
Learnt from: GTFalcao
PR: PipedreamHQ/pipedream#13961
File: components/fakturoid/actions/create-invoice/create-invoice.mjs:95-99
Timestamp: 2024-10-15T15:07:38.361Z
Learning: An "object array" type is not yet supported for Pipedream props.
Applied to files:
components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs
🧬 Code graph analysis (1)
components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs (1)
components/databricks/actions/get-sql-warehouse-config/get-sql-warehouse-config.mjs (1)
response(13-15)
🔇 Additional comments (1)
components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs (1)
112-118: Good: GET→merge→PUT preserves required fieldsSeeding payload from the current config mitigates full-replace pitfalls.
components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs (1)
6-6: Fix docs URL and warn about full-replace (PUT) semantics.Update the link slug and explicitly state this operation fully replaces the workspace config to prevent accidental resets.
- description: "Updates the global configuration for SQL Warehouses. [See docs](https://docs.databricks.com/api/workspace/warehouses/setworkspacewarehouseconfig)", + description: "Updates the workspace-wide SQL Warehouse configuration. This is a full-replace (PUT): retrieve the current config first, modify desired fields, then submit the complete configuration. [See docs](https://docs.databricks.com/api/workspace/warehouses/set-workspace-warehouse-config)",
🧹 Nitpick comments (4)
components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs (4)
134-156: Improve UX: accept plain strings for enabledWarehouseTypes.Allow values like PRO/CLASSIC without forcing JSON; keep JSON object support with validation.
- if (Array.isArray(this.enabledWarehouseTypes) && this.enabledWarehouseTypes.length) { - payload.enabled_warehouse_types = this.enabledWarehouseTypes.map((item, idx) => { - let obj = item; - if (typeof item === "string") { - try { obj = JSON.parse(item); } catch (e) { - throw new Error(`enabledWarehouseTypes[${idx}] must be valid JSON: ${e.message}`); - } - } + if (Array.isArray(this.enabledWarehouseTypes)) { + payload.enabled_warehouse_types = this.enabledWarehouseTypes.map((item, idx) => { + let obj = item; + if (typeof item === "string") { + const s = item.trim(); + if (s.startsWith("{")) { + try { obj = JSON.parse(s); } catch (e) { + throw new Error(`enabledWarehouseTypes[${idx}] invalid JSON: ${e.message}`); + } + } else { + obj = { warehouse_type: s, enabled: true }; + } + } if (!obj || typeof obj !== "object") { throw new Error(`enabledWarehouseTypes[${idx}] must be an object with { "warehouse_type": string, "enabled": boolean }`); } const { warehouse_type, enabled, } = obj; if (typeof warehouse_type !== "string" || typeof enabled !== "boolean") { throw new Error(`enabledWarehouseTypes[${idx}] invalid shape; expected { "warehouse_type": string, "enabled": boolean }`); } return { warehouse_type, enabled: Boolean(enabled), }; }); }
157-174: Allow explicit clearing of config arrays.Current checks skip setting empty arrays, making it impossible to clear existing values. Drop the length guard.
- if (Array.isArray(this.configParam) && this.configParam.length) { + if (Array.isArray(this.configParam)) { payload.config_param = { configuration_pairs: this.configParam, }; } - if (Array.isArray(this.globalParam) && this.globalParam.length) { + if (Array.isArray(this.globalParam)) { payload.global_param = { configuration_pairs: this.globalParam, }; } - if (Array.isArray(this.sqlConfigurationParameters) && this.sqlConfigurationParameters.length) { + if (Array.isArray(this.sqlConfigurationParameters)) { payload.sql_configuration_parameters = { configuration_pairs: this.sqlConfigurationParameters, }; } - if (Array.isArray(this.dataAccessConfig) && this.dataAccessConfig.length) { + if (Array.isArray(this.dataAccessConfig)) { payload.data_access_config = this.dataAccessConfig; }
34-39: Constrainchannelshape (optional).If the API expects specific keys (e.g., name, dbsql_version), define them to catch typos early.
- channel: { - type: "object", + channel: { + type: "object", label: "Channel", - description: "Channel details. Example: `{ \"name\": \"CHANNEL_NAME_PREVIEW\", \"dbsql_version\": \"2023.35\" }`", + description: "Channel details. Example: `{ \"name\": \"CHANNEL_NAME_PREVIEW\", \"dbsql_version\": \"2023.35\" }`", + properties: { + name: { type: "string", label: "Name", optional: true }, + dbsql_version: { type: "string", label: "DBSQL Version", optional: true }, + }, optional: true, },
175-181: Surface changed fields in summary (optional).Improves UX to show which keys were updated.
- $.export("$summary", "Successfully updated SQL Warehouse configuration"); + $.export("$summary", `Updated SQL Warehouse configuration: ${Object.keys(payload).sort().join(", ")}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-01T10:09:43.747Z
Learnt from: Lokeshchand33
PR: PipedreamHQ/pipedream#18148
File: components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs:40-45
Timestamp: 2025-09-01T10:09:43.747Z
Learning: Pipedream schema doesn't support nested properties in array elements, causing "unsupported array element type" errors when trying to use object[] with properties defined for array items.
Applied to files:
components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs
📚 Learning: 2024-10-15T15:07:38.361Z
Learnt from: GTFalcao
PR: PipedreamHQ/pipedream#13961
File: components/fakturoid/actions/create-invoice/create-invoice.mjs:95-99
Timestamp: 2024-10-15T15:07:38.361Z
Learning: An "object array" type is not yet supported for Pipedream props.
Applied to files:
components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs
🧬 Code graph analysis (1)
components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs (3)
components/databricks/actions/create-sql-warehouse/create-sql-warehouse.mjs (2)
payload(121-124)response(165-168)components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs (1)
response(37-43)components/databricks/actions/get-sql-warehouse-config/get-sql-warehouse-config.mjs (1)
response(13-15)
components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs
Show resolved
Hide resolved
components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs
Show resolved
Hide resolved
components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs
Outdated
Show resolved
Hide resolved
michelle0927
moved this from Ready for PR Review
to In Review
in Component (Source and Action) Backlog
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs (1)
9-9: Fix docs link to the Update SQL Warehouse endpoint
Previous reviews noted this; updating to the pluralized “sql-warehouses/update”.- description: "Edits the configuration of an existing SQL Warehouse. [See the documentation](https://docs.databricks.com/api/workspace/warehouses/edit)", + description: "Edits the configuration of an existing SQL Warehouse. [See the documentation](https://docs.databricks.com/api/workspace/sql-warehouses/update)",
🧹 Nitpick comments (5)
components/databricks/common/utils.mjs (2)
16-24: Limit recursion to plain objectsAvoid mutating non-plain objects (Date, Map, Error). Safe guard:
- if (typeof obj === "object") { + // Recurse only into plain objects + if (obj && typeof obj === "object" && obj.constructor === Object) {
1-1: Also export named function for ergonomicsKeeps current default export, adds named export.
-const parseObject = (obj) => { +export const parseObject = (obj) => { @@ export default { parseObject, };Also applies to: 29-31
components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs (3)
148-156: Tags: accept array input and allow explicit clearingCurrent logic only sets tags when non-empty and assumes object input. Support arrays and let users clear by sending empty object/array.
- const parsedTags = utils.parseObject(this.tags); - const tags = Object.entries(parsedTags).map(([ - key, - value, - ]) => ({ - key, - value, - })); + const parsedTags = utils.parseObject(this.tags); + let tagsPayload; + if (Array.isArray(parsedTags)) { + tagsPayload = parsedTags; + } else if (parsedTags && typeof parsedTags === "object") { + tagsPayload = Object.entries(parsedTags).map(([key, value]) => ({ key, value })); + } @@ - if (tags.length) payload.tags = tags; + if (this.tags !== undefined) payload.tags = tagsPayload ?? [];Also applies to: 161-161
140-146: Validate serverless-incompatible propsBlock props the API ignores/conflicts with when serverless is enabled.
if (this.enableServerlessCompute !== undefined) { if (this.warehouseType === "CLASSIC" && this.enableServerlessCompute) { throw new ConfigurationError("Serverless compute requires warehouseType = PRO."); } payload.enable_serverless_compute = this.enableServerlessCompute; } + if (this.enableServerlessCompute === true) { + const incompatible = []; + if (this.instanceProfileArn) incompatible.push("instanceProfileArn"); + if (this.spotInstancePolicy) incompatible.push("spotInstancePolicy"); + if (incompatible.length) { + throw new ConfigurationError(`These props are not applicable when enableServerlessCompute is true: ${incompatible.join(", ")}.`); + } + }Also applies to: 157-165
34-39: Unify prop toautoStopMinutes: Replace allautoStopMinsoccurrences in the prop definition (lines 34–39) and in the logic block (lines 116–121) withautoStopMinutes, falling back tothis.autoStopMins(e.g.const autoStopMinutes = this.autoStopMinutes ?? this.autoStopMins).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
components/databricks/actions/create-sql-warehouse/create-sql-warehouse.mjs(1 hunks)components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs(1 hunks)components/databricks/actions/get-sql-warehouse-config/get-sql-warehouse-config.mjs(1 hunks)components/databricks/actions/get-sql-warehouse-permissions/get-sql-warehouse-permissions.mjs(1 hunks)components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs(1 hunks)components/databricks/common/constants.mjs(1 hunks)components/databricks/common/utils.mjs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- components/databricks/common/constants.mjs
🚧 Files skipped from review as they are similar to previous changes (3)
- components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs
- components/databricks/actions/create-sql-warehouse/create-sql-warehouse.mjs
- components/databricks/actions/get-sql-warehouse-config/get-sql-warehouse-config.mjs
🧰 Additional context used
🧬 Code graph analysis (3)
components/databricks/common/utils.mjs (1)
components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs (2)
obj(110-110)obj(119-121)
components/databricks/actions/get-sql-warehouse-permissions/get-sql-warehouse-permissions.mjs (2)
components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs (1)
response(37-43)components/databricks/actions/get-sql-warehouse/get-sql-warehouse.mjs (1)
response(20-23)
components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs (8)
components/databricks/actions/create-sql-warehouse/create-sql-warehouse.mjs (4)
payload(104-107)parsedTags(134-134)tags(135-141)response(155-158)components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs (2)
payload(89-93)response(153-156)components/databricks/actions/get-sql-warehouse-config/get-sql-warehouse-config.mjs (1)
response(13-15)components/databricks/actions/get-sql-warehouse-permissions/get-sql-warehouse-permissions.mjs (1)
response(20-23)components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs (1)
response(37-43)components/databricks/actions/get-sql-warehouse/get-sql-warehouse.mjs (1)
response(20-23)components/databricks/actions/start-sql-warehouse/start-sql-warehouse.mjs (1)
response(20-23)components/databricks/actions/stop-sql-warehouse/stop-sql-warehouse.mjs (1)
response(20-23)
🔇 Additional comments (1)
components/databricks/actions/get-sql-warehouse-permissions/get-sql-warehouse-permissions.mjs (1)
6-6: Docs URL looks correctLink targets the canonical get-permissions reference for warehouses. LGTM.
michelle0927
moved this from In Review
to Ready for QA
in Component (Source and Action) Backlog
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
components/databricks/actions/create-sql-warehouse/create-sql-warehouse.mjs (1)
117-122: Nice fix on minNumClusters validation.Using nullish coalescing and enforcing 1–30 resolves the prior “||” bug and adds the upper bound.
🧹 Nitpick comments (4)
components/databricks/actions/create-sql-warehouse/create-sql-warehouse.mjs (4)
59-70: Align description with options for warehouseType.Description says “PRO or CLASSIC” but options include TYPE_UNSPECIFIED. Either remove it or update the description to reflect all allowed values.
- description: - "Warehouse type: PRO or CLASSIC. Set PRO + enableServerlessCompute = true to use serverless.", + description: + "Warehouse type: CLASSIC, PRO, or TYPE_UNSPECIFIED (API default). For serverless, set warehouseType = PRO and enableServerlessCompute = true.",
144-154: Validate serverless constraints (require PRO; disallow instanceProfileArn).Add guardrails so misconfigurations fail early in the UI.
if (this.enableServerlessCompute !== undefined) payload.enable_serverless_compute = this.enableServerlessCompute; if (this.warehouseType) payload.warehouse_type = this.warehouseType; + // Serverless constraints + if (this.enableServerlessCompute === true && this.warehouseType && this.warehouseType !== "PRO") { + throw new ConfigurationError("Enable Serverless Compute requires warehouseType = PRO."); + } + if (this.enableServerlessCompute === true && this.instanceProfileArn) { + throw new ConfigurationError("instanceProfileArn is not supported for serverless warehouses."); + }Also applies to: 96-101
89-95: Harden tags parsing (guard undefined, trim keys, coerce values).Prevents crashes if parseObject returns null/undefined and avoids empty/whitespace keys.
- const parsedTags = utils.parseObject(this.tags); - const tags = Object.entries(parsedTags).map(([ - key, - value, - ]) => ({ - key, - value, - })); + const parsedTags = utils.parseObject(this.tags) ?? {}; + const tags = Object.entries(parsedTags) + .filter(([key]) => typeof key === "string" && key.trim().length > 0) + .map(([key, value]) => ({ + key: key.trim(), + value: String(value ?? ""), + }));Also applies to: 135-143
161-164: Include warehouse ID in the summary (helps run logs).- $.export( - "$summary", - `Successfully created SQL Warehouse: ${response?.name || this.name}`, - ); + $.export( + "$summary", + `Successfully created SQL Warehouse: ${response?.name || this.name}${response?.id ? ` (ID: ${response.id})` : (response?.warehouse_id ? ` (ID: ${response.warehouse_id})` : "")}`, + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
components/databricks/actions/create-sql-warehouse/create-sql-warehouse.mjs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
components/databricks/actions/create-sql-warehouse/create-sql-warehouse.mjs (1)
components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs (4)
payload(106-106)parsedTags(148-148)tags(149-155)response(171-175)
🔇 Additional comments (2)
components/databricks/actions/create-sql-warehouse/create-sql-warehouse.mjs (2)
104-108: Confirm cluster_size requirement for serverless.Some Databricks serverless configs ignore/forbid cluster_size. Verify that always sending cluster_size won’t cause API rejections when serverless is enabled.
9-9: Verify docs link matches the API used by databricks.app.mjs.Ensure the URL targets the same API version/namespace your app method hits to avoid drift in parameter names.
components/databricks/actions/create-sql-warehouse/create-sql-warehouse.mjs
Show resolved
Hide resolved
vunguyenhung
moved this from Ready for QA
to Changes Required
in Component (Source and Action) Backlog
|
Hello everyone, I have tested this PR and there're some test cases failed or needed improvement. Please check the test report below for more information |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs (2)
9-9: Bump action version.Increment to align with other new Databricks actions in this PR and recent input changes.
- version: "0.0.1", + version: "0.0.2",
8-8: Fix docs URL and call out full-replace (PUT) semantics.The link path looks incorrect and the description should warn users this operation replaces the entire workspace config (GET → modify → PUT). Update the text and link to the canonical Databricks docs.
- description: "Updates the global configuration for SQL Warehouses. [See the documentation](https://docs.databricks.com/api/workspace/warehouses/setworkspacewarehouseconfig)", + description: "Updates the workspace-level SQL Warehouse configuration. This is a full-replace (PUT) operation: first retrieve the current config, modify only desired fields, then submit the complete config to avoid resets. See: Set workspace warehouse config.",References: Databricks CLI “set-workspace-warehouse-config” command and SDK “set_workspace_warehouse_config” confirm full-replace semantics. (docs.databricks.com, databricks-sdk-py.readthedocs.io)
components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs (4)
8-8: Point the docs link to the canonical Permissions API pageReplace the warehouse-specific path with the stable Permissions API docs. The CLI doc confirms the set-permissions semantics (PUT replaces; empty deletes). (docs.databricks.com)
- description: "Updates the permissions for a specific SQL Warehouse. [See docs](https://docs.databricks.com/api/workspace/warehouses/setpermissions)", + description: "Updates the permissions for a specific SQL Warehouse. [See docs](https://docs.databricks.com/api/workspace/permissions)",
20-24: Clarify ACL contract and warn about destructive PUTSpell out “exactly one” identity key, add an example JSON string per item, and warn that PUT replaces all direct permissions (omitted principals are revoked). This aligns with Permissions API usage and CLI semantics. (docs.databricks.com)
accessControlList: { type: "string[]", label: "Access Control List", - description: "List of access control entries. Each entry must include one of `user_name`, `group_name`, or `service_principal_name`, and a `permission_level` (`CAN_VIEW`, `CAN_MONITOR`, `CAN_USE`, `CAN_MANAGE`).", + description: "One JSON object per array item. Each must include exactly one of `user_name`, `group_name`, or `service_principal_name`, plus `permission_level` (`CAN_VIEW`, `CAN_MONITOR`, `CAN_USE`, `CAN_MANAGE`). Uses PUT and REPLACES all direct permissions; omitted principals are revoked. Example: `{ \"user_name\": \"[email protected]\", \"permission_level\": \"CAN_USE\" }`", + optional: false, },
25-25: Add a safety switch to prevent accidental permission wipeBlock empty ACL PUTs unless the user explicitly confirms. CLI docs state an empty set removes all direct permissions. (docs.databricks.com)
}, + confirmReplaceAll: { + type: "boolean", + label: "Confirm replace all existing permissions", + description: "Required to proceed when Access Control List is empty (PUT with an empty ACL deletes all direct permissions).", + optional: true, + },
26-33: Validate ACL entries and guard against empty setsAdd strict validation for identity keys and permission_level; refuse empty ACL unless explicitly confirmed. Allowed levels for warehouses include CAN_VIEW, CAN_MONITOR, CAN_USE, CAN_MANAGE. (docs.databricks.com)
async run({ $ }) { - let acl = utils.parseObject(this.accessControlList); + let acl = utils.parseObject(this.accessControlList); if (!Array.isArray(acl)) { throw new ConfigurationError("Access Control List must be an array"); } - acl = acl.filter((entry) => entry && Object.keys(entry).length > 0); + acl = acl.filter((entry) => entry && Object.keys(entry).length > 0); + const allowed = new Set(["CAN_VIEW", "CAN_MONITOR", "CAN_USE", "CAN_MANAGE"]); + acl.forEach((e, idx) => { + const ids = ["user_name", "group_name", "service_principal_name"].filter((k) => e?.[k]); + if (ids.length !== 1) { + throw new ConfigurationError(`ACL entry ${idx + 1}: include exactly one of user_name, group_name, or service_principal_name`); + } + if (!allowed.has(e?.permission_level)) { + throw new ConfigurationError(`ACL entry ${idx + 1}: invalid permission_level "${e?.permission_level}". Allowed: ${[...allowed].join(", ")}`); + } + }); + if (!acl.length && !this.confirmReplaceAll) { + throw new ConfigurationError("Refusing to send empty ACL. Enable 'Confirm replace all existing permissions' to remove all direct permissions."); + }
🧹 Nitpick comments (1)
components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs (1)
5-10: Future enhancement: offer PATCH “update-permissions” sibling actionConsider adding an “Update SQL Warehouse Permissions” (PATCH) action that merges ACL entries instead of replacing all, mirroring the CLI’s update-permissions. (docs.databricks.com)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
components/databricks/actions/create-sql-warehouse/create-sql-warehouse.mjs(1 hunks)components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs(1 hunks)components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs(1 hunks)components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs
- components/databricks/actions/create-sql-warehouse/create-sql-warehouse.mjs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-10-15T15:07:38.361Z
Learnt from: GTFalcao
PR: PipedreamHQ/pipedream#13961
File: components/fakturoid/actions/create-invoice/create-invoice.mjs:95-99
Timestamp: 2024-10-15T15:07:38.361Z
Learning: An "object array" type is not yet supported for Pipedream props.
Applied to files:
components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjscomponents/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs
📚 Learning: 2025-09-01T10:09:43.747Z
Learnt from: Lokeshchand33
PR: PipedreamHQ/pipedream#18148
File: components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs:40-45
Timestamp: 2025-09-01T10:09:43.747Z
Learning: Pipedream schema doesn't support nested properties in array elements, causing "unsupported array element type" errors when trying to use object[] with properties defined for array items.
Applied to files:
components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs
🧬 Code graph analysis (2)
components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs (3)
components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs (1)
response(174-178)components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs (1)
response(153-156)components/databricks/actions/get-sql-warehouse-permissions/get-sql-warehouse-permissions.mjs (1)
response(20-23)
components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs (2)
components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs (1)
response(33-39)components/databricks/actions/get-sql-warehouse-config/get-sql-warehouse-config.mjs (1)
response(13-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Lint Code Base
- GitHub Check: pnpm publish
- GitHub Check: Verify TypeScript components
- GitHub Check: Publish TypeScript components
🔇 Additional comments (5)
components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs (2)
72-93: Good: GET→whitelist preserves required fields and avoids read-only noise.Seeding from current config and whitelisting allowed keys is the right approach given full-replace semantics.
107-130: Good: Validates enabledWarehouseTypes items and surfaces clear errors.JSON parsing + shape checks prevent API 400s. Nice.
components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs (3)
13-19: LGTM: warehouseId propDefinition usageProp reuses the app’s propDefinition as expected.
41-43: LGTM: summary and response handlingGood use of $summary and returning the API response.
33-39: HTTP method and path are correct –setSQLWarehousePermissionsis implemented withmethod: "PUT"on/permissions/warehouses/${warehouseId}as expected.
components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs
Show resolved
Hide resolved
components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs
Show resolved
Hide resolved
michelle0927
moved this from Changes Required
to Ready for QA
in Component (Source and Action) Backlog
Hi @lokesh154 here I would recommend using an approach that can parse both cases either with string JSON format or object expression. Take a look at this example |
|
Hi @jcortes Yes, we’re already handling both cases (JSON string input and plain object input). The issue only comes up when an invalid JSON string is passed (for example: '{{{ "name": "CHANNEL_NAME_CUSTOM", "dbsql_version": "2023.35" }}}'). In that situation, it falls into the catch block and returns the raw string instead of parsing it , which is why we are seeing the problem |
vunguyenhung
moved this from Ready for QA
to Ready for Release
in Component (Source and Action) Backlog
|
Hi everyone, all test cases are passed! Ready for release! |
github-project-automation
bot
moved this from Ready for Release
to Done
in Component (Source and Action) Backlog
Summary
This PR adds support for Databricks SQL Warehouses APIs by implementing 11 new actions.
Changes
Implements 11 SQL Warehouses endpoints as discussed in the enhancement issue .
WHY
These additions provide full coverage of SQL Warehouse management in Databricks through Pipedream actions, enabling users to automate provisioning, configuration, lifecycle management, and permissions of SQL Warehouses.
Next Steps
Ready to work on additional sections like Vector Search or MLflow APIs once this is reviewed.
Contributes to #18126
Summary by CodeRabbit
New Features
Chores