Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Credentials Management #888
Credentials Management #888
Changes from 21 commits
409af50
51c094f
74ae1c3
1989d51
aadf828
cdfb8a9
102655c
b7b6445
5ed5f73
9fd898f
6a83b2e
b5abf17
2ee351e
dcb2d4c
f14b125
f47fad3
29c760f
ef8e981
2440bbf
004d6b9
7380cce
c848df8
23f6962
c34820f
150f199
4094ee4
4a0e321
676ae7b
3a7afa6
f7491f3
7712177
5025c19
6f33ac4
bc32e0d
b262bc7
6c76476
5f38544
ab3807e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Nit: I feel like we can preemptively make these generic (
AWS.getHelp
,AWS.viewLogs
or something). Also, capitalize "Logs"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'm going to leave these strings as credentials concerns for now, but a separate pass over all of the text to look for general-purpose opportunities is a good idea.
Capitalizing Logs
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 this something that should be attached to the credential itself rather than a top-level on
awsContext
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.
awsContext contains an AwsContextCredentials object, which then backs these calls. Would it make more sense to have a single getter for that object instead of getters for each property?
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.
nit: I really don't like the "create a mutable variable, apply a conditional and maybe re-assign it's value cos it's convenient" pattern - I know it's tightly scoped but I really like to avoid mutability where possible. It's a smell.
I think you're better off changing this to something like :
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 completely agree. Prefer immutable where possible. Will change
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 get that this will make it more seamless for a user but I don't care to have this lying around if it's solely for that purpose. I'd say just invalidate the existing credentials and have the user log back in...it's fast enough that it shouldn't be an issue. At most, give them a one-version grace period and mark this for deletion in the next release afterwards.
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.
This can be removed after a few versions. It makes for a better experience than seeing an "invalid credentials" notification, or a phantom log-out. It's low cost, the comment explains what is going on, and git history will tell us (if necessary) when this was added.
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.
what is this for?
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 is a hash of the contents that were used to produce the credentials.
Its purpose is to detect when things like Shared Credentials Profiles change during a toolkit session.
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.
Nit: change the name to
getOrCreateCredentials
; put the actions first.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.
Good catch
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.
Unfortunately, this was added in a separate PR, so the rename belongs in a separate PR.
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 cache has been updated to treat Credentials + Hash as one object, so I added the rename as well.
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.
Are these bubbled to the user? Should they be localized?
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.
They're only surfaced to the user in the form of logs. Might be a good idea to surface the error to the user through the interface, but we definitely don't want to localize the logs.
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.
They are only logged (not placed in a notification). The current approach has been to only localize contents that are sent to the UI, and not logged materials.
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.
Take a look at the revamped notification in the PR description. The intent was to steer people to the logs if they were interested, without having to route specific details about the error into the UI.
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.
Nit: the toolkit wasn't connected to AWS in the first place.
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 an array / list really the right data-structure here? We seem to be doing lookup-style operations mostly that point to a dictionary / map being a better choice.
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 top access is a mix of "list all providers" and "get provider x" (the doc is currently open in #889 ). Collection still seems reasonable to me -- the lookups don't seem like they would have a heavy performance.
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.
This "splitting by
:
" logic seems to have leaked into a bunch of places. A "CredentialProvider" has a type and identifier which together form a 'compound-style' key that we can use to store them - but couldn't that concern be localized into acredential ID
object itself?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 is only explicit in the tests, for readability. Anywhere in the toolkit code goes through the make/decode methods.
I initially stopped short of creating a credential provider id object. Since the intent was to support additional data in the future (using the separator), its probably saving future pain to make and pass around an object now instead of a string.
I'll give this a go.