Skip to content

fix: add models ConfigMap checksum and correct docs defaults#6

Merged
dhilgaertner merged 2 commits intomainfrom
feature/testing-go-live
Mar 9, 2026
Merged

fix: add models ConfigMap checksum and correct docs defaults#6
dhilgaertner merged 2 commits intomainfrom
feature/testing-go-live

Conversation

@dhilgaertner
Copy link
Contributor

Summary

Changes

  • templates/deployment.yaml: Added checksum/models annotation alongside existing checksum/env and checksum/secret
  • docs/CONFIGURATION.md: Changed default values from false to true for openaiModeration and passthrough.enabled

Test plan

  • helm template renders all three checksum annotations correctly
  • Docs defaults now match values.yaml
  • Verify helm upgrade with changed modelsConfig triggers pod rollout

Closes #3
Closes #4

🤖 Generated with Claude Code

dhilgaertner and others added 2 commits March 9, 2026 14:18
Add checksum/models annotation to deployment so pods restart when
modelsConfig changes via helm upgrade. Fix CONFIGURATION.md defaults
for guardrails.openaiModeration and passthrough.enabled to match
values.yaml (both are true, not false).

Closes #3
Closes #4

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Collaborator

@dgershman dgershman left a comment

Choose a reason for hiding this comment

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

Code & Security Review

Critical Issues

None — no blocking issues found.

Security Review

Strengths:

  • checksum/models annotation correctly ensures pods restart when model configuration changes, preventing stale config drift
  • Template syntax is correct and consistent with existing checksum/env and checksum/secret patterns
  • .gitignore additions are appropriate for developer tooling files

Concerns:

  • logging.requestBody and logging.responseBody changed from falsetrue in values.yaml — This is a behavioral default change, not just a docs fix, but it's not mentioned in the PR description. Enabling request/response body logging by default means sensitive user data (prompts, completions, potentially PII) will be stored in logs. The chart itself notes this "increases storage usage". For a gateway handling LLM traffic, defaulting to full body logging could have privacy implications. Consider whether this should default to false for safety, or at minimum document this as an intentional change in the PR description.
  • Similarly, guardrails.openaiModeration and passthrough.enabled were changed from falsetrue in both values.yaml and docs. The PR description frames these as docs-only fixes, but they are also behavioral changes to defaults. If the values.yaml on main already had these as true, disregard — but the diff shows they were false on main.

Code Quality

  • checksum/models annotation at templates/deployment.yaml:19 — clean, follows existing convention
  • Helm lint passes with no errors
  • helm template renders all three checksum annotations correctly
  • .gitignore: .playwright-mcp/ and .claude/settings.local.json are reasonable additions
  • Minor: PR also adds uiSessionSecret: "" to values.yaml (visible in full diff vs main) which isn't mentioned in the PR description

Summary Table

Priority Issue
🟡 Yellow Logging defaults changed to true — potential privacy/storage concern, undocumented in PR description
🟡 Yellow PR description says "fix docs defaults" but values.yaml defaults also changed for guardrails, passthrough, and logging
🟢 Green Undocumented uiSessionSecret addition to values.yaml

Recommendation: Comment — The core fix (checksum/models annotation) is solid. The docs alignment work is good. However, I'd like clarity on whether the values.yaml default changes (especially logging body capture) are intentional behavioral changes or accidental. If intentional, the PR description should reflect that these are default changes, not just docs fixes.

@dhilgaertner
Copy link
Contributor Author

@dgershman yes intentional values.yaml defaults changed after doing some testing and that request/response logging was off by default in helm chart values.

@dhilgaertner dhilgaertner merged commit 28b23ea into main Mar 9, 2026
1 check passed
@dhilgaertner dhilgaertner deleted the feature/testing-go-live branch March 9, 2026 22:02
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.

Docs: Wrong defaults in CONFIGURATION.md Bug: Models ConfigMap checksum missing from deployment annotations

2 participants