-
Notifications
You must be signed in to change notification settings - Fork 384
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add caching of scim/me API to databricks_current_user
data source and databricks_permissions
resource.
#2170
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #2170 +/- ##
==========================================
- Coverage 88.99% 88.79% -0.20%
==========================================
Files 137 137
Lines 11182 11209 +27
==========================================
+ Hits 9951 9953 +2
- Misses 827 849 +22
- Partials 404 407 +3
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you just create func (c *DatabricksClient) CurrentUser(ctx), that'll lock/unlock properly. just because there's only one instance of DatabricksClient per provider ;)
common/client.go
Outdated
if err != nil { | ||
return user, err | ||
} | ||
*a.cachedUser = *user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not a.cachedUser = user
?..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pointer points to the cached user in client. This is why we need to copy into the cachedUser value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just make it cachedUser *scim.User
on the client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would not work I think. If we do this, then a.cachedUser = user
will lead to the cachedUser within CachedUserService
to point to the new user, but the changes won't be propagated to the client.
common/client.go
Outdated
if err != nil { | ||
return user, err | ||
} | ||
*a.cachedUser = *user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just make it cachedUser *scim.User
on the client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
databricks_current_user
data source and databricks_permissions
resource.
databricks_current_user
data source and databricks_permissions
resource.databricks_current_user
data source and databricks_permissions
resource.
…nd `databricks_permissions` resource. (#2170) * Cache scim/me API * add mutext to protect cached user * naming fix * defer mu.unlock and remove unused code * cache ws client * inline method and rename
Changes
Tests
close #2120