Correcting the responder requests to actually match the API

[michael-bud]

It looks like this never actually worked and it doesn't match the API.

A correct request:

{
  "requester_id": "PQKSYYY",
  "message": "Please help with issue",
  "responder_request_targets": [
    {
        "responder_request_target": {
            "id": "PQKSYYY",
          "type": "user_reference"
        }
    }
  ]
}

What it was generating as a request (which would result [unhelpfully] in just an undescriptive 404 error back from pagerduty):

{
     "requester_id": "PQKSYYY",
    "message": "Please join an existing incident to help",
    "responder_request_targets": [
        {
            "id": "PQKSYYY",
            "type": "user_reference"
        }
    ]
}

Also the response would generate this error as the response has an array of targets:

json: cannot unmarshal array into Go struct field ResponderRequest.responder_request.responder_request_targets of type pagerduty.ResponderRequestTargets

Unfortunately, there didn't seem like a nice way to keep BC for this - that said, this never worked before anyway so I guess nobody is using it?

It appears the docs used to be wrong on this which is probably where it came from:
https://community.pagerduty.com/forum/t/create-responder-api-call-failing/226

It's worth noting the pagerduty API docs are also still wrong. The example is correct, but the schema is wrong: https://developer.pagerduty.com/api-reference/reference/REST/openapiv3.json/paths/~1incidents~1%7Bid%7D~1responder_requests/post

I have tested this fix locally with this code in a mock command and it works. I've tried to maintain BC also:

       if err := c.Meta.Setup(); err != nil {
		fmt.Println(err)
		return -1
	}
	client := c.Meta.Client()

	opts := pagerduty.ResponderRequestOptions{
		From: "XXXXX",
	}

	pid := "PPPSYYY"

	opts.Message = fmt.Sprintf("Please join an incident")
	opts.Targets = []pagerduty.ResponderRequestTarget{{APIObject: pagerduty.APIObject{ID: pid, Type: "user_reference"}}}
	opts.RequesterID = pid

	_, err := client.ResponderRequest("PPP1YYY", opts)
	fmt.Println(err)

I'd appreciate it if a tag could be done after merging this PR so I can pull the change down. Thanks!

[stmcallister]

Thanks for trying to keep this backwards compatible! However, if it's never worked (and that's on me because I reviewed the original PR) would it make sense to simply fix the Targets field to be []ResponderRequestTargets, similar to how you did in ResponderRequests?

[michael-bud]

To be honest, the DX of doing the construction with []ResponderRequestTarget does feel more intuitive and I can easily see people getting confused in the future to be honest if having to build []ResponderRequestTargets themselves. I postulate that's probably why the mistake was made to implement it like that originally.

On the other hand, what you get back is different enough you're not expecting consistency and as the developer isn't the one building it, it doesn't matter so much.

[theckman]

@michael-bud there are a few cases in this library where things were introduced that seem to have never worked, and so I'm proposing a v1.5.0 release where we fix all of these issues in a way that will unfortunately be a breaking change.

My justification for v1.5.0 over v2.0.0 is...

• These Go APIs never worked reliably, so I would not declare them as being stable, and the proper fix requires a breaking change.
• Go Modules adds unnecessary overhead when releasing a new major version, and so I'm trying to hold off on a v2.0.0 as long as possible.

Would you be willing to resolve the merge conflict and update this PR to be a breaking change that we target for v1.5.0? I'm trying to get as many fixes into v1.4.0 as I can so people don't feel as-pressured to upgrade to v1.5.0 and possibly have to fix BC.

[theckman]

Since we've not heard from you we'll close this in favor of #328.