Skip to content
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

Allow to skip creating serviceaccount #144

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

aehrt55
Copy link
Contributor

@aehrt55 aehrt55 commented Jan 30, 2024

Sometimes we need to reuse created service account for deployment or cronjob, add .Value.serviceaccount.skipCreation to control whether service account should be created.

Copy link
Contributor

@bluesky6529 bluesky6529 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! I left some comment, please check it, thanks!
And I think Chart.yaml also need to bump version for this PR

@@ -1,9 +1,15 @@
{{- if .Values.serviceaccount }}
{{- if .Values.serviceaccount.skipCreation }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We usually use create as a switch to determine whether resources need to be generated.

Comment on lines +3 to +5
{{- /*
Skip create service account
*/}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think users should be able to determine whether the resource will be created or not by themselves, so having this comment here would be too much.

@@ -140,12 +140,14 @@ envFrom: {}
# - test-2-secret

# Service account is used by pod. For more details on fields "serviceaccount" and "serviceAccount", please have a look on ./cronjob/templates/serviceaccount.yaml
# "serviceAccount" is deprecated, please use "serviceaccount" instead.
serviceaccount: {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the following changes will make more common sense as below:

serviceaccount: 
  create: true
  annotations: 
    eks.amazonaws.com/role-arn: <aws-role-arn>
    name: <serviceaccount-name>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants