Skip to content

feat: helm config for prod #WPB-26095#78

Open
spoonman01 wants to merge 1 commit into
mainfrom
feat/helm-config-prod-WPB-26095
Open

feat: helm config for prod #WPB-26095#78
spoonman01 wants to merge 1 commit into
mainfrom
feat/helm-config-prod-WPB-26095

Conversation

@spoonman01

Copy link
Copy Markdown

PR Submission Checklist for internal contributors

  • The PR Title

    • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
    • contains a reference JIRA issue number like SQPIT-764
    • answers the question: If merged, this PR will: ... ³
  • The PR Description

    • is free of optional paragraphs and you have filled the relevant parts to the best of your ability

What's new in this PR?

Issues

Helm setup for staging but not prod

Solutions

Remove outdated config for old authentication and put the values-prod.yml here, while waiting for ArgoCd in prod


PR Post Submission Checklist for internal contributors (Optional)

  • Wire's Github Workflow has automatically linked the PR to a JIRA issue

PR Post Merge Checklist for internal contributors

  • If any soft of configuration variable was introduced by this PR, it has been added to the relevant documents and the CI jobs have been updated.

References
  1. https://sparkbox.com/foundry/semantic_commit_messages
  2. https://github.com/wireapp/.github#usage
  3. E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

@spoonman01
spoonman01 requested a review from a team as a code owner June 23, 2026 09:00
@spoonman01 spoonman01 self-assigned this Jun 23, 2026
@claude

claude Bot commented Jun 23, 2026

Copy link
Copy Markdown

PR Review — feat: helm config for prod (WPB-26095)

Thanks for this! The change is small and focused. I reviewed the diff against the chart templates and the rest of the repo. Overall it looks good and ready to merge with a few minor points to consider.

What looks good

  • WIRE_SDK_ENVIRONMENT removal is consistent. I grepped the codebase and the variable isn't referenced in any source, so dropping it from .env.sample, values.yaml, and NOTES.txt is a clean, complete removal — no dangling references left behind.
  • Pinned image tag for prod. tag: sha-9a03ad7 is the right call for prod (immutable/reproducible) versus the default latest. It also matches the tag format produced by staging.yml (type=sha,format=short produces sha-<short>), so it resolves correctly.
  • No secrets in plaintext. DB_PASSWORD, SDK_APP_TOKEN, and CRYPTO_PASSWORD stay in the secretKeyRef path in statefulset.yaml; only non-sensitive config landed in values-prod.yaml. Good separation.

Things worth considering

  1. Helm replaces lists wholesale — keep env in sync. Helm overrides arrays rather than deep-merging by index, so values-prod.yaml's env block fully replaces the one in values.yaml. Today both list the same 4 keys, so it's fine — but if someone later adds a 5th env var to values.yaml, prod will silently NOT get it. Worth a note that they must be kept in sync (or, longer term, templating env so prod only overrides values it actually changes).

  2. Infra details committed to a public repo. The prod Aurora endpoint (apps-prod-aurora-pg.cluster-...rds.amazonaws.com) and SDK_APP_ID are now in plaintext in a public repo. These aren't secrets and the DB isn't externally reachable, but it is internal-topology disclosure. The file comment already flags the intent to move this to argocd-integrations once ArgoCd is set up for prod — that move is the right home for it, so I'd just make sure this is genuinely a temporary stopgap.

  3. Filename vs. comment/description mismatch. The file is values-prod.yaml but both the in-file comment and the PR description call it values-prod.yml. Minor, but worth aligning to avoid confusion.

  4. Default image.repository: pollapp is registry-less. Not introduced by this PR, but worth noting: the base values.yaml uses a bare pollapp while prod uses the full quay.io/wire/poll-app. Staging/default deploys therefore depend on an override existing somewhere — might be worth normalizing the default too.

Tests
No test coverage applies here — this is deploy-time config. I confirmed staging.yml only builds/pushes the image and doesn't consume these values files (presumably ArgoCd/manual helm does), so there's nothing to wire up in CI for this change.

Nice, tidy change overall — the points above are mostly nits plus a maintainability heads-up on the env list merge behavior.

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.

2 participants