Skip to content

fix: we use sslmode not pgsslmode in the deploy repos#957

Merged
Chickensoupwithrice merged 3 commits into
mainfrom
al/fix-sslmode-docs
Feb 21, 2025
Merged

fix: we use sslmode not pgsslmode in the deploy repos#957
Chickensoupwithrice merged 3 commits into
mainfrom
al/fix-sslmode-docs

Conversation

@Chickensoupwithrice

@Chickensoupwithrice Chickensoupwithrice commented Feb 5, 2025

Copy link
Copy Markdown
Contributor

@marcleblanc2 pointed this out: https://linear.app/sourcegraph/issue/REL-638/configure-aws-rds-databases-for-tls-connections-in-helm-chart

Pull Request approval

You will need to get your PR approved by at least one member of the Sourcegraph team. For reviews of docs formatting, styles, and component usage, please tag the docs team via the #docs Slack channel.

@vercel

vercel Bot commented Feb 5, 2025

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
sourcegraph-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 20, 2025 6:51pm

@jdpleiness jdpleiness left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Chickensoupwithrice

Copy link
Copy Markdown
Contributor Author

ahhh, fuck 🤔

@marcleblanc2

Copy link
Copy Markdown
Contributor

I think the Helm chart values key is .sslmode, but the env vars are PGSSLMODE? Point kinda illustrated here, one of the results from Jacob's search.

@Chickensoupwithrice

Copy link
Copy Markdown
Contributor Author

Yeah, I think I actually need to change the helm values file.

Strange that it worked in my testing though? 🤔

@marcleblanc2

marcleblanc2 commented Feb 7, 2025 via email

Copy link
Copy Markdown
Contributor

@Chickensoupwithrice

Chickensoupwithrice commented Feb 7, 2025

Copy link
Copy Markdown
Contributor Author

It works because the there's a helper template that applies a prefix

On second thought, the docs need to reflect the difference between helm chart and env var? Need to think about this more.

@marcleblanc2

marcleblanc2 commented Feb 7, 2025

Copy link
Copy Markdown
Contributor
  • Sure, docs specific to the env var name should match the env var name
  • In this case, I'm pointing out the docs specific to the Helm override values should match the Helm chart template, as per my comment here 🙏 which looks like just a handful 🎉 . lol, this search includes a couple files I've committed to a different repo based on the docs I copy / pasted from, I'll go fix those, and communicate to the affected customers.

Key = pgsslmode

Current docs:

apiVersion: v1
kind: Secret
...
data:
...
  pgsslmode: "require" # optional, enable if using SSL

Example README.md

Key = sslmode

Current Helm template helper function:

- name: {{ printf "%sSSLMODE" $prefix }}
  valueFrom:
    secretKeyRef:
      key: sslmode
      name: {{ $secretName }}

Current Helm chart pgsql secret template

{{- if not .Values.pgsql.auth.existingSecret }}
apiVersion: v1
kind: Secret
...
data:
...
  sslmode: {{ .Values.pgsql.auth.sslmode | toString | b64enc | quote }}

@MaedahBatool MaedahBatool left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM @Chickensoupwithrice feel free to merge. :)

@marcleblanc2 marcleblanc2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

AFAIA, there needs to be a different xSSLMODE env var for each database, matching the same prefix as the other env vars for the same db.

The Helm values key, however is sslmode

The docs must match this.

Comment thread docs/admin/deploy/docker-single-container/index.mdx Outdated
Comment thread docs/admin/external_services/postgres.mdx Outdated
Comment thread docs/admin/external_services/postgres.mdx Outdated
Comment thread docs/admin/external_services/postgres.mdx Outdated
Comment thread docs/admin/external_services/postgres.mdx Outdated
Comment thread docs/admin/external_services/postgres.mdx Outdated
Comment thread docs/admin/updates/migrator/migrator-operations.mdx Outdated
Comment thread docs/admin/updates/migrator/migrator-operations.mdx Outdated
@Chickensoupwithrice

Copy link
Copy Markdown
Contributor Author

Thank you @marcleblanc2 🙏🏽 I addressed your comments

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.

4 participants