-
Notifications
You must be signed in to change notification settings - Fork 28
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
Fix token refresh #3
Conversation
maxlaverse
commented
Mar 4, 2018
- Stores the Token between calls
- Add tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
break; | ||
case "expires_in": | ||
try { | ||
token.expire = System.currentTimeMillis() + Long.parseLong(pair.getValue()) * 1000 - EARLY_EXPIRE_DELAY; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is 100ms enough here - what if I wanted to grab this for use in a build in a pipeline - there could be a few minutes delay before starting a script that needs the credential and the use of the credential?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. I overlooked it thought it was seconds. I don't know what a good value would be but 5 minutes sounds reasonnable.
@jtnord Do you have an Openshift installation do test it ? |
@maxlaverse I only have k8s not openshift distribution. |
I will have to check if |
Thanks for the review @jtnord |