Skip to content

Conversation

@michaellzc
Copy link
Member

@michaellzc michaellzc commented Oct 7, 2024

this can result in duplicated env var and cause weird behaviour.

Checklist

Test plan

n/a

@michaellzc michaellzc requested review from a team, Chickensoupwithrice and eseliger and removed request for a team October 7, 2024 19:49
@michaellzc
Copy link
Member Author

@eseliger this is what caused the broken kustomize build | kubectl apply -f when rolling out MT

Comment on lines -62 to -65
{{- range $name, $item := .Values.frontend.env }}
- name: {{ $name }}
{{- $item | toYaml | nindent 10 }}
{{- end }}
Copy link
Member

Choose a reason for hiding this comment

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

I have no clue about this - are there any env vars in frontend that migrator would also need? I think in the MVU case it might need things like "where's the KMS secrets" etc 🤔 But not exactly sure there. I could see this being used at customers .. 🤔

cc @sourcegraph/release

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 this is fine.

for db connection string, we don't read from frontend.env, it's always computed from the partial template

- name: migrator
image: {{ include "sourcegraph.image" (list . "migrator") }}
imagePullPolicy: {{ .Values.sourcegraph.image.pullPolicy }}
args: {{- default (list "up") .Values.migrator.args | toYaml | nindent 8 }}
env:
{{- if not .Values.migrator.databaseAuthOverrideEnvVars }}
{{- include "sourcegraph.databaseAuth" (list . "pgsql" "PG") | nindent 8 }}
{{- include "sourcegraph.databaseAuth" (list . "codeIntelDB" "CODEINTEL_PG") | nindent 8 }}
{{- include "sourcegraph.databaseAuth" (list . "codeInsightsDB" "CODEINSIGHTS_PG") | nindent 8 }}
{{- end }}

not sure about the KMS example?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm concerned this might be used in autoupgrade -- reading 👀 brain is cooked from the release stuff

Copy link
Contributor

@DaedalusG DaedalusG Oct 8, 2024

Choose a reason for hiding this comment

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

Alright I think these are all we need:

      - PGHOST=pgsql
      - PGPORT=5432
      - PGUSER=sg
      - PGPASSWORD=sg
      - PGDATABASE=sg
      - PGSSLMODE=disable

      - CODEINTEL_PGHOST=codeintel-db
      - CODEINTEL_PGPORT=5432
      - CODEINTEL_PGUSER=sg
      - CODEINTEL_PGPASSWORD=sg
      - CODEINTEL_PGDATABASE=sg
      - CODEINTEL_PGSSLMODE=disable

      - CODEINSIGHTS_PGHOST=codeinsights-db
      - CODEINSIGHTS_PGPORT=5432
      - CODEINSIGHTS_PGUSER=postgres
      - CODEINSIGHTS_PGPASSWORD=password
      - CODEINSIGHTS_PGDATABASE=postgres
      - CODEINSIGHTS_PGSSLMODE=disable

And those should all be generated by this

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, thank you

@michaellzc michaellzc merged commit 96b6393 into main Oct 8, 2024
11 checks passed
@michaellzc michaellzc deleted the michaellzc-patch-1 branch October 8, 2024 20:35
@marcleblanc2
Copy link
Contributor

This was a breaking change, which impacted self-hosted customers upgrading, which should have been communicated to them in advance of upgrading >= v5.8

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.

5 participants