Skip to content
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

[FEATURE] Detect workspace/account config upon construction of a client #553

Conversation

ShripadMhetre
Copy link

@ShripadMhetre ShripadMhetre commented Jul 17, 2023

Issue fixed - #552

Changes

In workspace_client.go, added the conditional check to catch if account profile is being passed to instantiate WorkspaceClient.

Tests

  • make test passing
  • make fmt applied
  • [] relevant integration tests applied

@ShripadMhetre ShripadMhetre changed the title [FEATURE] Detect workspace/account config upon construction of a client #552 [FEATURE] Detect workspace/account config upon construction of a client Jul 17, 2023
Copy link
Contributor

@nfx nfx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will not work with Terraform, as sometimes we know host only after upstream modules are initialized.

we can't deterministically give this kind of error on the level of NewWorkspaceClient

@@ -791,6 +795,11 @@ func NewWorkspaceClient(c ...*Config) (*WorkspaceClient, error) {
if err != nil {
return nil, err
}

if cfg.AccountID == "" || cfg.IsAccountClient() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will not work with Terraform, as sometimes we know host only after upstream modules are initialized.

Copy link
Author

@ShripadMhetre ShripadMhetre Jul 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar kind of check exist in account_client.go -

if cfg.AccountID == "" || !cfg.IsAccountClient() {
	return nil, ErrNotAccountClient
}

Can you help me understand the optimal solution then? to make it work with Terraform as well.
Also, should I add the call to cfg.EnsureResolved() as well before cfg.IsAccountClient() check?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

terraform resolves config upon the first request

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please review the latest changes and help with proper feedback

@@ -30,6 +32,10 @@ func NewWorkspaceClient(c ...*Config) (*WorkspaceClient, error) {
// default config
cfg = &config.Config{}
}
if cfg.AccountID == "" || cfg.IsAccountClient() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is not enough, proper implementation should be lazy evaluated - e.g. after the error. otherwise this will break certain terraform workflows, where account id is not yet available and is loaded from another resource

@pietern
Copy link
Contributor

pietern commented Aug 31, 2023

For posterity, what was missing here was a call to EnsureResolved on the config.

The lazy initialization in TF is handled by the TF provider itself: https://github.com/databricks/terraform-provider-databricks/blob/eeb453e5c41a71466a762f9145eca6d90124700b/provider/provider.go#L202-L247

That's where it pulls the current values from the provider configuration block, populates a config.Config object, and constructs a client through client.New. Then later the config field from that client is used to construct a workspace client or an account client (see common/client.go).

This is fixed by #596.

@pietern pietern closed this Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants