-
Notifications
You must be signed in to change notification settings - Fork 446
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 documenation for AWS pod identity providers #56
Add documenation for AWS pod identity providers #56
Conversation
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 - Added a few small suggestions but overall very clear, thank you!
@jeffhollan @ahmelsayed We might want to introduce some version information in our docs so that folks know when it is introduced. Thoughts?
provider: kiam # Optional. Default: false | ||
``` | ||
|
||
Kiam will give access to containers based on the role provided by the `iam.amazonaws.com/role` annotation on the pod. |
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.
Thinking out loud - Since this is documentation on how to configure it for our dependency, should we document this here or merely point to their docs? For example, what if they change their approach?
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 not expect kiam to change. I assume it will be superseded, rather than changed.
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 can remove these lines though. I understand the skepticism of essentially documenting another project.
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.
Maybe just add a link instead, but I think you already did it somewhere else, right?
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 in the first paragraph. I will remove thsese sections.
Co-Authored-By: Tom Kerkhove <kerkhove.tom@gmail.com>
Co-Authored-By: Tom Kerkhove <kerkhove.tom@gmail.com>
'kiam' -> 'aws-kiam' 'eks' -> 'aws-eks'
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.
Apologies know this has been open for a bit. This LGTM. @zach-dunton-sf is this still accurate and ready to merge in?
LGTM, ok to merge or still pending changes @zach-dunton-sf ? |
This reverts commit 60129df.
No description provided.