dagster-io / dagster

An orchestration platform for the development, production, and observation of data assets.
https://dagster.io
Apache License 2.0
11.58k stars 1.46k forks source link

Making dagster-user-deployments a map instead of a list would make injecting tags during CI/CD easier #8172

Open dagsir[bot] opened 2 years ago

dagsir[bot] commented 2 years ago

Issue from the Dagster Slack

This issue was generated from the slack conversation at: https://dagster.slack.com/archives/C014N0PK37E/p1654194223055249?thread_ts=1654194223.055249&cid=C014N0PK37E


Conversation excerpt

U03AE3Z2A0H: This is a broad question about the Helm chart dagster-user-deployments Here the deployments section is made up of array of deployments with names. The problem with arrays in Yaml is that they are replaced when layered with overrides, so if I have base values set in the base chart, and only want to update the tag of deployment[0] in a separate values.yaml file, it will completely overwrite my deployment[0]’th item U016C4E5CP8: Hi Binoy, this is one option - you can do something like:

--set-string dagster-user-deployments.deployments[0].image.tag=NEW_TAG_VALUE when running the helm upgrade command, while also applying a values.yaml file to handle everything else other than the tags U016C4E5CP8: as long as the square brackets are escaped properly U03AE3Z2A0H: Sure but it makes it hard to track which user deployment is to be affected .. coding it to index based lookup vs name based lookups. U016C4E5CP8: fair enough - maybe wrapping it a script that maps the names to indexes? U03AE3Z2A0H: just out of curiosity, was this intentional or was the values.yaml file layering / yaml merging aspect missed. I am talking more in terms of using CI/CD to merge things in the code commit pipelines U03AE3Z2A0H: modifying more parameters via cli set-string makes it harder to manage on the long run.. U016C4E5CP8: This is good feedback, I can file an issue for this U016C4E5CP8: <@U018K0G2Y85> issue Making dagster-user-deployments a map instead of a list would make injecting tags during CI/CD easier


Message from the maintainers:

Do you care about this too? Give it a :thumbsup:. We factor engagement into prioritization.

rfoster2576 commented 2 years ago

👍

danielgafni commented 6 months ago

I've encountered this issue with ArgoCD and argocd-image-updater - image updater creates ArgoCD Helm parameters like deployments[0].image.tag = my-tag, which overwrites the whole deployments[0] map.

danielgafni commented 6 months ago

I tried implementing the required changes in helm/dagster/charts/dagster-user-deployments/templates/helpers/_helpers.tpl, but failed miserably.

I tried to write a map -> array deployments converter, something like this:

# _helpers.tpl
{{- define "dagsterUserDeployments.deploymentsArray" -}}
{{- if kindIs "map" .Values.deployments }}
# convert map to list
{{- $deployments := .Values.deployments }}
{{- range $name, $deployment := $deployments }}
- name: {{ $name }}
{{ $deployment | toYaml | indent 2 }}  # toYaml needed or not??
{{- end }}
{{- else }}
# just return the list as is
{{- .Values.deployments | toYaml }}  # toYaml needed or not???
{{- end }}
{{- end }}

but was unable to use it correctly

However, I think this should be a really simple task for someone with better helm knowledge, right @gibsondan @alangenfeld ?