Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correcting the responder requests to actually match the API #251

Closed
wants to merge 1 commit into from

Conversation

michael-bud
Copy link
Contributor

@michael-bud michael-bud commented Nov 26, 2020

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!

Message string `json:"message"`
RequesterID string `json:"requester_id"`
Targets []ResponderRequestTarget `json:"-"`
NestedTargets []ResponderRequestTargets `json:"responder_request_targets"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@michael-bud michael-bud Feb 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator

theckman commented Feb 20, 2021

@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 theckman added the open question there is an open question on the issue / PR label Feb 20, 2021
@theckman theckman added this to the v1.5.0 milestone Feb 20, 2021
@theckman theckman added the breaking change This PR contains a breaking change label Mar 8, 2021
@theckman
Copy link
Collaborator

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

@theckman theckman closed this May 16, 2021
@theckman theckman removed this from the v1.5.0 milestone May 16, 2021
@theckman theckman removed the open question there is an open question on the issue / PR label May 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change This PR contains a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants