oxide: refactor exported client API

sudomateo commented 10 months ago

Previously, there were two exported functions to create an Oxide API client.

func NewClient(token, userAgent, host string) (*Client, error)
func NewClientFromEnv(userAgent string) (*Client, error)

These two functions required a user agent to be passed as an argument, which should be an optional argument instead. Additionally, having two separate functions means a user must switch functions when they wish to switch to or from using an environment to configure the client.

The code always created a custom http.RoundTripper to be used as the transport for the underlying HTTP client. The only function of this custom tranport was to add HTTP headers to the request.

This patch updates the SDK to export a single function to create an Oxide API client.

func NewClient(cfg *Config) (*Client, error)

This new function uses a new Config type to accept optional configuration for the client. Now, a user can not only build an API client from their environment, but also override the user agent or HTTP client used.

The custom transport was removed and the logic to set HTTP headers for the request was moved into the buildRequest method where it will have access to the user agent string and the request to set the headers.

Closes: #165, #166

karencfv commented 10 months ago

Heya @sudomateo thanks a bunch for taking this on! Sorry for the late reply, I was out for the holidays and just returned. I'll take a good look this week.

Thanks again! :bowing_woman:

karencfv commented 10 months ago

sudomateo commented 9 months ago

I ran some manual tests with a little client I built which uses the this branch as a dependency, and I get an authorization error using both a nil config with environment variables and setting the host and token directly through the Config.

Sorry about that! It's not the http.RoundTripper, but rather the fact that I forgot to add Bearer in the authorization header.

The header should have been:

Authorization: Bearer TOKEN

But the code had it as:

Authorization: TOKEN

I fixed that in my latest commit.

Perhaps it may be a good idea to test functionality of a code change before setting it as ready to review.

Is there a process I could use to test against an Oxide API, mocked or otherwise? I had access to a demo environment but that was spun down and provisioned anew. I'd love to do more testing before marking the pull request ready.

sudomateo commented 9 months ago

Gargh! sorry about that! At the moment we don't really have any other way of manually testing other than using one of our racks. Don't worry about it, I've tested it myself. All good now .

Thanks a bunch for the PR, and for bearing with me :)

🙇‍♀️

It's not a burden at all! I know there was a lot of back and forth, but I really appreciate the reviews and discussions. It helps make the code better for everyone. Plus you get to know the style of the codebase and meet cool people!

oxidecomputer / oxide.go

oxide: refactor exported client API #180