Return the hash with the token #506
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
@lucasvianna We do need to get a CLA signature from you in order to merge your pull request. Otherwise, I can reproduce your change in a separate pull request. |
|
@dazuma, thank you for reviewing this PR. I already submitted my CLA signature but haven't received an update so far. Feel free to reproduce my change if it helps to get the fix released sooner. |
lib/googleauth/base_client.rb
Outdated
| @@ -37,6 +37,8 @@ def apply! a_hash, opts = {} | |||
| hash = Digest::SHA256.hexdigest token | |||
| Google::Logging::Message.from message: "Sending auth token. (sha256:#{hash})" | |||
| end | |||
|
|
|||
| a_hash | |||
There was a problem hiding this comment.
@lucasvianna Actually, I should have looked at this more carefully. Prior to the change you referenced in #505, this method returned not the hash but the header value (i.e. the string "Bearer #{token}"). If you wanted to restore the original behavior, we'd need to do something different here. So just want to confirm whether you want the string or the full hash.
Also, on thinking about this, apply! isn't meant for you to use the return value. That's what apply is for. I'm happy to make a change here to restore the original behavior because it doesn't hurt anything, but I also recommend not depending on behavior that isn't actually documented in the method comments.
There was a problem hiding this comment.
@dazuma you're right. I changed it to reflect the old behavior. That's what I meant since the beginning. Thank you for pointing that out.
Also, I'll do what you recommend and use apply instead.
Related to this issue #505