refactor: move equinix-sdk-go Metal client setup into the config package #563
Conversation
ctreatma
force-pushed
the
lazy-client-creation
branch
from
69caa9a to
101f9cc
Compare
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #563 +/- ##
==========================================
- Coverage 63.11% 55.69% -7.43%
==========================================
Files 94 96 +2
Lines 18250 18613 +363
==========================================
- Hits 11519 10367 -1152
- Misses 6299 7919 +1620
+ Partials 432 327 -105 ☔ View full report in Codecov by Sentry. |
| // NewMetalGoClient returns a new metal-go client for accessing Equinix Metal's API. | ||
| func (c *Config) NewMetalGoClient() *metalv1.APIClient { | ||
| func (c *Config) NewMetalClientForSDK(d *schema.ResourceData) *metalv1.APIClient { | ||
| client := c.newMetalClient() |
There was a problem hiding this comment.
Note this means we're setting up a new client every time, which should avoid coordination issues for User-Agent modifications. If we're OK with creating a bunch of separate clients, we could also use this pattern for other clients and avoid the occasional concurrent map writes panics we've seen (#286, #403).
There was a problem hiding this comment.
I don't understand what the downside would be.
There was a problem hiding this comment.
I'm not totally clear on the overhead of creating these separate clients. There's the potential for increased resource utilization, but we're still reusing the http.DefaultTransport across all clients so I think any increase should be negligible.
There was a problem hiding this comment.
Shared http.DefaultTransport looks good for HTTP KeepAlive and HTTP2 Stream, TCP level concerns. The Go/memory level only introduces a few extra bytes of strings. At the application level we may run into rate limiting response handling problems if each client object is queueing up retries independently. I'm not overly concerned about this today because the API does not return 429s with RetryAfter headers.
ctreatma
force-pushed
the
lazy-client-creation
branch
from
101f9cc to
7b6c26d
Compare
There was a problem hiding this comment.
With the exception of my comment above, this all seems reasonable to me. I would still like to see @displague or @ocobles sign off on it too.
ctreatma
force-pushed
the
lazy-client-creation
branch
from
7b6c26d to
6f0e504
Compare
ctreatma
force-pushed
the
lazy-client-creation
branch
from
6f0e504 to
ecad4a2
Compare
ctreatma
force-pushed
the
lazy-client-creation
branch
from
ecad4a2 to
c6efeb5
Compare
| @@ -24,7 +24,7 @@ func dataSourceMetalPlans() *schema.Resource { | |||
| return datalist.NewResource(dataListConfig) | |||
| } | |||
|
|
|||
| func getPlans(meta interface{}, extra map[string]interface{}) ([]interface{}, error) { | |||
| func getPlans(_ *schema.ResourceData, meta interface{}, extra map[string]interface{}) ([]interface{}, error) { | |||
There was a problem hiding this comment.
You showed great restraint in not converting this from Packngo to the new SDK.
When we need to use a Metal API client inside a terraform resource or datasource, we need to ensure that the client is instrumented correctly. Previously this was done by repeating the same function calls in every CRUD function for every resource and datasource; as a result we were often missing instrumentation, and there was a high risk of losing instrumentation when converting from the old packngo SDK to equinix-sdk-go.
This refactors the equinix-sdk-go client setup code to better ensure that we get a correctly-configured client in all situations and help ensure that we maintain visibility into usage after doing SDK conversions.