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 ProviderClient.Reauthenticate() function #743

Merged
merged 1 commit into from
Mar 22, 2018

Conversation

majewsky
Copy link
Contributor

@majewsky majewsky commented Jan 28, 2018

If a user wants to do their own HTTP requests and reauthenticate in case of 401 responses, they can already use ProviderClient.ReauthFunc(), but that function is not thread-safe. This commit provides a safer alternative by pulling the relevant piece of code out of ProviderClient.Request().

For #742.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Jan 28, 2018

Build failed.

If a user wants to do their own HTTP requests and reauthenticate in case
of 401 responses, they can already use ProviderClient.ReauthFunc(), but
that function is not thread-safe. This commit provides a safer
alternative by pulling the relevant piece of code out of
ProviderClient.Request().
@coveralls
Copy link

coveralls commented Jan 28, 2018

Coverage Status

Coverage increased (+0.007%) to 73.221% when pulling f0a5d28 on sapcc:make-reauth-public into 9746ef6 on gophercloud:master.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Jan 28, 2018

Build succeeded.

@majewsky
Copy link
Contributor Author

majewsky commented Feb 7, 2018

Any opinion? I would like to have this merged soon since I want to rely on this in a library that I'm currently building.

@majewsky
Copy link
Contributor Author

@jrperritt What do you think about this?

@jrperritt
Copy link
Contributor

I can take a look at this tomorrow. At first glance, it looks reasonable to me.

Copy link
Contributor

@jrperritt jrperritt left a comment

Choose a reason for hiding this comment

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

This looks good to me. @jtopjian what do you think?

@jtopjian
Copy link
Contributor

If I understand this correctly, and forgive me if I'm being dense, the idea here is to call client.Reauthenticate() and never call client.ReauthFunc() directly?

@jrperritt
Copy link
Contributor

jrperritt commented Mar 22, 2018

@jtopjian right, I think the goal is to have thread-safe re-authentication without having to use ProviderClient.Request. It'd enable a user to, say, use a different library for the HTTP calls.

Otherwise, @majewsky , is there any reason to export the Reauthenticate method?

@majewsky
Copy link
Contributor Author

majewsky commented Mar 22, 2018

As you said. My usecase is a custom Swift client library: I tried to build it on top of ProviderClient.Request at first, but found it too limiting. I tried patching around it for a while (e.g. I set OkCodes to a 500-element slice with codes 100..599 to avoid Gophercloud's error handling and get the bare http.Response in all cases), but in the end, it emerged that the only thing I really want to re-use from Gophercloud is the authentication parts. For reference, this is the interface between my library and Gophercloud: https://github.com/majewsky/schwift/blob/master/gopherschwift/package.go#L69-L105 - This is already using the Reauthenticate method proposed here. I'm using my fork for development at the moment.

@jrperritt
Copy link
Contributor

OK, thanks. Sorry it took so long for me to get around to this

@jrperritt jrperritt merged commit b4d283c into gophercloud:master Mar 22, 2018
@majewsky majewsky deleted the make-reauth-public branch July 23, 2018 10:46
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.

None yet

4 participants