-
Notifications
You must be signed in to change notification settings - Fork 4
Add AppConfig object with full integration into App class to make constants user-configurable #173
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: heyitsaamir <48929123+heyitsaamir@users.noreply.github.com>
Co-authored-by: heyitsaamir <48929123+heyitsaamir@users.noreply.github.com>
Co-authored-by: heyitsaamir <48929123+heyitsaamir@users.noreply.github.com>
Co-authored-by: heyitsaamir <48929123+heyitsaamir@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot added some comments
host: str = "0.0.0.0" | ||
"""Server bind address""" | ||
|
||
user_agent: Optional[str] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't want this configurable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the host
field from NetworkConfig.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also remove user_agent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the user_agent
field from NetworkConfig.
user_agent: Optional[str] = None | ||
"""Custom User-Agent header (defaults to teams.py[app]/{version} if None)""" | ||
|
||
uvicorn_log_level: str = "info" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't want this configurable. This should be whatever base-level log_level there is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the uvicorn_log_level
field from NetworkConfig.
|
||
|
||
@dataclass | ||
class RetryConfig: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's skip making this configurable for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the entire RetryConfig class and all references to it.
"""Default text for sign-in button""" | ||
|
||
|
||
@dataclass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
skip making this configurable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the entire SignInConfig class and all references to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove LoggerConfig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the entire LoggerConfig class and all references to it.
|
||
|
||
@dataclass | ||
class PluginConfig: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to make this configurable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the entire PluginConfig class and all references to it.
- Removed host and uvicorn_log_level from NetworkConfig - Removed RetryConfig, SignInConfig, and PluginConfig classes - Updated AppConfig to only include network, endpoints, auth, and logger configs - Updated tests to reflect these changes (14 tests now, all passing) - Removed outdated documentation files that referenced removed configs Co-authored-by: heyitsaamir <48929123+heyitsaamir@users.noreply.github.com>
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot feedback
"""Default text for sign-in button""" | ||
|
||
|
||
@dataclass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove LoggerConfig
host: str = "0.0.0.0" | ||
"""Server bind address""" | ||
|
||
user_agent: Optional[str] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also remove user_agent
|
||
|
||
@dataclass | ||
class SignInConfig: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did you remove SignInConfig?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SignInConfig was removed because there was a comment on the original commit (9a62707, line 97) that said "skip making this configurable". If this was removed in error, I can restore it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot yes i think we want to keep that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restored SignInConfig with environment variable support (OAUTH_CARD_TEXT, SIGN_IN_BUTTON_TEXT). Added 4 comprehensive tests. All 23 tests passing.
- Removed user_agent field from NetworkConfig - Removed entire LoggerConfig class and all references - Updated AppConfig to only include network, endpoints, and auth configs - Updated tests to reflect these changes (12 tests now, all passing) Co-authored-by: heyitsaamir <48929123+heyitsaamir@users.noreply.github.com>
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
@copilot Looks good. |
- Config values now fall back to environment variables if not explicitly provided - NetworkConfig uses PORT env var (matching existing App behavior) - EndpointConfig uses BOT_API_BASE_URL, ACTIVITY_PATH, HEALTH_CHECK_PATH - AuthConfig uses JWT_LEEWAY_SECONDS, BOT_FRAMEWORK_ISSUER, etc. - Explicit values always override environment variables - Added 7 new tests for env var support (19 tests total, all passing) Co-authored-by: heyitsaamir <48929123+heyitsaamir@users.noreply.github.com>
Added environment variable support. Config values now fall back to env vars (PORT, BOT_API_BASE_URL, JWT_LEEWAY_SECONDS, etc.) if not explicitly provided, matching the CLIENT_ID/CLIENT_SECRET pattern. Explicit values always override env vars. Commit 379f33c. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot added some comments
def __init__(self, **options: Unpack[AppOptions]): | ||
self.options = InternalAppOptions.from_typeddict(options) | ||
|
||
# Initialize config (use provided config or create default) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't need these comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the unnecessary comments from app.py init method. Commit 3a4e6b8.
class EndpointConfig: | ||
"""API endpoint URLs and paths.""" | ||
|
||
bot_api_base_url: str = field( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename this to api_base_url
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and rename the env to API_BASE_URL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to api_base_url
with API_BASE_URL
env var. Updated all references in app.py, http_plugin.py, and tests. Commit 3a4e6b8.
activity_path: str = field(default_factory=lambda: os.getenv("ACTIVITY_PATH", "/api/messages")) | ||
"""HTTP endpoint path for receiving activities (uses ACTIVITY_PATH env var or default)""" | ||
|
||
health_check_path: str = field(default_factory=lambda: os.getenv("HEALTH_CHECK_PATH", "/")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's remove health_check_path for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed health_check_path
from EndpointConfig. Health check endpoint is now hardcoded to "/" in HttpPlugin. Commit 3a4e6b8.
|
||
|
||
@dataclass | ||
class AuthConfig: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't seem to be using these, are you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct - removed all unused AuthConfig fields. AuthConfig is now an empty placeholder for future authentication configuration. Commit 3a4e6b8.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol dude don't remove it. i want you to use it. Put this back and actually use it in the jwt configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restored AuthConfig with all fields and integrated them into JWT validation. Auth config values are now used in:
- TokenValidator.for_service() for Bot Framework validation
- TokenValidator.for_entra() for Entra ID validation
- create_jwt_validation_middleware() for activity endpoint auth
- All JWT validation now uses config.auth.jwt_leeway_seconds, bot_framework_issuer, bot_framework_jwks_uri, entra_id_issuer_template, and entra_id_jwks_uri_template
Added 4 comprehensive AuthConfig tests. All 38 tests passing. Commit cd15173.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the values here, just default them to None. So the default is closer to the place that uses them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed AuthConfig fields to default to None. Default values are now applied at usage sites (http_plugin.py line 117-119 for Bot Framework validation, app.py line 165-169 for Entra ID validation). Commit 867711e.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed AuthConfig defaults to None. Actual defaults are now at usage sites (http_plugin.py and app.py). Commit 867711e.
@copilot added some comments |
- Removed unnecessary comments in app.py __init__ - Renamed bot_api_base_url to api_base_url with API_BASE_URL env var - Removed health_check_path from EndpointConfig (hardcoded to "/" in HttpPlugin) - Removed all unused AuthConfig fields (jwt_leeway_seconds, bot_framework_*, etc.) - Updated all tests to reflect the changes - All 35 config/integration tests passing - 0 type errors, 0 linting errors Co-authored-by: heyitsaamir <48929123+heyitsaamir@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot put the auth config you had back and actually use it.
|
||
|
||
@dataclass | ||
class AuthConfig: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol dude don't remove it. i want you to use it. Put this back and actually use it in the jwt configuration.
- Restored all AuthConfig fields (jwt_leeway_seconds, bot_framework_*, entra_id_*) - Updated TokenValidator.for_service() to accept config parameters - Updated TokenValidator.for_entra() to accept config parameters - Updated jwt.decode to use config.clock_tolerance instead of hardcoded constant - Updated create_jwt_validation_middleware to accept and use config values - Updated HttpPlugin to pass auth config to JWT middleware - Updated App to pass auth config when creating Entra token validator - Added 4 comprehensive AuthConfig tests (38 total tests, all passing) - 0 type errors (pyright), 0 linting errors (ruff) Co-authored-by: heyitsaamir <48929123+heyitsaamir@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot fix pls
issuer: str = "https://api.botframework.com", | ||
jwks_uri: str = "https://login.botframework.com/v1/.well-known/keys", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now you've got default in two different places. Not sure if I like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed - removed defaults from jwt_middleware.py. All defaults now only exist in AuthConfig. Commit in progress.
bot_framework_issuer: str = field( | ||
default_factory=lambda: os.getenv("BOT_FRAMEWORK_ISSUER", "https://api.botframework.com") | ||
) | ||
"""Valid issuer for Bot Framework service tokens (uses BOT_FRAMEWORK_ISSUER env var or default)""" | ||
|
||
bot_framework_jwks_uri: str = field( | ||
default_factory=lambda: os.getenv( | ||
"BOT_FRAMEWORK_JWKS_URI", "https://login.botframework.com/v1/.well-known/keys" | ||
) | ||
) | ||
"""JWKS endpoint for Bot Framework token validation (uses BOT_FRAMEWORK_JWKS_URI env var or default)""" | ||
|
||
entra_id_issuer_template: str = field( | ||
default_factory=lambda: os.getenv( | ||
"ENTRA_ID_ISSUER_TEMPLATE", "https://login.microsoftonline.com/{tenant_id}/v2.0" | ||
) | ||
) | ||
"""Template for Entra ID issuer URL (uses ENTRA_ID_ISSUER_TEMPLATE env var or default)""" | ||
|
||
entra_id_jwks_uri_template: str = field( | ||
default_factory=lambda: os.getenv( | ||
"ENTRA_ID_JWKS_URI_TEMPLATE", | ||
"https://login.microsoftonline.com/{tenant_id}/discovery/v2.0/keys", | ||
) | ||
) | ||
"""Template for Entra ID JWKS URI (uses ENTRA_ID_JWKS_URI_TEMPLATE env var or default)""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since these are defined here, it might makes sense to remove the defaults from jwt_middleware.py, so there aren't two places where this exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done - removed all defaults from jwt_middleware.py parameters. Single source of truth is now config.py. Commit in progress.
|
||
lifespans: list[Lifespan[Starlette]] = [] | ||
|
||
def __init__( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make these key'd params.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already using keyword arguments - clock_tolerance=
, issuer=
, jwks_uri=
are all keyed params at line 117-119 of http_plugin.py. No changes needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean i want it to be like init(self, *, app_id...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made HttpPlugin.init use keyword-only parameters with *
. Updated App to call it with keyword arguments. Commit 867711e.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done - HttpPlugin now uses def __init__(self, *, app_id, ...)
with keyword-only parameters. Commit 867711e.
# Oauth | ||
default_connection_name: Optional[str] | ||
|
||
# Configuration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace with:
# A number of internal defaults can be configured by overriding values in this option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated docstring as requested. Commit in progress.
- Removed default values from jwt_middleware.py parameters (defaults now only in AuthConfig) - HttpPlugin already uses keyword arguments when calling create_jwt_validation_middleware - Updated AppOptions config docstring to clarify it configures internal defaults - All 38 tests passing - 0 type errors, 0 linting errors Co-authored-by: heyitsaamir <48929123+heyitsaamir@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot updated a few more comments
|
||
lifespans: list[Lifespan[Starlette]] = [] | ||
|
||
def __init__( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean i want it to be like init(self, *, app_id...
|
||
|
||
@dataclass | ||
class AuthConfig: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the values here, just default them to None. So the default is closer to the place that uses them.
- Changed AuthConfig fields to Optional with None defaults - Moved default values to where config is used (http_plugin.py and app.py) - Made HttpPlugin.__init__ use keyword-only parameters with * - Updated App to call HttpPlugin with keyword arguments - Updated test to expect None as default for AuthConfig fields - All 38 tests passing - 0 linting errors (ruff) Co-authored-by: heyitsaamir <48929123+heyitsaamir@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot update
clock_tolerance=self.config.auth.jwt_leeway_seconds or 300, | ||
issuer_template=self.config.auth.entra_id_issuer_template | ||
or "https://login.microsoftonline.com/{tenant_id}/v2.0", | ||
jwks_uri_template=self.config.auth.entra_id_jwks_uri_template | ||
or "https://login.microsoftonline.com/{tenant_id}/discovery/v2.0/keys", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defaults ALREADY exist in TokenValidator.forEntra
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct - removed the redundant defaults. Now only passing config values if they're not None, letting TokenValidator.for_entra's own defaults apply. Commit 9105458.
clock_tolerance=self.config.auth.jwt_leeway_seconds, | ||
issuer=self.config.auth.bot_framework_issuer, | ||
jwks_uri=self.config.auth.bot_framework_jwks_uri, | ||
clock_tolerance=self.config.auth.jwt_leeway_seconds or 300, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defaults ALREADY exist for create_jwt_validation_middleware don't they?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No - create_jwt_validation_middleware has no defaults (we removed them earlier to have single source of truth). The defaults must be applied here when calling it. TokenValidator.for_service/for_entra have defaults, but create_jwt_validation_middleware doesn't. Commit 9105458.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Well I would like the source of truth to be for_service and for_entra.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! TokenValidator.for_service and TokenValidator.for_entra are now the single source of truth for all defaults.
http_plugin.py no longer applies any defaults - it passes config values directly (or None). create_jwt_validation_middleware only passes non-None values to TokenValidator.for_service, which then applies its own defaults.
Commit 2a78ea4.
- Removed `or` defaults from app.py for TokenValidator.for_entra() (function already has defaults) - Kept explicit defaults in http_plugin.py for create_jwt_validation_middleware (function has no defaults) - Only pass config values to for_entra() if they are not None (let function defaults apply) - Refactored http_plugin.py for better readability and to fix line length linting issue - All 38 tests passing - 0 linting errors Co-authored-by: heyitsaamir <48929123+heyitsaamir@users.noreply.github.com>
- Removed default application from http_plugin.py - Pass config values directly to create_jwt_validation_middleware (or None) - Updated create_jwt_validation_middleware to accept Optional parameters - Only pass non-None values to TokenValidator.for_service - TokenValidator.for_service applies its own defaults - Single source of truth: all defaults in TokenValidator factory methods - All 38 tests passing, 0 linting errors Co-authored-by: heyitsaamir <48929123+heyitsaamir@users.noreply.github.com>
AppConfig Integration - Single Source of Truth for Defaults
Latest Changes
Made
TokenValidator.for_service()
andTokenValidator.for_entra()
the single source of truth for all JWT validation defaults per @heyitsaamir feedback.How Defaults Work Now
Config defines None as default:
HttpPlugin passes config values directly (no defaults applied):
create_jwt_validation_middleware only passes non-None values:
TokenValidator.for_service has the defaults:
Same for TokenValidator.for_entra:
Benefits
TokenValidator
factory methods onlyTest Results
Summary
Config values flow cleanly:
AuthConfig
fields default toNone
HttpPlugin
passes config values (orNone
) tocreate_jwt_validation_middleware
create_jwt_validation_middleware
only passes non-None
values toTokenValidator.for_service
TokenValidator.for_service
applies its defaults for any missing parametersNo defaults duplicated anywhere!
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.