Use new workspace_url format + fix azure test

[EliiseS]

Implementation of the issue: #34

Co-authored-by: [email protected]
[email protected]

[lawrencegripper]

Looks like we're nearly there! Just running again to check it's consistent.

[stikkireddy]

again thank you @lawrencegripper @EliiseS for picking this up!

[codecov-commenter]

Codecov Report

Merging #114 into master will increase coverage by 0.04%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #114      +/-   ##
==========================================
+ Coverage   47.98%   48.02%   +0.04%     
==========================================
  Files          58       58              
  Lines        7228     7219       -9     
==========================================
- Hits         3468     3467       -1     
+ Misses       3700     3690      -10     
- Partials       60       62       +2     

Flag	Coverage Δ
#client	75.33% <ø> (ø)
#provider	40.46% <0.00%> (+0.04%)	⬆️

Impacted Files	Coverage Δ
databricks/azure_auth.go	0.00% <0.00%> (ø)
databricks/provider.go	74.69% <0.00%> (+1.38%)	⬆️

[EliiseS]

@stikkireddy codecov/patch — 26.31% of diff hit (target 48.22%) is failing, but we're not sure if we need to change anything or what we need to change

[lawrencegripper]

Just investigating some odd behavior with this change. Lets not merge until we've got to the bottom of it.

[EliiseS]

@stikkireddy The odd behaviour we were investigating concluded in #119, which, while not great, isn't blocking this issue. Are you happy to merge this PR?

[stikkireddy]

@EliiseS on the above comment regarding turning the azure_auth into a list/set rather than a map, would you like to make that another PR?

If you would like to move it outside of the scope of this PR we can merge this in please let me know.

[EliiseS]

@stikkireddy I actually looked into doing that and I was unable to get the tests to work with it. Let's move that discussion to another issue. I'm waiting for clarification from terraform in this PR: hashicorp/terraform-plugin-sdk#142

[stikkireddy]

awesome that is fine @EliiseS , I added a comment on that issue by the way.

[stikkireddy]

Hey @EliiseS one last comment I would like to add:

1. When we add workspace_url we break all existing provider configurations.
2. We are just mapping workspace url to host, I am wondering is it really necessary to configure workspace_url?
   Two things that we can do to alleviate Merge down from master #2:
3. Repurpose HOST for the workspace url as introducing it as a new field just for workspace URL then only to assign it back to host.
4. The following functions api call response actually contains the workspace url and the user does not need to provide it explicitly:

func (a *AzureAuth) getWorkspaceID(config *service.DBApiClientConfig) error {
	log.Println("[DEBUG] Getting Workspace ID via management token.")
	// Escape all the ids
	url := fmt.Sprintf("https://management.azure.com/subscriptions/%s/resourceGroups/%s"+
		"/providers/Microsoft.Databricks/workspaces/%s",
		urlParse.PathEscape(a.TokenPayload.SubscriptionID),
		urlParse.PathEscape(a.TokenPayload.ResourceGroup),
		urlParse.PathEscape(a.TokenPayload.WorkspaceName))
	headers := map[string]string{
		"Content-Type":  "application/json",
		"cache-control": "no-cache",
		"Authorization": "Bearer " + a.ManagementToken,
	}
	type apiVersion struct {
		APIVersion string `url:"api-version"`
	}
	uriPayload := apiVersion{
		APIVersion: "2018-04-01",
	}
	var responseMap map[string]interface{}
	resp, err := service.PerformQuery(config, http.MethodGet, url, "2.0", headers, false, true, uriPayload, nil)
	if err != nil {
		return err
	}
	err = json.Unmarshal(resp, &responseMap)
	if err != nil {
		return err
	}
	a.AdbWorkspaceResourceID = responseMap["id"].(string)
	return err
}

[nfx]

this will not be fully idempotent, when there are other db provider files there already, possibly with different names. I'd suggest to clear all files starting with terraform-provider-databricks and then do the move.

i'd suggest to rename target to simply install, following make install mantra :)

[EliiseS]

This pr has been merged. I can make these changes in another PR

[stikkireddy]

removed this command in another pr #129 considering that the version is hardcoded there for local install. If we want to make an install target for the makefile we can work on that in another PR

[EliiseS]

Other providers have a script to create a binary from the local files as well. It's very useful for testing and trying out your changes. Why is the hard-coded version number a problem enough that you'd remove the addition? How would you like to set a version number instead?