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

Add support for better API debugging; start v1.5.0 development #325

Merged
merged 1 commit into from
Aug 29, 2021

Conversation

theckman
Copy link
Collaborator

@theckman theckman commented Apr 23, 2021

In prepration for #305, this adds a mechanism to inspect all API requests and
responses handled by the client. That way if you want to figure out why
something isn't working as expected, or you'd like to make use of an
undocumented field/feature, you have access to the response and can use it.

This also adds a Do() method, with the same signature as *http.Client.Do(),
that allows consumrs to generate their own request, have the client add
authentication details and other headers, before sending the request to the
PagerDuty API. This allows consumers to further debug and make use of features
not yet supported in the REST client.

Also, updates Version string to v1.5.0 as this is work towards that release.

Lastly, this updates some of the test files to remove some duplication across
them. These changes were needed due to the new fields added to the struct that
had to be initialized to make HTTP requests.

@theckman theckman added this to the v1.5.0 milestone Apr 23, 2021
@theckman theckman force-pushed the better_last_response_handling branch from 2941d9c to e27c9e5 Compare May 8, 2021 22:37
### Client Library

#### NOTICE: Breaking API Changes in master branch
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This isn't technically true yet, but I plan to merge open PRs soon that will make this true... so may as well prematurely optimize. 😄

@theckman theckman force-pushed the better_last_response_handling branch 2 times, most recently from b0805db to 919619a Compare May 29, 2021 00:24
theckman added a commit that referenced this pull request Jul 4, 2021
This is to help with debugging issue #339. This branch is based off of #325, so
that I could make use of the new response debugging functionality I added.

I ran the following commands from within the `command` directory to get the
below result:

```shell
go build -o pd .
./pd schedule override create -authtoken "$(cat ../api.key)" PF4QFIN debug_override.json
```

The `api.key` file is just an API key from the PD console. `PF4QFIN` is a
PagerDuty service in my development account. Here is the output:

```
time="2021-07-04T01:00:24-07:00" level=info msg="service id is:PF4QFIN"
time="2021-07-04T01:00:24-07:00" level=info msg="Input file is:debug_override.json"
req:
req.Method: POST
req.URL: https://api.pagerduty.com/schedules/PF4QFIN/overrides
req.Header: http.Header{"Accept":[]string{"application/vnd.pagerduty+json;version=2"}, "Authorization":[]string{"Token token=u+XP5yPH7pzsQ3aJA5EQ"}, "Content-Type":[]string{"application/json"}, "User-Agent":[]string{"go-pagerduty/1.5.0"}}
req.Body:
{"override":{"start":"2021-06-29T12:00:00Z","end":"2021-06-29T18:00:00Z","user":{"id":"P6QKA9O"}}}

err: HTTP response with status code 400, JSON error object decode failed: json: cannot unmarshal string into Go struct field APIError.error of type []string

resp:
resp.Status: 400 Bad Request
resp.Header: http.Header{"Access-Control-Allow-Headers":[]string{"Authorization, Content-Type, AuthorizationOauth, X-EARLY-ACCESS"}, "Access-Control-Allow-Methods":[]string{"GET, POST, PUT, DELETE, OPTIONS"}, "Access-Control-Allow-Origin":[]string{"*"}, "Access-Control-Expose-Headers":[]string{""}, "Access-Control-Max-Age":[]string{"1728000"}, "Cache-Control":[]string{"no-cache"}, "Connection":[]string{"keep-alive"}, "Content-Length":[]string{"95"}, "Content-Type":[]string{"application/json"}, "Date":[]string{"Sun, 04 Jul 2021 08:00:24 GMT"}, "Server":[]string{"nginx"}, "X-Request-Id":[]string{"f6cda459-587c-49a6-ab98-17fc5bd60642"}}
resp.Body:
{"error":{"message":"Invalid Override","code":2001,"errors":"Invalid input for this override"}}

```

Note that `error.errors` is not an array, it's a single string. This seems to
violate the API spec.
@theckman theckman force-pushed the better_last_response_handling branch 2 times, most recently from 8796c89 to 26d0025 Compare July 5, 2021 21:21
In prepration for #305, this adds a mechanism to inspect all API requests and
responses handled by the client. That way if you want to figure out why
something isn't working as expected, or you'd like to make use of an
undocumented field/feature, you have access to the response and can use it.

