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

Remove *http.Response returns from API methods #305

Closed
5 tasks done
theckman opened this issue Mar 5, 2021 · 3 comments
Closed
5 tasks done

Remove *http.Response returns from API methods #305

theckman opened this issue Mar 5, 2021 · 3 comments
Milestone

Comments

@theckman
Copy link
Collaborator

theckman commented Mar 5, 2021

Currently we have some methods that return an *httpResponse, like this one:

func (c *Client) CreateRuleset(r *Ruleset) (*Ruleset, *http.Response, error) {

Although, digging further I noticed that in many cases we read the response body and don't replace it:

return getRulesetFromResponse(c, resp, err)

if dErr := c.decodeJSON(resp, &target); dErr != nil {

go-pagerduty/client.go

Lines 314 to 318 in 1a0c44e

func (c *Client) decodeJSON(resp *http.Response, payload interface{}) error {
defer resp.Body.Close()
decoder := json.NewDecoder(resp.Body)
return decoder.Decode(payload)
}

So, the only thing consumers can read from the *http.Response are the headers, and not the response. We should just remove this as one of the breaking changes in v1.5.0, since it never really worked.

Impacted Files

Impacted Methods

  • AssignTags
  • AssignTagsWithContext
  • AssociateServiceDependencies
  • CreateBusinessService
  • CreateRuleset
  • CreateRulesetRule
  • CreateRulesetRuleWithContext
  • CreateTag
  • CreateTagWithContext
  • DisassociateServiceDependencies
  • DisassociateServiceDependencies
  • GetIncidentAlert
  • GetRuleset
  • GetRulesetRule
  • GetTag
  • GetTagWithContext
  • ListBusinessServiceDependencies
  • ListTechnicalServiceDependencies
  • ManageIncidentAlerts
  • UpdateBusinessService
  • UpdateRuleset
  • UpdateRulesetRule
  • UpdateRulesetRuleWithContext
@theckman
Copy link
Collaborator Author

So I think I came up with a pattern we can use that allows us to remove this from the majority of the API, while also giving consumers a debug capability to allow them to capture the response if they need it. I'll raise a PR for that once v1.4.0 is out.

theckman added a commit that referenced this issue Apr 23, 2021
In prepration for #305, this adds a mechanism to inspect all API responses
received 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, 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.
theckman added a commit that referenced this issue Apr 23, 2021
In prepration for #305, this adds a mechanism to inspect all API responses
received 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, 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 deduplication 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 added a commit that referenced this issue May 8, 2021
In prepration for #305, this adds a mechanism to inspect all API responses
received 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, 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 deduplication 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 added a commit that referenced this issue May 29, 2021
In prepration for #305, this adds a mechanism to inspect all API responses
received 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, 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 deduplication 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 added a commit that referenced this issue May 29, 2021
In prepration for #305, this adds a mechanism to inspect all API responses
received 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, 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 deduplication 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 added a commit that referenced this issue Jul 4, 2021
In prepration for #305, this adds a mechanism to inspect all API requsts 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 deduplication 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 added a commit that referenced this issue Jul 5, 2021
In prepration for #305, this adds a mechanism to inspect all API requsts 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 deduplication 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 added a commit that referenced this issue Jul 5, 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
Copy link
Collaborator Author

theckman commented Sep 4, 2021

So to my comment above, that was #325 which is now merged. I can start looking at this, finally.

@theckman theckman changed the title Explore removing *http.Response returns from API methods Remove *http.Response returns from API methods Sep 4, 2021
theckman added a commit that referenced this issue 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 issue 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 issue 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 issue 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 issue 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
Copy link
Collaborator Author

theckman commented Nov 9, 2021

This is done.

@theckman theckman closed this as completed Nov 9, 2021
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

No branches or pull requests

2 participants