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

deployment: use overridden service account name if specified #93

Merged
merged 1 commit into from
Oct 22, 2024

Conversation

USA-RedDragon
Copy link
Contributor

@USA-RedDragon USA-RedDragon commented Oct 11, 2024

Prior to this patch, if a serviceAccount.name is specified, the deployment would always use the namespace name as the name.

After this patch, the serviceAccountName will check for a custom value before defaulting to the namespace

Related: #91

Prior to this patch, if a `serviceAccount.name` is specified, the deployment would always use the namespace name as the name. 

After this patch, the `serviceAccountName` will check for a custom value before defaulting to the namespace
@USA-RedDragon USA-RedDragon requested a review from a team as a code owner October 11, 2024 20:51
@USA-RedDragon
Copy link
Contributor Author

@jkerry @miguelhrocha could this get your attention? PR #91 was missing this change, causing anyone trying to use serviceAccount.name to have issues as the TFE pod's serviceAccountName is hard-coded.

Copy link
Contributor

@nikolasrieble nikolasrieble left a comment

Choose a reason for hiding this comment

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

Thank you very much for following up, this is a clear bug fix.
I left a minor suggestion that is very much a nit for your consideration.

Comment on lines +67 to 71
{{- if .Values.serviceAccount.name}}
serviceAccountName: {{ .Values.serviceAccount.name }}
{{- else }}
serviceAccountName: {{ .Release.Namespace }}
{{- end }}
Copy link
Contributor

Choose a reason for hiding this comment

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

If we were to use the default function, this could be simplified a tad:

Suggested change
{{- if .Values.serviceAccount.name}}
serviceAccountName: {{ .Values.serviceAccount.name }}
{{- else }}
serviceAccountName: {{ .Release.Namespace }}
{{- end }}
serviceAccountName: {{ .Values.serviceAccount.name | default .Release.Namespace }}

@nikolasrieble
Copy link
Contributor

As this is a bug fix and we are preparing a release, lets land this. The suggested change is functionally equivalent.

@nikolasrieble nikolasrieble merged commit 7602c54 into hashicorp:main Oct 22, 2024
2 checks passed
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