oxide: refactor exported client API

[sudomateo]

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]

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! 🙇‍♀️

[karencfv]

Thanks a bunch for working on this! I've left a few comments below.

Could you also add a small entry for the changelog here https://github.com/oxidecomputer/oxide.go/blob/main/.changelog/v0.1.0-beta3.toml? There are some good examples in the file for the last release https://github.com/oxidecomputer/oxide.go/blob/main/.changelog/v0.1.0-beta2.toml

[karencfv]

Thanks for addressing my comments @sudomateo, this is looking great!

Just a few things here and there, but overall looks almost ready 🎉

[karencfv]

Related: #164

[karencfv]

Just a few more bits.

Thanks for bearing with me! 😅

[karencfv]

Thanks for updating the PR!

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.

$ go run main.go
2024/01/22 16:10:42 GET https://oxide.sys.rack2.eng.oxide.computer/v1/vpc-firewall-rules?vpc=a06093b1-0984-47f9-b895-b7a6304b5d2c
----------- RESPONSE -----------
Status: 401 Unauthorized
Message: credentials missing or invalid
RequestID: 420453ee-242b-4123-b749-f962734c79a8
------- RESPONSE HEADERS -------
Content-Type: [application/json]
X-Request-Id: [420453ee-242b-4123-b749-f962734c79a8]
Date: [Mon, 22 Jan 2024 03:10:41 GMT]
Content-Length: [137]
exit status 1

I confirmed my token and host were valid by running the same code with the current SDK as a dependency.

Additionally, I think I missed this last time around, but I realised you deleted the RoundTrip() method. Could you please revert that change (and any change that is not related to retrieving token and host from the config) please?

http.RoundTripper is an interface, so we need to implement it.

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

[sudomateo]

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.

[karencfv]

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.

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 :)

🙇‍♀️

[sudomateo]

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!