Skip to content

Add mapping between values key and names used for multi-word resources#94

Closed
caugustus-sourcegraph wants to merge 1 commit into
caug/use-secrets-databasefrom
caug/add-mapping
Closed

Add mapping between values key and names used for multi-word resources#94
caugustus-sourcegraph wants to merge 1 commit into
caug/use-secrets-databasefrom
caug/add-mapping

Conversation

@caugustus-sourcegraph
Copy link
Copy Markdown
Contributor

Goal: I need to be able to reference the auto-generated database secret by name. The secret name is hardcoded (which is not ideal) but even if it were generated, the same problem would occur (example: we would still need to be able to predict the "codeintel-db" section of this line).

My untested hack to use snakecase $service to convert codeIntelDB to codeintel-db didn't work (it produces code-intel-db, of course).

This approach is pretty much me admitting defeat: I can't think of a better way to map codeIntelDB to codeintel-db. It's a little goofy putting this mapping into the values.yaml since it's not intended to be customized, but maybe leaving it in un-documented will be sufficient?

Checklist

Test plan

A followup PR actually generates the correct secret references

@caugustus-sourcegraph caugustus-sourcegraph requested a review from a team April 6, 2022 18:10
@michaellzc
Copy link
Copy Markdown
Member

I ran into a similar problem in #52 (comment)

{{- define "sourcegraph.serviceAccountName" -}}
{{- $top := index . 0 }}
{{- $service := index . 1 }}
{{- $defaultServiceAccountName := ((snakecase $service) | replace "_" "-") }}
{{- default $defaultServiceAccountName (index $top.Values $service "serviceAccount" "name") }}
{{- end }}

What's your thought on introducing another breaking change? e.g., we rename the values codeInsightDB -> codeintelDb?

If not, I think we should update serviceAccoutName partial template to follow the same conventino.

@caugustus-sourcegraph
Copy link
Copy Markdown
Contributor Author

caugustus-sourcegraph commented Apr 6, 2022

@michaellzc Actually I realized I'm dumb for introducing a new variable - couldn't we just prepopulate codeIntelDB.name with codeintel-db, instead of adding codeIntelDB.mappingName? And then your serviceAccountName logic could be simpler too:

 {{- $defaultServiceAccountName := (index $top.Values $service "name")
 {{- default $defaultServiceAccountName (index $top.Values $service "serviceAccount" "name") }} 

@michaellzc
Copy link
Copy Markdown
Member

@michaellzc Actually I realized I'm dumb for introducing a new variable - couldn't we just prepopulate codeIntelDB.name with codeintel-db, instead of adding codeIntelDB.mappingName? And then your serviceAccountName logic could be simpler too:

 {{- $defaultServiceAccountName := (index $top.Values $service "name")
 {{- default $defaultServiceAccountName (index $top.Values $service "serviceAccount" "name") }} 

🙃 I was dumb too.

Yes!

@caugustus-sourcegraph
Copy link
Copy Markdown
Contributor Author

Great work everyone, we discovered this is a bad idea

@michaellzc michaellzc deleted the caug/add-mapping branch April 7, 2022 21:42
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