Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Feb 21, 2025

Conversation

Chickensoupwithrice
Copy link
Contributor

@Chickensoupwithrice Chickensoupwithrice commented Feb 5, 2025

@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.

Copy link

vercel bot commented Feb 5, 2025

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

Copy link
Contributor

@jdpleiness jdpleiness left a comment

Choose a reason for hiding this comment

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

@Chickensoupwithrice
Copy link
Contributor Author

ahhh, fuck 🤔

@marcleblanc2
Copy link
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
Contributor Author

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

Strange that it worked in my testing though? 🤔

@marcleblanc2
Copy link
Contributor

marcleblanc2 commented Feb 7, 2025 via email

@Chickensoupwithrice
Copy link
Contributor Author

Chickensoupwithrice commented Feb 7, 2025

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
Copy link
Contributor

marcleblanc2 commented Feb 7, 2025

  • 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 }}

Copy link
Collaborator

@MaedahBatool MaedahBatool left a comment

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. :)

Copy link
Contributor

@marcleblanc2 marcleblanc2 left a comment

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.

@Chickensoupwithrice
Copy link
Contributor Author

Thank you @marcleblanc2 🙏🏽 I addressed your comments

@Chickensoupwithrice Chickensoupwithrice enabled auto-merge (squash) February 20, 2025 18:45
@Chickensoupwithrice Chickensoupwithrice merged commit c922425 into main Feb 21, 2025
5 checks passed
@Chickensoupwithrice Chickensoupwithrice deleted the al/fix-sslmode-docs branch February 21, 2025 21:27
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