Fix Helm chart environment variable merging for groupFiltering.allowedGroups

[aponcedeleonch]

Closes: #199

• Fixed groupFiltering.allowedGroups not being applied to podTemplateSpec by implementing automatic env var merging
• Created new template helper that merges ALLOWED_GROUPS, database URLs, and custom env vars into podTemplateSpec
• Updated documentation to clarify that mcpserver.env is merged with podTemplateSpec defaults
• Changed SQLite path from /tmp to /data for consistency with emptyDir volume mount
• Bumped chart version to 0.1.1 and appVersion to 0.2.0

🤖 Generated with Claude Code

[claude]

PR Review

✅ Strengths

Problem Solved: Correctly fixes the groupFiltering.allowedGroups not being applied due to env var merging issue.

Clean Implementation: The new mcp-optimizer.podTemplateSpec helper (lines 102-124 in _helpers.tpl) elegantly solves env var merging using:

• deepCopy to avoid mutating source values
• Logical append order: base → auto-generated (DB URLs, ALLOWED_GROUPS) → custom
• Clear separation of concerns

Documentation: Comprehensive README updates explain the env var merging behavior well.

🔍 Issues & Recommendations

1. Potential Duplicate Environment Variables (Critical)

The helper doesn't check for duplicates before appending. If podTemplateSpec.spec.containers[0].env already contains ASYNC_DB_URL, DB_URL, or ALLOWED_GROUPS, they'll appear twice in the final spec.

Fix: Add deduplication logic (last value wins):

{{- $finalEnv := list }}
{{- $envMap := dict }}
{{- range concat $container.env $additionalEnvs .Values.mcpserver.env }}
  {{- $_ := set $envMap .name . }}
{{- end }}
{{- range $key, $val := $envMap }}
  {{- $finalEnv = append $finalEnv $val }}
{{- end }}

2. Silent Failure if podTemplateSpec is Missing

Line 35 in mcpserver.yaml: {{- if .Values.mcpserver.podTemplateSpec }} means no podTemplateSpec is rendered if undefined. Since the chart now depends on it for env vars, this creates inconsistency.

Fix: Either remove the conditional (always require podTemplateSpec) or fail explicitly if missing.

3. SQLite Path Inconsistency

values.yaml line 135 changed path from /tmp to /data, but SQLITE_TMPDIR still points to /tmp (line 64). While functionally okay due to volume mounts, it's confusing.

Recommendation: Either rename SQLITE_TMPDIR to SQLITE_DATADIR or document why tmpdir ≠ db path.

4. Documentation Clarity

README states custom env vars from mcpserver.env are "appended" but doesn't explain precedence. Users may not realize earlier values can be shadowed if duplicates aren't handled.

📊 Summary

Must Fix: Issue #1 (duplicate env vars)
Should Fix: Issue #2 (conditional podTemplateSpec)
Consider: Issues #3-4 (naming/docs)

Overall solid fix, but needs duplicate handling to be production-ready.

[tgrunnagle]

Nice!

[aponcedeleonch]

@claude please check if your comments have been addressed

[claude]

Claude finished @aponcedeleonch's task —— View job

---

Review Comments Status

Todo List

• Read current state of changed files
• Check Issue Update python Docker tag to v3.14 #1: Duplicate environment variables handling
• Check Issue Dependency Dashboard #2: Silent failure for missing podTemplateSpec
• Check Issue Helm charts not released #3: SQLite path inconsistency
• Check Issue chore(deps): update python docker tag to v3.14 #4: Documentation clarity
• Provide summary of findings

---

✅ All Issues Addressed

All four issues from my previous review have been successfully addressed:

Issue #1: Duplicate Environment Variables (Critical) ✅ FIXED

• Implemented deduplication logic in _helpers.tpl:120-129
• Uses a map to ensure each env var name appears only once
• Last value wins as recommended

Issue #2: Silent Failure for Missing podTemplateSpec ✅ FIXED

• Removed the conditional in mcpserver.yaml:35
• podTemplateSpec is now always rendered, ensuring consistency

Issue #3: SQLite Path Inconsistency ✅ FIXED

• Changed SQLITE_TMPDIR from /tmp to /data in values.yaml:64
• Now consistent with the SQLite database path /data/mcp_optimizer.db

Issue #4: Documentation Clarity ✅ FIXED

• Added clear "Environment Variable Precedence" section in README.md:134-140
• Explains the three-tier precedence system (base → auto-generated → custom)
• Documents that last value wins for duplicates

Summary

The PR is now production-ready. All critical and recommended issues have been properly addressed with clean implementations and comprehensive documentation.

---

Job Run: https://github.com/StacklokLabs/mcp-optimizer/actions/runs/${{ github.run_id }}
Branch: fix-allowed-groups