[GitHub] [trafficcontrol] rawlinp opened a new pull request #5334: Refactor TO Go client internals to use a consistent, shared method

[GitBox <gi...@apache.org>]

rawlinp opened a new pull request #5334:
URL: https://github.com/apache/trafficcontrol/pull/5334


   ## What does this PR (Pull Request) do?
   Refactor the TO Go client methods to all use the same set of internal methods (`to.post()`, `to.put()`, `to.get()`, `to.del()`, ) that each funnel down into the same shared method (`to.makeRequestWithHeader()`).
   
   Before this PR, there were a handful of different ways the TO Go client methods made requests, each of them slightly different and inconsistent in their own ways. By funneling all the top-level methods down into the same, shared, consistent method, all the top-level methods now make requests in the same consistent manner. By doing this, it allows us to remove a tremendous amount of duplicated code that has been copied and pasted into new methods as the client has grown.
   
   Additionally, this PR fixes countless little bugs among various methods in the TO Go client:
   - returning `nil` instead of the `err` from decoding the response body
   - not using `url.QueryEscape()` or `url.Values{}.Encode()` to properly escape query strings
   - unchecked errors
   - not setting `ReqInf.RemoteAddress` properly
   
   The vast majority of top-level methods returned an error if the http status code was not 304 or < 300. There were a few I had to change in order to make them consistent with the entire client:
   - deliveryservice_requests
   - roles
   - staticdnsentries
   
   These client methods now return an error if the status code is not 304 or < 300, so I had to modify a few TO API tests to reflect that new change in behavior. I don't expect this change in behavior to impact client users very much, since most users (if any) will be checking `err != nil` anyways. In general, the consistency that this change brings to the client will make it less surprising and easier to use.
   
   Note: this PR doesn't change the signatures of the top-level client methods, but it does make `ErrUnlessOkOrNotModified` private (as it should've always been), since it should not be exposed and used publicly.
   
   ## Which Traffic Control components are affected by this PR?
   
   - Traffic Control Client (Go)
   
   ## What is the best way to verify this PR?
   Verify the TO API tests pass, since they heavily exercise the TO Go client.
   
   ## The following criteria are ALL met by this PR
   
   - [x] This PR includes tests
   - [x] internal refactor, no docs necessary
   - [x] This PR includes an update to CHANGELOG.md
   - [x] This PR includes any and all required license headers
   - [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://www.apache.org/security/) for details)


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] rob05c merged pull request #5334: Refactor TO Go client internals to use a consistent, shared method

[GitBox <gi...@apache.org>]

rob05c merged pull request #5334:
URL: https://github.com/apache/trafficcontrol/pull/5334


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org