Add `CurrentWorkspaceID()` method on WorkspaceClient by mgyucht · Pull Request #808 · databricks/databricks-sdk-go

mgyucht · 2024-02-08T15:55:45Z

Changes

There are times when it is especially useful to get the workspace ID for the current workspace client. Currently, the workspace ID for the current workspace is exposed as a header in the SCIM Me API call. We'll expose this through a GetWorkspaceId() method, caching the workspace ID for the lifetime of the client.

In the future, we may add a meta service for exposing information about the current account/workspace. At that point, we can migrate off of this somewhat hacky approach.

Tests

make test passing
make fmt applied
relevant integration tests applied

mgyucht · 2024-02-08T16:03:02Z

internal/workspace_client_test.go

+	"github.com/stretchr/testify/assert"
+)
+
+func TestUcAccWorkspaceClient_CurrentWorkspaceId(t *testing.T) {


We use UC acceptance tests because it's the only environment where the current workspace ID is configured.

pietern · 2024-02-08T16:07:38Z

workspace_functions.go

+// connected to.
+func (w *WorkspaceClient) CurrentWorkspaceId(ctx context.Context) (int64, error) {
+	var workspaceIdStr string
+	err := w.apiClient.Client.Do(ctx, "GET", "/api/2.0/preview/scim/v2/Me", httpclient.WithResponseHeader("X-Databricks-Org-Id", &workspaceIdStr))


Is this a stable interface?

I'm raising internally, will send you the links where I'm asking offline.

workspace_functions.go

pietern · 2024-02-08T16:09:21Z

client/client.go

@@ -111,7 +111,7 @@ func (c *DatabricksClient) Do(ctx context.Context, method, path string,
 	if strings.HasPrefix(path, "/api/2.0/fs/files//") {
 		path = strings.Replace(path, "/api/2.0/fs/files//", "/api/2.0/fs/files/", 1)
 	}


Unrelated: with the recent updates to the Files API spec, can we remove this?

Yes, we'll do that as a fast follow.

pietern · 2024-02-08T16:10:38Z

client/client.go

 type DatabricksClient struct {
 	Config *config.Config
-	client *httpclient.ApiClient
+	Client *httpclient.ApiClient


Does exporting this come with unforeseen risks? We may want to include the rationale for exporting/unexporting this in a field comment (there must have been a reason to unexport it to begin with).

I was trying to figure out a convenient way to handle this without exporting the client. Let me take another stab. The thing is that there are some complex dependencies, as the API client doesn't depend on the config package, but these convenience constructors do. config doesn't depend on client, which is why these convenience constructors can live here.

g

codecov-commenter · 2024-02-09T13:36:31Z

Codecov Report

Attention: 51 lines in your changes are missing coverage. Please review.

Comparison is base (ea52c05) 18.53% compared to head (e56387b) 18.43%.
Report is 2 commits behind head on main.

Files	Patch %	Lines
config/api_client.go	31.25%	41 Missing and 3 partials ⚠️
client/client.go	46.15%	5 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #808      +/-   ##
==========================================
- Coverage   18.53%   18.43%   -0.10%     
==========================================
  Files         116      117       +1     
  Lines       21474    21514      +40     
==========================================
- Hits         3980     3966      -14     
- Misses      17250    17302      +52     
- Partials      244      246       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mgyucht · 2024-02-09T14:03:00Z

client/client.go

 )

 func New(cfg *config.Config) (*DatabricksClient, error) {
-	if skippable, ok := cfg.HTTPTransport.(interface {


Moved to config/api_client.go.

## Changes There are times when it is especially useful to get the workspace ID for the current workspace client. Currently, the workspace ID for the current workspace is exposed as a header in the SCIM Me API call. We'll expose this through a get_workspace_id() method, caching the workspace ID for the lifetime of the client. In the future, we may add a meta service for exposing information about the current account/workspace. At that point, we can migrate off of this somewhat hacky approach. Ports databricks/databricks-sdk-go#808 to the Python SDK. ## Tests  - [ ] `make test` run locally - [ ] `make fmt` applied - [ ] relevant integration tests applied

mgyucht added 5 commits February 8, 2024 16:27

cache workspace ID by default

e02c0b5

correctly autogenerate

73d220d

work

1d74995

Merge branch 'main' into add-current-workspace-id

1897fdf

no need to cache

152a785

mgyucht commented Feb 8, 2024

View reviewed changes

mgyucht added 2 commits February 8, 2024 17:03

fmt

cfd7d00

remove caching

47c67ec

pietern reviewed Feb 8, 2024

View reviewed changes

mgyucht mentioned this pull request Feb 8, 2024

Add get_workspace_id to WorkspaceClient databricks/databricks-sdk-py#537

Merged

3 tasks

comment

bd39b6f

g

pietern changed the title ~~Add GetWorkspaceId() method on WorkspaceClient~~ Add CurrentWorkspaceID() method on WorkspaceClient Feb 8, 2024

leave client private

e56387b

mgyucht commented Feb 9, 2024

View reviewed changes

mgyucht requested a review from pietern February 9, 2024 14:04

pietern approved these changes Feb 9, 2024

View reviewed changes

mgyucht added this pull request to the merge queue Feb 9, 2024

Merged via the queue into main with commit 97b3ac0 Feb 9, 2024

mgyucht deleted the add-current-workspace-id branch February 9, 2024 16:10

hectorcast-db mentioned this pull request Feb 14, 2024

Release v0.32.0 #817

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `CurrentWorkspaceID()` method on WorkspaceClient#808

Add `CurrentWorkspaceID()` method on WorkspaceClient#808
mgyucht merged 9 commits intomainfrom
add-current-workspace-id

mgyucht commented Feb 8, 2024 •

edited

Loading

Uh oh!

mgyucht Feb 8, 2024

Uh oh!

pietern Feb 8, 2024

Uh oh!

mgyucht Feb 8, 2024

Uh oh!

Uh oh!

pietern Feb 8, 2024

Uh oh!

mgyucht Feb 8, 2024

Uh oh!

pietern Feb 8, 2024

Uh oh!

mgyucht Feb 8, 2024

Uh oh!

codecov-commenter commented Feb 9, 2024

Uh oh!

mgyucht Feb 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

mgyucht commented Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Tests

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Feb 9, 2024

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mgyucht commented Feb 8, 2024 •

edited

Loading