Databricks CLI config reads #85
Conversation
nfx
changed the title
databricks/provider.go
Outdated
| return fmt.Errorf("Authentication is not configured for provider. Please configure it\n"+ | ||
| "through one of the following options:\n"+ | ||
| "1. DATABRICKS_HOST + DATABRICKS_TOKEN environment variables.\n"+ | ||
| "2. host + token provider argumeents.\n"+ | ||
| "3. Run `databricks configure --token` that will create %s file.\n\n"+ | ||
| "Please check https://bit.ly/2XCtuZU for details", configFile) |
There was a problem hiding this comment.
I'm out of terraform context, but I would not understand that I need to create an ini file with host and token in it by reading this error message. Why it's not just printed to console instead of passing as an error, by the way? In other places we return internal errors, do we print those directly to the client?
Why is it an ini and not json or yaml by the way?
There was a problem hiding this comment.
Terraform prints errors in it's own way, so i just rely on standard mechanism of it's framework.
Regarding INI files - 🤷, https://docs.databricks.com/dev-tools/cli/index.html#set-up-authentication, https://github.com/databricks/databricks-cli/blob/master/databricks_cli/configure/provider.py - most likely because https://docs.python.org/3/library/configparser.html is an INI parser
| "3. Run `databricks configure --token` that will create %s file.\n\n"+ | ||
| "Please check https://bit.ly/2XCtuZU for details", configFile) | ||
| } | ||
| if profile, ok := d.GetOk("profile"); ok { |
| config.Host = host | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
schema.ResourceData does not contain required fields to fill. Is it really a positive outcome for this function?
databricks/provider.go
Outdated
| if config.Host == "" || config.Token == "" { | ||
| if err := tryDatabricksCliConfigFile(d, &config); err != nil { | ||
| return nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
I'd probaly go with something like this for separation of concerns and readability:
if config.Host == "" || config.Token == "" {
pathRaw, ok := d.GetOk("config_file")
if !ok { return err... }
path, ok := pathRaw.(string) // Not sure if ok check is needed here. I see it's never done, so maybe it doesn't panic if d.GetOk returns ok = true
if !ok {
return fmt.errorf(...)
}
profile, ok := d.Get("profile")
... // similar to path
host, token, err := getCredentials(path, profile) // this function would open file and try to read profile settings.
if err {...}
config.Host = host
config.Token = token
}
| raw["config_file"] = "testdata/.databrickscfg" | ||
| raw["profile"] = "invalidhost" | ||
| err := testAccProvider.Configure(terraform.NewResourceConfigRaw(raw)) | ||
| assert.NotNil(t, err) |
There was a problem hiding this comment.
It might also be a good idea to actually check that the error message is the one that you expect and contains all the context required.
There was a problem hiding this comment.
according to certain beliefs, asserting exception message was not considered a good practice - e.g. because it may change the content of the string or be translated to other language, therefore making test "more overfitting" and fragile to insignificant changes.
…kslabs/databricks-terraform into cluster-policies � Conflicts: � databricks/provider.go
…anded the bit.ly links
Codecov Report
@@ Coverage Diff @@
## master #85 +/- ##
==========================================
+ Coverage 46.75% 47.27% +0.51%
==========================================
Files 53 53
Lines 6771 6815 +44
==========================================
+ Hits 3166 3222 +56
+ Misses 3552 3539 -13
- Partials 53 54 +1
|
stikkireddy
changed the title
stikkireddy
changed the title
stikkireddy
changed the title
This PR adds support for reading credentials from
~/.databrickscfgif neither DATABRICKS_HOST/DATABRICKS_TOKEN nor provider host/token parameters are specified.