-
Notifications
You must be signed in to change notification settings - Fork 71
Skip creating serviceaccount #52
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
helm/values.yaml
Outdated
| serviceAccount: | ||
| # Specifies whether a service account should be created | ||
| create: true | ||
| create: false |
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 this may break users installing for the first time, unless you're relying on eksctl (or something similar) to create the serviceaccount.
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.
Agree with @ellistarn here. The defaults should be set for new users.
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.
Today, the service account is always created by eksctl create iamserviceaccount first . step 3 in Readme and step3-step6 on Deploying the AWS API Controller. And they will run into error due to this if they follows the README or Deployment Guide
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.
My guess is that the vast majority of your users will not be using eksctl to manage their clusters. Better to flip this variable to false in the helm install for your getting started guide, but leave the helm default to true.
helm/values.yaml
Outdated
| serviceAccount: | ||
| # Specifies whether a service account should be created | ||
| create: true | ||
| create: false |
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.
Agree with @ellistarn here. The defaults should be set for new users.
jaypipes
left a comment
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.
👍
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.