stmcginnis / gofish

Gofish is a Golang client library for DMTF Redfish and SNIA Swordfish interaction.
BSD 3-Clause "New" or "Revised" License
211 stars 113 forks source link

Capability of adding custom headers in HTTP calls #129

Closed mikeletux closed 3 years ago

mikeletux commented 3 years ago

Hello Sean,

I've been thinking for a while about an issue that we could face regarding including headers using the different methods this library has (Get, Post, PostMultipart, Put, Patch, Delete, runRequest and runRequestWithMultipartPayload).

All of above method eventually use runRawRequest method to perform the call to the server.

It turns out that runRawRequest has an argument in its signature called contentType and that allow to set the header Content Type to whatever. But what if I need to set a different header (custom one)? Current implementation doesn't allow to do this.

Would you be open to modify current implementation to add an argument to all above methods so it allows adding custom headers to the different HTTP requests? I.e:

func (c *APIClient) Post(url string, payload interface{}, customHeaders map[string]string) (*http.Response, error) {
    return c.runRequest(http.MethodPost, url, payload)
}

I need this because, I'm trying to create a Resource in our Terraform project that upgrades firmware. In order to do so, before using PostMultipart to upload the package, we need to get the Etag from the Firmware/Software repository, and then do a PostMultipart with a custom header with "If-Match" as key and the Etag value as value.

What do you think? Thanks! /Miguel

stmcginnis commented 3 years ago

Hey Miguel. That makes sense. I could see this being needed for other things as well.

My only reservation is that Post is a public function, so there's the potential that changing it could break someone calling it directly from their own code for whatever reason.

The only other way around that I see would be to add a new function called something like PostWithHeaders. Then the existing Post call could just end up calling PostWithHeaders and pass in an empty value for customHeader.

That would be the safest way to add this capability without breaking anyone. That said, we are still at a 0.x release, so there's no guarantee that we wont break the interface, so we could just change it. :)

I'm kind of leaning towards adding the second function, but more because that would mean we wouldn't need to update all the other places in the code that are currently calling Post. What do you think?

mikeletux commented 3 years ago

Yeah, I agree, let's keep it for now compatible as I also would break my existing code.

I will create additional function signatures and when v1.X is release you can decide how to move forward!

Thanks! /Miguel

mikeletux commented 3 years ago

@stmcginnis we can close this one 😄

mikeletux commented 3 years ago

Merged. We can close this one.