-
Notifications
You must be signed in to change notification settings - Fork 567
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 the time when it will expire to debug log #746
Conversation
aba4cb6
to
1eb5eed
Compare
pkg/awsconfig/awsconfig.go
Outdated
@@ -113,13 +113,13 @@ func (p *CredentialsProvider) Load() (*AWSCredentials, error) { | |||
} | |||
|
|||
// Expired checks if the current credentials are expired | |||
func (p *CredentialsProvider) Expired() bool { | |||
func (p *CredentialsProvider) Expired() (bool, *time.Time) { |
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.
The function name doesn't really do what it says now.
Why not create a new method ExpiresAt
to return the *time.Time
instead?
f25bb11
to
7b9add7
Compare
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.
Looks nice 👍 just a small comment on using value semantic
pkg/awsconfig/awsconfig.go
Outdated
@@ -122,6 +122,15 @@ func (p *CredentialsProvider) Expired() bool { | |||
return time.Now().After(creds.Expires) | |||
} | |||
|
|||
func (p *CredentialsProvider) ExpiredAt() *time.Time { |
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.
I would recommend passing it as value, since a time.Time
is a measure of Time.
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.
Just noticed 👀, can we perhaps use previousCreds.Expires instead of adding ExpiredAt?
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.
@mikutas that is indeed a lot simpler :-)
FWIW, LGTM! 🚀 |
8d48e13
to
08656cf
Compare
@mapkon please rebase |
Do you want me to merge this? |
yes please consider merging. @mikutas found this useful for debugging |
Do we want to know when to expire even if credentials are not expired for now ?