Repository navigation
feat(BE-8827): adopt AC_ env naming + optional Config Service bootstrap - #12
Closed
osmankibar wants to merge 1 commit into
Closed
osmankibar wants to merge 1 commit into
osmankibar wants to merge 1 commit into
Conversation
Renames every env var this service reads to the AC_-prefixed convention (doc/internal-doc/docs/development/backend/conventions/environment-variables.md), checking the new name first and falling back to the original - same dual-name pattern the rest of BE-8827 uses everywhere else (Appcircle.Core's Rijndael/bucket-prefix fallbacks etc.): - APPCIRCLE_ACCESS_TOKEN -> AC_API_ACCESS_TOKEN - APPCIRCLE_API_URL -> AC_API_EXTERNAL_URL - APPCIRCLE_MCP_ALLOWED_HOST -> AC_ALLOWED_HOST - APPCIRCLE_MCP_PORT -> AC_PORT - LOG_LEVEL -> AC_LOG_MIN_LEVEL - APPCIRCLE_EXCLUDED_TOOLSETS -> AC_EXCLUDED_TOOLSETS - AC_MCP_ENABLE_WRITE_TOOLS -> AC_WRITE_TOOLS_ENABLED (drops the redundant own-service "MCP" component) Also adds server.py's _bootstrap_hosted_config(), resolving env through Appcircle ConfigService (ac-lib-python-config's appcircle-config-client) for Appcircle's own hosted deployment (mcp.appcircle.io) only. This repo is a public, MIT-licensed package customers `pip install .` directly onto their own machines - unlike ac-codepush-service/ac-web-event, a hard dependency on the private-Nexus-hosted config-client would break every customer install. Confirmed the approach with the user before touching pyproject.toml: appcircle-config-client is an optional "hosted" extra (pip install ".[hosted]"), and _bootstrap_hosted_config() no-ops - doesn't even attempt the import - unless AC_CONFIG_SERVICE_URL is set, which no customer install ever does. Full env-var mapping, the confirmed-vs-not-yet-confirmed context/secret references, and a documented known limitation (AC_LOG_MIN_LEVEL isn't resolved in time because core.logger's module-level read happens before main() runs) are in docs/tasks/BE-8827/spec.md. Also fixed a real regression this surfaced in an existing test (test_invalid_port_env_falls_back_to_default expected the literal env var name in the warning log; the AC_PORT-first rewrite had dropped it) - see plan.md. Verified: 711/711 unit tests pass (707 pre-existing + 4 new _bootstrap_hosted_config() tests). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: appcircleio/appcircle-mcp/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Member
|
Cancelling this PR as discussed with @osmankibar |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
AC_-prefixed convention, checking the new name first and falling back to the original - a customer's existing config (documented in this repo's own README/installation guides) keeps working unchanged._bootstrap_hosted_config()toserver.py, resolving env through Appcircle ConfigService (ac-lib-python-config'sappcircle-config-client) for Appcircle's own hosted deployment (mcp.appcircle.io) only.The constraint this repo has that other PL-123/BE-8827 consumers didn't
appcircle-mcp-serveris public/MIT-licensed - customersgit clone+pip install .this directly onto their own machines.appcircle-config-clientlives on the private Nexus registry, unreachable from a customer's machine. Making it a hard dependency would break every customer install. Confirmed the optional-dependency approach with the requester before touchingpyproject.toml- seedocs/tasks/BE-8827/spec.mdfor the full reasoning.appcircle-config-clientis an optionalhostedextra (pip install ".[hosted]");_bootstrap_hosted_config()doesn't even attempt the import unlessAC_CONFIG_SERVICE_URLis set, which no customer install ever does.Env var mapping
APPCIRCLE_ACCESS_TOKENAC_API_ACCESS_TOKENAPPCIRCLE_API_URLAC_API_EXTERNAL_URLAPPCIRCLE_MCP_ALLOWED_HOSTAC_ALLOWED_HOSTAPPCIRCLE_MCP_PORTAC_PORTLOG_LEVELAC_LOG_MIN_LEVELAPPCIRCLE_EXCLUDED_TOOLSETSAC_EXCLUDED_TOOLSETSAC_MCP_ENABLE_WRITE_TOOLSAC_WRITE_TOOLS_ENABLEDFull mapping table + confirmed-vs-not-yet-confirmed
envdef.yamlcontext/secret references + a documented known limitation (AC_LOG_MIN_LEVELisn't resolved in time for the logger, sincecore.logger's module-level read happens beforemain()runs) are indocs/tasks/BE-8827/spec.md.Still open (infra-side, out of scope here)
mcp.appcircle.iodeployment's Docker image/manifest - this repo's own exampleDockerfilewas deliberately left untouched (also plausibly used by customers self-hosting via Docker).mcp_external_urltoAppcircle.ConfigService'scontextdef.yaml.service: appcircle-mcpinenvdef.yamlagainst Config Service's real onboarding record.Test plan
pytest test/unit/suite: 711/711 pass (707 pre-existing + 4 new_bootstrap_hosted_config()tests)🤖 Generated with Claude Code