Coverage decreased (-0.005%) to 79.821% when pulling d9ea55c on iurygregory:vendor_passthru into 1e1b375 on gophercloud:master.

Build failed.

• gophercloud-unittest : SUCCESS in 5m 20s
• gophercloud-acceptance-test : RETRY_LIMIT in 1m 54s
• gophercloud-acceptance-test-ironic : RETRY_LIMIT in 1m 54s

openlab-ci seems to be broken =(

Indeed. I've been meaning to open an Issue about this. I'll try to get to that today.

Build failed.

• gophercloud-unittest : SUCCESS in 5m 18s
• gophercloud-acceptance-test : RETRY_LIMIT in 1m 54s
• gophercloud-acceptance-test-ironic : RETRY_LIMIT in 1m 57s

@iurygregory Thanks for submitting this.

At first review, I think the pattern being used here is too "high level" for Gophercloud and I couldn't find an existing similar implementation.

Gophercloud should have relatively simple and low level API calls with little logic used to build the request. In this case, I suggest having each function require a CallVendorPassthruOpts in addition to the per-method (create, delete, etc) opts. Once these are in place, helper functions that simplify the interaction with these low-level functions could be added to gophercloud/utils to help make this interaction easier.

Another possible option would be to combine CallVendorPassthruOpts and the per-function opts like this:

type CreateSubscriptionOpts struct {
        Method      string   `q:"method"`
	Destination string   `json:"Destination"`
	EventTypes  []string `json:"EventTypes,omitempty"`
	Context     string   `json:"Context,omitempty"`
	Protocol    string   `json:"Protocol,omitempty"`
}

You can then call two methods on the struct: one to create the query string and one to create the request body. It's still not a clean, intuitive solution, though.

I understand that this makes calling the functions rather awkward, but the way these routes are designed seem to be a little different than most.

Let me know if you have any questions or would like to discuss further.

Hi @jtopjian , thanks for the feedback!
I will give a try to the approach you mentioned of having a single struct including the method.

Having the Method inside the same struct was causing gophercloud to send as part of the request, I'm trying now to add CallVendorPassthruOpts as parameter for each function.

Build failed.

• gophercloud-unittest : SUCCESS in 5m 52s
• gophercloud-acceptance-test : RETRY_LIMIT in 2m 08s
• gophercloud-acceptance-test-ironic : RETRY_LIMIT in 2m 05s

@iurygregory Thanks for making those changes. This code looks good to me, though I'm not familiar with Ironic. Have you been able to test this on a running Ironic service? If it works the way you're expecting, I can go ahead and merge this.

Hi @jtopjian

I've tested and I've found one small issue in the delete - ironic returns 200, so I've added
OkCodes: []int{200, 202, 204},
to it just to avoid the message

Expected HTTP response code [202 204] when accessing [DELETE http://192.168.122.1:6385/nodes/c5a2bcdb-0ead-4162-a5ce-ea3f23929a87/vendor_passthru?method=delete_subscription], but got 200 instead

All other actions are working as expected, I'm pushing a new commit to address the problem with delete and it should be fine.

Thanks!

Build failed.

• gophercloud-unittest : SUCCESS in 5m 27s
• gophercloud-acceptance-test : RETRY_LIMIT in 1m 59s
• gophercloud-acceptance-test-ironic : RETRY_LIMIT in 2m 02s

@jtopjian let me know if there is something else I should do, thanks! =)

@iurygregory Everything looks good. Sorry for the delay!

@jtopjian Thanks! If possible can you publish a tag with this feature? It would make things easier to do the changes in metal3.

@iurygregory Sure, I don't mind making a release for this. Do you have any additional PRs planned? If so, I can wait. But if not, I will create a release now.

@jtopjian I've checked with some folks today, for now this would be the only PR we need to start working on the feature =)

@iurygregory Sounds good. v0.20.0 has been created.