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.

Sign up for GitHub

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

Jump to bottom

Support custom service account name #52

Merged
merged 1 commit into from
Mar 2, 2022
Merged

Support custom service account name #52

merged 1 commit into from
Mar 2, 2022

Conversation

michaellzc
Copy link
Member

@michaellzc michaellzc commented Feb 28, 2022 •
edited
Loading

This PR enables users to override service account for all deployed resources

Test plan

Initial deployment

kind create cluster

override.yaml

storageClass:
  create: false # Disable if you have your own existing storage class
  name: standard

sourcegraph:
  localDevMode: true
helm upgrade --install -n sourcegraph -f charts/sourcegraph/override.yaml sourcegraph charts/sourcegraph/

Enable SA

Updated override.yaml

storageClass:
  create: false # Disable if you have your own existing storage class
  name: standard

sourcegraph:
  localDevMode: true

codeInsightsDB:
  serviceAccount:
    create: true
codeIntelDB:
  serviceAccount:
    create: true
githubProxy:
  serviceAccount:
    create: true
gitserver:
  serviceAccount:
    create: true
indexedSearch:
  serviceAccount:
    create: true
minio:
  serviceAccount:
    create: true
pgsql:
  serviceAccount:
    create: true
preciseCodeIntel:
  serviceAccount:
    create: true
redisCache:
  serviceAccount:
    create: true
redisStore:
  serviceAccount:
    create: true
repoUpdater:
  serviceAccount:
    create: true
searcher:
  serviceAccount:
    create: true
symbols:
  serviceAccount:
    create: true
syntectServer:
  serviceAccount:
    create: true
tracing:
  serviceAccount:
    create: true
worker:
  serviceAccount:
    create: true
 helm diff -n sourcegraph -f charts/sourcegraph/override.yaml sourcegraph charts/sourcegraph/
  • Verify rendered manifest is expected
  • Verify sourcegraph server still works

fix https://github.com/sourcegraph/sourcegraph/issues/31889

@michaellzc
Copy link
Member Author

michaellzc commented Feb 28, 2022 •
edited
Loading

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

michaellzc
michaellzc commented Feb 28, 2022
View reviewed changes
charts/sourcegraph/templates/_helpers.tpl
Comment on lines +44 to +53
{{- $defaultServiceAccountName := ((snakecase $service) | replace "_" "-") }}
{{- default $defaultServiceAccountName (index $top.Values $service "serviceAccount" "name") }}
{{- end }}

{{- define "sourcegraph.renderServiceAccountName" -}}
{{- $top := index . 0 }}
{{- $service := index . 1 }}
{{- if or (index $top.Values $service "serviceAccount" "create") (index $top.Values $service "serviceAccount" "name") }}
serviceAccountName: {{ include "sourcegraph.serviceAccountName" (list $top $service) }}
{{- end }}
Copy link
Member Author

Choose a reason for hiding this comment

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

a different implementation would be rendering serviceAccountName: default whenever .serviceAccount.create=true and .serviceAccount.name!="

They work the same tho

michaellzc
michaellzc commented Feb 28, 2022
View reviewed changes
charts/sourcegraph/templates/_helpers.tpl
@@ -41,7 +41,16 @@ Create the name of the service account to use
{{- define "sourcegraph.serviceAccountName" -}}
{{- $top := index . 0 }}
{{- $service := index . 1 }}
{{- default $service (index $top.Values $service "serviceAccount" "name") }}
{{- $defaultServiceAccountName := ((snakecase $service) | replace "_" "-") }}
Copy link
Member Author

Choose a reason for hiding this comment

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

I think the convention of resource name is dashed instead of camelCase

this line does the following

camelCase -> snake_case -> replace("_", "-")

repoUpdater -> repo_updater -> repo-updater

codeInsightsDb -> code_insights_db -> code-insights-db
in this case, ideally we want codeinsights-db to be consistent with the deployment name, but the top-level key in values.yaml is codeInsightsDB instead of `codeinsightsDB.

@michaellzc michaellzc force-pushed the Support_custom_service_account_name branch from 7fa74f9 to fcf2721 Compare February 28, 2022 22:55
@michaellzc michaellzc requested review from a team and caugustus-sourcegraph February 28, 2022 22:56
@michaellzc michaellzc marked this pull request as ready for review February 28, 2022 23:00
This was referenced Mar 1, 2022
Merged
Draft
@michaellzc michaellzc force-pushed the 02-25-Revamp_config_docs_with_helm-docs branch from 3929944 to bdddee6 Compare March 1, 2022 05:38
@michaellzc michaellzc force-pushed the Support_custom_service_account_name branch from fcf2721 to 6b435c7 Compare March 1, 2022 05:38
@michaellzc michaellzc mentioned this pull request Mar 1, 2022
Merged
caugustus-sourcegraph
caugustus-sourcegraph approved these changes Mar 1, 2022
View reviewed changes
charts/sourcegraph/values.yaml Outdated Show resolved Hide resolved
@michaellzc michaellzc force-pushed the 02-25-Revamp_config_docs_with_helm-docs branch from bdddee6 to 26210d7 Compare March 1, 2022 21:32
@michaellzc michaellzc force-pushed the Support_custom_service_account_name branch 2 times, most recently from 1844b85 to af95c21 Compare March 1, 2022 21:52
@michaellzc michaellzc force-pushed the 02-25-Revamp_config_docs_with_helm-docs branch from 26210d7 to 1366f4b Compare March 1, 2022 21:55
@michaellzc michaellzc force-pushed the Support_custom_service_account_name branch 2 times, most recently from bdc40a3 to f878f46 Compare March 1, 2022 21:56
@michaellzc michaellzc force-pushed the 02-25-Revamp_config_docs_with_helm-docs branch from 1366f4b to 09f629a Compare March 1, 2022 22:05
@michaellzc michaellzc force-pushed the Support_custom_service_account_name branch from f878f46 to 4cd6dd7 Compare March 1, 2022 22:05
@michaellzc
Copy link
Member Author

michaellzc commented Mar 2, 2022 •
edited
Loading

Graphite Merge Job

Current status: ✅ Merged

This pull request was successfully merged as part of a stack.

This comment was auto-generated by Graphite.

Job Reference: yNqXdfNl9dvIL5h0XluD

Base automatically changed from 02-25-Revamp_config_docs_with_helm-docs to main March 2, 2022 20:30
@michaellzc michaellzc force-pushed the Support_custom_service_account_name branch from 4cd6dd7 to 86b0fd9 Compare March 2, 2022 20:32
@michaellzc michaellzc merged commit fac583b into main Mar 2, 2022
@michaellzc michaellzc deleted the Support_custom_service_account_name branch March 2, 2022 20:33
@michaellzc michaellzc mentioned this pull request Apr 6, 2022
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caugustus-sourcegraph caugustus-sourcegraph caugustus-sourcegraph approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Helm: support custom service account name
2 participants
@michaellzc @caugustus-sourcegraph