Set bundle auth configuration in command context #2195
Conversation
shreyas-goenka
requested review from
pietern,
andrewnester and
denik
as code owners
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
cmd/root/bundle_test.go
Outdated
| err := cmd.Flag("profile").Value.Set("PROFILE-2") | ||
| require.NoError(t, err) | ||
| b := setupWithProfile(t, cmd, "PROFILE-1") | ||
| func TestBundleConfigureProfile(t *testing.T) { |
There was a problem hiding this comment.
While acceptance tests would be better suited for most of these, since this PR changes how auth is loaded I opted to keep the unit tests.
This reverts commit 27f1ae3.
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
cmd/root/bundle.go
Outdated
|
|
||
| // Set the auth configuration in the command context. This can be used | ||
| // downstream to initialize a API client. | ||
| client, err := b.InitializeWorkspaceClient() |
There was a problem hiding this comment.
There is also WorkspaceClient() which creates and caches the client on bundle.
If that's not necessary, should we clean that up?
Related: Bundle.client, Bundke.clientOnce, can we get rid of those?
There was a problem hiding this comment.
If that's not necessary, should we clean that up?
Eyeballing it we could clean that up a bit without issues. It's out of the scope of what this PR is trying to achieve, though.
There was a problem hiding this comment.
I can send a follow-up PR to do so.
There was a problem hiding this comment.
I think it's in scope, we should not have multiple ways to set up client.
Also, please take a look "variables v2" doc. How would this work if we want to interpolate variables first?
There was a problem hiding this comment.
we should not have multiple ways to set up client.
Fair enough. I modified the change to set the client as well. I was hesitant to do this because I don't like how decoupled the client's setting is from its usage, but this change should be safe enough (and covered by tests).
How would this work if we want to interpolate variables first?
We'd need to resolve the variables before we resolve the client.
There was a problem hiding this comment.
Changed the approach to still ensure that the client is only initialized / set once. Did so by renaming InitializeWorkspaceClient to WorkspaceClientE
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Changes
This change is required to enable tracking execution time telemetry for bundle commands. In order to track execution time for the command generally, we need to have the databricks auth configuration available at this section of the code:
cli/cmd/root/root.go
Line 99 in 41bbd89
In order to do this we can rely on the
configUsedcontext key.Most commands rely on the
root.MustWorkspaceClientfunction which automatically sets the client config in theconfigUsedcontext key. Bundle commands, however, do not do so. They instead store their workspace clients in the&bundle.Bundle{}object.With this PR, the
configUsedcontext key will be set for allbundlecommands. Functionally nothing changes.Tests
Existing tests. Also manually verified that either
root.MustConfigureBundleorutils.ConfigureBundleWithVariablesis called for all bundle commands (exceptbundle init) thus ensuring this context key would be set for all bundle commands.refs for the functions:
root.MustConfigureBundle:cli/cmd/root/bundle.go
Line 88 in 41bbd89
utils.ConfigureBundleWithVariables:cli/cmd/bundle/utils/utils.go
Line 19 in 41bbd89