This also adds a `Do()` method, with the same signature as `*http.Client.Do()`,
that allows consumrs to generate their own request, have the client add
authentication details and other headers, before sending the request to the
PagerDuty API. This allows consumers to further debug and make use of features
not yet supported in the REST client.

Also, updates Version string to v1.5.0 as this is work towards that release.

Lastly, this updates some of the test files to remove some duplication across
them. These changes were needed due to the new fields added to the struct that
had to be initialized to make HTTP requests.
@theckman theckman force-pushed the better_last_response_handling branch from 26d0025 to 4f01c5b Compare July 5, 2021 21:22
Copy link
Contributor

@stmcallister stmcallister left a comment

Choose a reason for hiding this comment

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

Overall, looks great! I did have one question on the change_events_test that I dropped in that file.

authToken: "foo",
HTTPClient: defaultHTTPClient,
}
client := defaultTestClient(server.URL, "foo")
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically, shouldn't these also be passing values for the v2EventsAPIEndpoint?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@theckman theckman merged commit a4171cc into master Aug 29, 2021
theckman added a commit that referenced this pull request Sep 4, 2021
This is a breaking change.

This is one of the changes that will need to be made to complete issue #305,
which is being done in a minor release even though it does contain breaking
changes.

The internal implementation details of how this was implemented meant it never
worked in a way that consumers could use, because the body was always empty.
Seeing as this API never worked and we do not wish to support it, we are going
to remove it so that anyone who may have depended on it, but missed that it was
broken, can be made aware.

If someone wanted to capture the full response of these API calls, they can now
do so using the the functionality added in #325 (4f01c5b).
theckman added a commit that referenced this pull request Sep 4, 2021
This is a breaking change.

This is one of the changes that will need to be made to complete issue #305,
which is being done in a minor release even though it does contain breaking
changes.

The internal implementation details of how this was implemented meant it never
worked in a way that consumers could use, because the body was always empty.
Seeing as this API never worked and we do not wish to support it, we are going
to remove it so that anyone who may have depended on it, but missed that it was
broken, can be made aware.

If someone wanted to capture the full response of these API calls, they can now
do so using the the functionality added in #325 (4f01c5b).
theckman added a commit that referenced this pull request Sep 4, 2021
This is a breaking change.

This is one of the changes that will need to be made to complete issue #305,
which is being done in a minor release even though it does contain breaking
changes.

The internal implementation details of how this was implemented meant it never
worked in a way that consumers could use, because the body was always empty.
Seeing as this API never worked and we do not wish to support it, we are going
to remove it so that anyone who may have depended on it, but missed that it was
broken, can be made aware.

If someone wanted to capture the full response of these API calls, they can now
do so using the the functionality added in #325 (4f01c5b).
theckman added a commit that referenced this pull request Sep 4, 2021
This is a breaking change.

This is one of the changes that will need to be made to complete issue #305,
which is being done in a minor release even though it does contain breaking
changes.

The internal implementation details of how this was implemented meant it never
worked in a way that consumers could use, because the body was always empty.
Seeing as this API never worked and we do not wish to support it, we are going
to remove it so that anyone who may have depended on it, but missed that it was
broken, can be made aware.

If someone wanted to capture the full response of these API calls, they can now
do so using the the functionality added in #325 (4f01c5b).
theckman added a commit that referenced this pull request Sep 4, 2021
This is a breaking change.

This is one of the changes that will need to be made to complete issue #305,
which is being done in a minor release even though it does contain breaking
changes.

The internal implementation details of how this was implemented meant it never
worked in a way that consumers could use, because the body was always empty.
Seeing as this API never worked and we do not wish to support it, we are going
to remove it so that anyone who may have depended on it, but missed that it was
broken, can be made aware.

If someone wanted to capture the full response of these API calls, they can now
do so using the the functionality added in #325 (4f01c5b).
@theckman theckman deleted the better_last_response_handling branch November 14, 2021 00:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants