Refactor

[tri-adam]

Use functional arguments for NewClient. Modify (*Client).NewRequest to accept a context and a relative URL reference. Add (*Client).Do method. Add HTTPError type to represent unsuccessful HTTP responses, utilizing Go 1.13 error wrapping. Improve documentation around errors. Improve testing.

Return version string directly from (*Client).GetVersion.

Accept all 2xx HTTP status codes as successful.

[cclerget]

@tri-adam Would it be possible to also pass a callback function in order to grab the body content response or copy the content to retrieve if after the request ? In case of success it could be helpful to display content to give some hint. As for error by example in pkg/sypgp/sypgp.go there is an hardcoded hint to guide users about authentication, as this is something specific to an endpoint the keyserver could provide this hint instead via the JSON error response with a hint field maybe ?

[tri-adam]

Hi @cclerget, apologies for the slow response!

I'm making the assumption you're referring solely to (*Client).PKSLookup(), as that's the relevant API for the specific part of the code you reference in the sypgp package.

I'm not sure there is much use to have an alternative way to get a copy of the body when the request was successful, since it is already returned in its entirety in this case.

When the request is unsuccessful, it will return a (*github.com/sylabs/scs-key-client/client).HTTPError, which a caller could then use errors.As() and (*HTTPError).Code() to obtain the HTTP status code. Since the HKP draft specification really doesn't define error/hint formats, this is likely a good level for Singularity to do its logic at in terms of whether authentication is necessary.

The use case you describe about providing hints was actually the inspiration for the github.com/sylabs/json-resp package. Since all Sylabs HKP servers use that format, this module will also return a wrapped jsonresp.Error when the response contains that format, making it possible to return more detailed help to the user via the Message field.

But, back to your question... if the ask is to handle some other type of error format, I would suggest that exposing the response body via a Body() method on the (*github.com/sylabs/scs-key-client/client).HTTPError would be the idiomatic way to do so. The caller could then implement any sort of custom logic they choose to mine any clues from the content of the response.

Let me know what you think, and thanks for taking the time to provide feedback!

[cclerget]

Hi @tri-adam , I'm referring to PKSAdd as there is key server like Hagrid https://keys.openpgp.org/about/api by example which are providing some hint as a result of a push because it does additional verification regarding the key being uploaded and it would be nice to return the response message on CLI then a user could proceed with verification by following instructions returned by the server without being misguided by a message like : your key has been successfully pushed. So just returning nil from PKSAdd as a success can't cover those cases.
That's not really related to error as this is pretty much covered and can be done from singularity at this level

[tri-adam]

Ah, that makes sense, thanks for the clarification. I've opened #47 to track that. I think it's best kept separate from this refactoring, given the scope of this is already quite large.

[dtrudg]

LGTM me and pairs with singularity via apptainer/singularity#5689 ok. 👍