feat: Update oss charts #15719
feat: Update oss charts #15719
Conversation
…pdate extraEnv usage. Add PodDistributionBudget into all deployments
xpuska513
requested review from
supertopher and
git-phu
and removed request for
supertopher
github-actions
bot
added
area/platform
|
|
| app.airbyte.io/fullname: {{ include "airbyte.fullname" . }} | ||
| annotations: | ||
| helm.sh/hook: pre-install,pre-upgrade | ||
| helm.sh/hook-weight: "-1" |
| secretKeyRef: | ||
| name: webapp-secrets | ||
| key: {{ $k }} | ||
| {{- end }} |
There was a problem hiding this comment.
the spacing for this end block seems inconsistent with likes 97-98. I don't know what helm standards are.
does this tabbing match helm standard practice?
There was a problem hiding this comment.
tabbing for the gotmpl stuff is not required in general, unless you just want the code to look more beautified
There was a problem hiding this comment.
I always want the code to look pretty.
I don't have enough depth here to have strong opinions though so as long as we are consistent ¯\(ツ)/¯
There was a problem hiding this comment.
Thanks.
Two major comments:
- Can we think of a better way to not leak the GSM abstraction? Or at least make it clear this is Airbyte Cloud only/a way for users to inject secret maps?
- We don't need a worker HPA in OSS. Is this only for Cloud? Can we override it on inheritance?
|
Hi @davinchia ,
|
| secretKeyRef: | ||
| name: webapp-secrets | ||
| key: {{ $k }} | ||
| {{- end }} |
There was a problem hiding this comment.
I always want the code to look pretty.
I don't have enough depth here to have strong opinions though so as long as we are consistent ¯\(ツ)/¯
| app.airbyte.io/fullname: {{ include "airbyte.fullname" . }} | ||
| annotations: | ||
| helm.sh/hook: pre-install,pre-upgrade | ||
| helm.sh/hook-weight: "-1" |
There was a problem hiding this comment.
Curious, why is this -1?
| extraEnv: [] | ||
| extraEnv: {} | ||
| secrets: {} | ||
| gsm: {} |
There was a problem hiding this comment.
should we get rid of all the gsm values?
| - name: gcs-log-creds-volume | ||
| secret: | ||
| secretName: {{ ternary (printf "%s-gcs-log-creds" ( .Release.Name )) (.Values.global.credVolumeOverride) (eq .Values.global.deploymentMode "oss") }} | ||
| {{- end }} | ||
| {{- if .Values.extraVolumes }} | ||
| {{ toYaml .Values.extraVolumes | nindent 6 }} | ||
| {{- end }} |
charts/airbyte-server/values.yaml
Outdated
|
|
||
| secrets: {} | ||
| gsm: {} |
There was a problem hiding this comment.
same here on removing this.
There are a few other places too.
| @@ -656,6 +656,9 @@ worker: | |||
| ## | |||
| extraVolumes: [] | |||
|
|
|||
| hpa: | |||
There was a problem hiding this comment.
we can also get rid of this
There was a problem hiding this comment.
Thanks @xpuska513 !
Looks like there is still minor leftover values - please remove them before merging.
There are also some Cloud related deployment bits here with the volume mounts. At least it's clear what these are for - so not a big issue.
Is there documentation to clarify how secrets and the extra env blocks are handled? We definitely need those. Can do in a separate PR. Thanks for the good work!
|
@davinchia i'll do a separate pr for docs update, right now working on that one |
* fix: revert extraEnv delition in values.yaml for bootloader * add newline * feat: Update bootloader,webapp,server. Add way of defining secrets, update extraEnv usage. Add PodDistributionBudget into all deployments * feat: Update oss charts, make them able to be ingested in cloud deployment * fix: include #15685 changes * fix: Update Chart.yaml. fix minio deployment conditional operator * fix: fix EOF in worker, update worker HPA conditional * fix: remove cloud related stuff * fix: add conditional for hooks * fix: remove hooks for worker * fix: update nit, remove gsm * fix: fix nits * fix: remove gsm and hpa from values.yaml
| @@ -2,7 +2,7 @@ | |||
| apiVersion: v1 | |||
| kind: Service | |||
| metadata: | |||
| name: {{ include "common.names.fullname" . }} | |||
| name: airbyte-server-svc | |||
There was a problem hiding this comment.
Just wonder why we hard-coded the service names. This kinda messed up the branch deployments :|
What
Recommended reading order
charts/x/deployment.yamlcharts/x/secrets.yamlcharts/x/pdb.yamlcharts/x/hpa.yamlcharts/x/values.yaml🚨 User Impact 🚨
Are there any breaking changes? What is the end result perceived by the user? If yes, please merge this PR with the 🚨🚨 emoji so changelog authors can further highlight this if needed.