search

IBM / go-sdk-core

The go-sdk-core repository contains core functionality required by Go code generated by the IBM OpenAPI SDK Generator.
Apache License 2.0
30 stars 24 forks source link

feat: add detailed error response to iam/cp4d authenticators #66

Closed jorge-ibm closed 4 years ago

jorge-ibm commented 4 years ago

This PR creates a DetailedResponse instance for both the IAM/CP4D authenticators in the case where the api call to either servers fails (returns a status code we consider a failure). This DetailedResponse instance contains the response status code, headers, and the raw byte result.

I've created a AuthenticationErrorstruct that contains an instance of DetailedResponse along with the err field which contains the actual error object. Whenever we expect a detailed response to be returned from the Authenticate method due to an error, we attempt to cast the returned error into AuthenticationError type. If successful, the detailed response is included in the returned value for the Base Service's Request method.

ref: https://github.ibm.com/arf/planning-sdk-squad/issues/2030

jorge-ibm commented 4 years ago

After some discussion with @padamstx, there's some needed refactoring on my set of changes. I will re-request reviews once those changes are done.

mkistler commented 4 years ago

This looks like it will be a breaking change, since it changes an external interface. If that's correct, we just need to make sure we include the right markers in the commit message so that semantic release does the right thing.

padamstx commented 4 years ago

This looks like it will be a breaking change, since it changes an external interface. If that's correct, we just need to make sure we include the right markers in the commit message so that semantic release does the right thing.

Agree... I hadn't considered this angle yet, but we do in fact have code other than the Go core itself that explicitly calls the Authenticate() method, so we'll need to package this as a new major version.

@jorge-ibm To do this, we'll need to do two things:

  1. Change the commit message to include the BREAKING CHANGE marker in the body of the commit message.
  2. We'll need to rename the top-level v4 directory to be v5 and modify .bumpversion.cfg to reflect the new directory name

Then once this change has been merged in, we'll need to change the generator to use "v5" in the import path used for the core. Let's discuss so we can coordinate this.

Edit: Actually, I just thought of a possible way to avoid a new major version in this situation... Because the new AuthenticationError struct implements the "error" interface, I think we can change the return type back to just "error" and other code using Authenticate() can continue thinking that it is just a simple error object on which they can call the Error() method. When the BaseService.Request() method detects a non-nil error object returned by Authenticate() it could do a type assertion to "cast" it as an AuthenticationError and then extract the DetailedResponse field, then return it along with the error message to complete the error processing.

jorge-ibm commented 4 years ago

@padamstx @mkistler I've pushed a commit addressing the review comments along with running go fmt ./... to auto-format some code.

ibm-devx-automation commented 4 years ago

:tada: This PR is included in version 4.2.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: