[Feature] Migrate U2M Code to SDKs #2076
Conversation
mgyucht
requested review from
pietern,
andrewnester,
shreyas-goenka and
denik
as code owners
mgyucht
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
|
If integration tests don't run automatically, an authorized user can run them manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
mgyucht
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
mgyucht
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
mgyucht
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
mgyucht
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
mgyucht
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
mgyucht
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
mgyucht
changed the title
There was a problem hiding this comment.
Renaming the file to cmd/auth/in_memory_test.go seems like a mistake.
There was a problem hiding this comment.
This is actually intentional. It is an in-memory implementation of the token cache used only for tests and is not meant to be part of the normal build. Because it is used in the token_test.go tests which are in package auth, this should also be in package auth. At least, I will unexport this type.
cmd/auth/in_memory_test.go
Outdated
| @@ -23,4 +24,4 @@ func (i *InMemoryTokenCache) Store(key string, t *oauth2.Token) error { | |||
| return nil | |||
| } | |||
|
|
|||
| var _ TokenCache = (*InMemoryTokenCache)(nil) | |||
| var _ cache.TokenCache = (*InMemoryTokenCache)(nil) | |||
There was a problem hiding this comment.
Can we remove this line of code? What's the purpose?
There was a problem hiding this comment.
This line just ensures that the InMemoryTokenCache implements the cache.TokenCache interface, similar to OAuthEndpointSupplier and MockApiClient below.
| } | ||
|
|
||
| func getContextForTest(f fixtures.HTTPFixture) context.Context { | ||
| var _ u2m.OAuthEndpointSupplier = (*MockApiClient)(nil) |
There was a problem hiding this comment.
Why do we need this?
There was a problem hiding this comment.
This line just ensures that the MockApiClient implements the OAuthEndpointSupplier interface. If I had a typo in one of the method names or parameter lists, it wouldn't implement that interface. For tests, this isn't terribly important since this is only used in the context of this test.
Changes
Ports the SDK to this change: databricks/databricks-sdk-go#1108. This eliminates the need for the SDK to invoke itself (!) when loading an OAuth token. It also allows us to respond with more suitable errors when fetching tokens. Functionally, this should behave identically to the existing CLI from an external perspective.
One concrete improvement with this change is that the error message returned when a refresh token is invalid now provides the correct command to run, including the appropriate
--profileflag.Users will see the following message when using the CLI with a profile/host with an expired refresh token:
Tests
Mostly this change involves refactoring existing code/tests to use/mock the Go SDK interfaces, so there aren't too many substantial test changes.
token_test.gois rewritten using the table test approach to reduce boilerplate.