Added support to use token from managed identity #22
Conversation
anshulvermapatel
force-pushed
the
anshulvermapatel/ARO-7195-adding-support-to-use-managed-id
branch
from
695163a
to
a4668be
Compare
| a.cond.L.Unlock() | ||
| a.cond.Broadcast() | ||
| } | ||
| return token, err |
There was a problem hiding this comment.
It's bad practice in go to return both an initialised value and an error. It can cause all sorts of headaches. Recommend going with return token, nil and doing return nil, err directly when an error occurs.
There was a problem hiding this comment.
Here, we would be using the current still valid token if it is not expired and setting err to nil. If we wanna return token, nil at then end then an else condition will have to be added post line 131. There also, then from line 132 to 134 will have to be repeated in that else condition as they are necessary to be executed even if there is error.
| a.acquiring, acquire = true, true | ||
| break | ||
| } | ||
| // Getting here means that this goroutine will wait for the updated token |
There was a problem hiding this comment.
Having this comment makes me think that we need to invert the if statement or restructure the for loop
There was a problem hiding this comment.
The only thing which I can think of is putting a.cond.L.Unlock() in the first if expired section however it is gonna work the same way.
| a.cond.L.Lock() | ||
| token := a.token | ||
|
|
||
| for { |
There was a problem hiding this comment.
I think the majority of this for loop can be re-organised and simplified.
There was a problem hiding this comment.
Since this is a little complex structure, those extra lines are needed to have proper comments for someone to understand. For example if you see the if statement in the for loop, the middle else if and the last else both have the lines -
token = a.token break
However if you see the comments, both the lines are there to use the existing token but for different reasons.
|
LGTM but I lack Merge. |
| return &tokenAuthorizer{token: token} | ||
| // Get returns the underlying resource. | ||
| // If the resource is fresh, no refresh is performed. | ||
| func (a *tokenAuthorizer) aquireToken() (string, error) { |
There was a problem hiding this comment.
nit:
| func (a *tokenAuthorizer) aquireToken() (string, error) { | |
| func (a *tokenAuthorizer) acquireToken() (string, error) { |
anshulvermapatel
force-pushed
the
anshulvermapatel/ARO-7195-adding-support-to-use-managed-id
branch
from
a4668be
to
ce2d496
Compare
|
/lgtm |
Fixes - ARO-7195
Modified the tokenAuthorizer to have more fields with just the token and it caches and renews the token when expired or near to expiry.