Skip to content

Fixed TestWorkspace_WaitForResolve_Failure by checking the unwrapped *net.DNSError#2957

Closed
nfx wants to merge 2 commits intomainfrom
test-httpclient
Closed

Fixed TestWorkspace_WaitForResolve_Failure by checking the unwrapped *net.DNSError#2957
nfx wants to merge 2 commits intomainfrom
test-httpclient

Conversation

@nfx
Copy link
Copy Markdown
Contributor

@nfx nfx commented Nov 24, 2023

This PR fixes the test failure by properly depending on the error from Go's networking stack, which is now propagated by databricks/databricks-sdk-go#699

…d `*net.DNSError`

This PR fixes the test failure by properly depending on the error from Go's networking stack
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

Merging #2957 (156d80e) into master (52af61d) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2957   +/-   ##
=======================================
  Coverage   84.01%   84.01%           
=======================================
  Files         153      153           
  Lines       13634    13634           
=======================================
  Hits        11454    11454           
  Misses       1533     1533           
  Partials      647      647           
Files Coverage Δ
mws/resource_mws_workspaces.go 87.61% <100.00%> (ø)

github-merge-queue bot pushed a commit to databricks/databricks-sdk-go that referenced this pull request Nov 24, 2023
…ent` into `httpclient.ApiClient` (#699)

## Changes

This PR decouples resiliency of HTTP client (rate limiting, retries,
timeouts, debug logging) from `client.DatabricksClient` and moves it to
a dedicated class, allowing to later use the same robust mechanism to
refresh AAD tokens.

What's changed in `httpclient.ApiClient` compared from
`client.DatabricksClient` (originated in Databricks Terraform Provider
back in the early 2020):
- added native integration with `golang.org/x/oauth2` package for both
using token sources in the request as well as injecting a custom
`*http.Client` the context for token refreshes.
- error mapper is now a pluggable function, default implementation
available
- error retry checking is now pluggable function, default implementation
checks for HTTP 429 and 504
- transient error messages are now configurable per client instance
- HTTP request visitors are now configurable per client instance
- Explicit and coupled `Do(ctx context.Context, method, path string,
headers map[string]string, request, response any, visitors
...func(*http.Request) error) error` became concise and flexible `func
(c *ApiClient) Do(ctx context.Context, method, path string, opts
...DoOption) error`. Headers can now be specified with
`httpclient.WithHeaders(headers)`, JSON deserialisation can now be
specified with `httpclient.WithUnmarshal(&entity)`.
- added `httpclient.WithCaptureHeader(key, &value)` to assign headers to
a string pointer. This is required to get a proper integration with
Azure Container Registry Managed Identity passwordless setup.
- body can be specified as `httpclient.WithBody(any)`, that internally
sets a special data field. Technically, this could have been a request
visitor, but we have special logic for "debug bytes" logging, which is
out of scope of this PR change.
- removed flawed trailing double-forward-slash trimming (`//`), as it
was breaking requests containing full URLs.
- Once we merge the
#697, we can
properly unit-test the implementations of Azure Client Secret and MSI
flows

This is required for downstream tasks like:
- #240
- #566
- #495
- proper solution to
#321 instead of
hack in
d5e57f7
- 
## Tests

- [x] Only one test was failing in TF provider, fixed in
databricks/terraform-provider-databricks#2957
@mgyucht
Copy link
Copy Markdown
Contributor

mgyucht commented Apr 14, 2025

This has already been handled in this resource, so I'll close this stale PR.

@mgyucht mgyucht closed this Apr 14, 2025
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.

4 participants