search

equinixmetal-archive / packngo

[Deprecated] A Golang client for the Equinix Metal API. (Packet is now Equinix Metal)
https://deploy.equinix.com/labs/equinix-sdk-go/
Other
78 stars 53 forks source link

add WithHeader and convert NewClient to use options #349

Closed jabielecki closed 2 years ago

jabielecki commented 2 years ago

Primary Motivation

Described in internal ticket ENG-20050. This change is related to one of EM projects.

Description

~Add yet another constructor in order to provide means for callers to insert custom headers into every request. For example, if they need a third header in addition to the typical X-Auth-Key and X-Consumer-Token, they can use NewClientWithHeader() to obtain that.~

Add the ClientOpt, just classic functional options: https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis

The immediate need is to provide means for callers to insert custom headers into every request. For example, if callers need a third header in addition to the typical X-Auth-Key and X-Consumer-Token pair, they can now use WithHeader.

The existing possibility of customizing headers per every request with DoRequestWithHeaders has the problem that for an existing code base using packngo.DoRequest heavily, it becomes hard to add a header to every request without a massive code modification.

Compatibility

I modified the constructor NewClient in a backward-compatible way to use the functional options. The small difference in logic is that if the builtin default base API URL turns out to be non-parseable:

Also the order of reading env variables changed, which means that from now on majority of the constructor code can benefit from Client.debug ($PACKNGO_DEBUG). Previously it was known quite late into the construction.

Unit Test Simplification

Remove duplicated code: the function claiming to be a "mockX" is in fact "exactlyDuplicatedX". Do not duplicate code from packngo.go with the sole purpose of passing a test through it. The test's goal is to warn about bad changes to the real code - focus the test on that goal.

jabielecki commented 2 years ago

Hi @displague , in PR top document I've rewritten much of the "Description" section and added a new "Compatibility" section. Also, rewritten most of my code. Could you review, please?

jabielecki commented 2 years ago

Added private bool field Client.apiKeySet.

While it adds a bit of complexity, it's a price to keep backward compatibility for users who emitted an empty X-Auth-Token (verbatim X-Auth-Token: and end of line). Could happen in tests and whatnot.