feat: add support for custom environment variable expansion#539
feat: add support for custom environment variable expansion#539
Conversation
✅ Docs preview readyThe preview is ready to be viewed. View the preview File Changes 0 new, 1 changed, 0 removedBuild ID: a7e05f447a36fe36d782297e URL: https://www.apollographql.com/docs/deploy-preview/a7e05f447a36fe36d782297e |
gocamille
force-pushed
the
camille/env-var-expansion
branch
from
824a742 to
e6fcaf0
Compare
|
❌ Changeset file missing for PR All changes should include an associated changeset file. |
DaleSeo
left a comment
DaleSeo
left a comment
There was a problem hiding this comment.
Thanks @gocamille, for implementing this so cleanly with solid tests! I believe this should meet the customer's request. However, I wanted to mention that most features in Apollo MCP Server have been following Router's patterns since they've been refined and tested over several years. This approach also ensures a consistent experience across Apollo products.
As I was reviewing this, I took a look at how Apollo Router implements the same functionality: https://github.com/apollographql/router/blob/dev/apollo-router/src/configuration/expansion.rs
Here are a couple of differences worth noting:
- Router performs type coercion during expansion. Environment variables that contain
trueor10can be converted to YAML booleans or numbers. The current implementation only does string substitution before YAML parsing, which is simpler but requires users to be careful about how values are interpreted. - Architecturally, Router's expansion module is part of the library and available to crate consumers, while this implementation is limited to the binary's runtime module. That works for now, but it's something to keep in mind if we expect other consumers to want this functionality.
This isn't blocking anything, just flagging it for consideration.
| } | ||
|
|
||
| #[test] | ||
| fn it_prioritizes_apollo_mcp_env_over_expanded_vars() { |
There was a problem hiding this comment.
These tests are really helpful to understand the precedence behavior!
|
❌ Changeset file missing for PR All changes should include an associated changeset file. |
|
❌ Changeset file missing for PR All changes should include an associated changeset file. |
Co-authored-by: Dale Seo <5466341+DaleSeo@users.noreply.github.com>
|
❌ Changeset file missing for PR All changes should include an associated changeset file. |
…hidden from the LLM via meta entry on the tool
|
Thank you so much for your thorough review @DaleSeo ! I appreciated the insight on achieving logical parity with our long-standing Router features, that makes sense! I did take that suggestion and updated the core
This PR is only targeting a very narrow band of features for environment variable expansion, but we could look into adding the following in the future to achieve parity with apollo-router (and consider other ways we'd want to extend it), including:
|
| @@ -0,0 +1,15 @@ | |||
| ### feat: add support for custom environment variable expansion - @gocamille PR #539 | |||
|
|
|||
| ## Summary | |||
There was a problem hiding this comment.
Shouldn't we use #### here instead? I'm concerned that this might break the structure when we add it to the changelog.
There was a problem hiding this comment.
Good point -- I'll update this!
3 participants
Summary
This PR adds support for
${env.VAR_NAME}syntax in configuration files, allowing users to reference custom environment variables without being limited to theAPOLLO_MCP_*naming convention.Closes #454.
Changes
runtime/env_expansion.rs(new module) - parser for variable expansionruntime.rs(modified) - integrates expansion into theread_config()functionconfig-file.mdx- updated docs with syntax, escaping, and special characters handlingNote The
APOLLO_MCP_*environment variable(s) will still take precedence over expanded custom config values (no breaking change).