-
Notifications
You must be signed in to change notification settings - Fork 7
Upfront Identity Fetching #327
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
Conversation
ASYNC_EXPIRATION_LENGTH = 600 # 10 minutes | ||
|
||
def initialize | ||
def initialize(_options = {}) |
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.
Why is the options parameter added back into the method header?
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 suppose this is not necessary right now, but I may want to add back the before refresh callback. Is there a use case maybe with token file reading in SSO?
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 do see that in v3 we have the before_refresh
option in certain providers and @before_refresh
member in the RefreshingCredentials
, but I don't immediately see where this is used in the SDK code. Going through the default chain at least I don't think we use before_refresh
for SSOCredentials. What use case were you thinking of?
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.
It's used by customers. My thought was we can make it a list of callbacks and not just one proc, then we can register a default one that also reread the token file instead of building it into refresh, but I don't know if we care to do that. It was just a thought.
gems/smithy-client/spec/smithy-client/plugins/resolve_auth_spec.rb
Outdated
Show resolved
Hide resolved
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 think it looks good overall and changes to V4 are minor. Let me know if you end up keeping the options in the RefreshingIdentityProvider initializer.
Rework of identity and credentials to be more like SDK v3.
Major changes: