Skip to content

Conversation

@therealnb
Copy link
Collaborator

See stacklok/toolhive#2605 (comment) and stacklok/toolhive#2606 (comment).
These are minor changes to make the new operator code work.

Signed-off-by: nigel brown <nigel@stacklok.com>
@claude
Copy link

claude bot commented Nov 18, 2025

PR Review: Minor changes to env passing

Summary

This PR simplifies the Helm chart configuration by removing duplicate environment variable definitions and clarifying the relationship between the env field and podTemplateSpec.

✅ Positive Changes

  1. Eliminates Duplication: Removes the redundant env array that duplicated all environment variables already defined in podTemplateSpec.spec.containers[0].env
  2. Clearer Documentation: The comment "NOTE: When using podTemplateSpec, define env vars there instead of here to avoid duplicates" makes the configuration pattern explicit
  3. Reduces Confusion: Setting env: [] as the default is clearer than having duplicate definitions that might conflict
  4. Removes MCP_PORT Override: The removal of MCP_PORT from podTemplateSpec with the comment that it's "automatically set by the operator from spec.port" is correct and prevents conflicts

🔍 Observations

  1. Breaking Change Risk: Users upgrading who rely on the top-level env field will need to migrate their values to podTemplateSpec.spec.containers[0].env. Consider documenting this in release notes.
  2. Template Behavior: Verify that the Helm template properly handles env: [] - some templates concatenate arrays and empty arrays should work fine, but worth confirming.

📝 Suggestions

  1. Consider adding a comment in the values file showing an example of how to add custom env vars to podTemplateSpec for users who aren't familiar with Kubernetes pod specs
  2. The database configuration section (lines 127-161) still references SQLite paths that differ from the podTemplateSpec env vars (/tmp/mcp_optimizer.db vs /data/mcp_optimizer.db). Consider aligning these or documenting why they differ.

✅ Code Quality

  • Change is minimal and focused (-36 lines, +5 lines)
  • Aligns with the operator's responsibility for setting MCP_PORT
  • Clear comments explaining the intent

Verdict

LGTM - This is a sensible simplification that reduces duplication and clarifies configuration patterns. Just ensure migration guidance is provided for existing users.

Copy link
Collaborator

@tgrunnagle tgrunnagle left a comment

Choose a reason for hiding this comment

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

Might be worth calling out the breaking k8s config change in 'release notes' whenever the cut a new one.

@therealnb therealnb merged commit e5c2891 into main Nov 18, 2025
6 checks passed
@therealnb therealnb deleted the toolhive-2606 branch November 18, 2025 14:47
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.

3 participants