Feature/config updates#127
Merged
Merged
Conversation
- Updated AdminActionTopics to utilize IConfiguration for dynamic forum user ID and forum ID resolution. - Refactored BannersController and ExternalController to use IConfiguration for base URLs. - Enhanced HealthCheckController to validate community URL against configuration. - Modified IdentityHostingStartup to derive OAuth endpoints from configuration. - Adjusted AdminActionsController and ServerAdminController to use configuration for forum base URL and admin ID. - Updated GameTypeIconTagHelper and ServerLinkTagHelper to utilize IConfiguration for dynamic URLs. - Refactored Program.cs to streamline Azure App Configuration integration. - Updated ProxyCheckService to derive API base URL from configuration. - Adjusted Startup.cs to read Application Insights sampling percentages from configuration. - Refactored various Razor views to utilize IConfiguration for dynamic URLs and API keys. - Removed geo_location_api configuration from tfvars files and variables.tf.
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
frasermolyneux
temporarily deployed
to
Development
GitHub Actions
Inactive
Contributor
⛔ Superseded — A newer run has replaced this result.⛔ Superseded — A newer run has replaced this result.🏗️ Terraform Plan
✅ Validate — Passed ✅ Plan
📋 Resource Details
|
There was a problem hiding this comment.
Pull request overview
This pull request centralizes configuration management by moving hardcoded URLs and default values into configuration settings loaded from Azure App Configuration. The changes affect multiple layers including Terraform infrastructure, controllers, views, tag helpers, and integration services, establishing a hierarchical configuration namespace pattern (e.g., XtremeIdiots:Forums:*, GameTracker:*, Google:*).
Changes:
- Removed local appsettings.json entries and Terraform variables for
AdminActionsandGeoLocationApi, centralizing all configuration in Azure App Configuration - Refactored Program.cs to simplify Azure App Configuration loading with explicit namespace selections
- Updated controllers, views, and services to read configuration values with fallbacks instead of using hardcoded constants
- Standardized URL construction patterns using
TrimEnd('/')for base URLs across the codebase - Made Application Insights sampling percentages and external API endpoints configurable
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| terraform/web_app.tf | Removed GeoLocationApi environment variables previously set via Terraform |
| terraform/variables.tf | Removed geo_location_api variable declaration |
| terraform/tfvars/prd.tfvars | Removed production GeoLocationApi configuration values |
| terraform/tfvars/dev.tfvars | Removed development GeoLocationApi configuration values |
| src/XtremeIdiots.Portal.Web/appsettings.json | Removed AdminActions section (now in Azure App Configuration) |
| src/XtremeIdiots.Portal.Web/Program.cs | Simplified Azure App Configuration loading, added explicit namespace selections |
| src/XtremeIdiots.Portal.Web/Startup.cs | Made Application Insights sampling percentages configurable |
| src/XtremeIdiots.Portal.Web/Controllers/AdminActionsController.cs | Replaced hardcoded constants with configuration-based readonly fields |
| src/XtremeIdiots.Portal.Web/Controllers/ServerAdminController.cs | Replaced hardcoded constants with configuration-based readonly fields |
| src/XtremeIdiots.Portal.Web/Services/ProxyCheckService.cs | Made ProxyCheck base URL configurable |
| src/XtremeIdiots.Portal.Web/ApiControllers/HealthCheckController.cs | Made forums community URL validation configurable |
| src/XtremeIdiots.Portal.Web/ApiControllers/ExternalController.cs | Made portal base URL configurable for external API responses |
| src/XtremeIdiots.Portal.Web/ApiControllers/BannersController.cs | Made GameTracker banner base URL configurable |
| src/XtremeIdiots.Portal.Web/Helpers/ServerTagHelpers.cs | Made GameTracker server info URL configurable |
| src/XtremeIdiots.Portal.Web/Helpers/GameTypeIconTagHelper.cs | Made portal base URL configurable for external game icons |
| src/XtremeIdiots.Portal.Web/Areas/Identity/IdentityHostingStartup.cs | Made OAuth endpoints configurable via forums base URL |
| src/XtremeIdiots.Portal.Integrations.Forums/AdminActionTopics.cs | Added configuration-driven forum ID resolution with fallback logic |
| src/XtremeIdiots.Portal.Web/Views/Shared/_Layout.cshtml | Made Google Analytics ID configurable with conditional rendering |
| src/XtremeIdiots.Portal.Web/Views/Shared/Components/AdminActions/Default.cshtml | Made forum topic base URL configurable |
| src/XtremeIdiots.Portal.Web/Views/AdminActions/_MyAdminActionDetailsPanel.cshtml | Made forum base URL configurable |
| src/XtremeIdiots.Portal.Web/Views/External/LatestAdminActions.cshtml | Made portal base URL configurable |
| src/XtremeIdiots.Portal.Web/Views/Servers/ServerInfo.cshtml | Made Google Maps API key configurable with conditional rendering |
| src/XtremeIdiots.Portal.Web/Views/Servers/Map.cshtml | Made Google Maps API key configurable with conditional rendering |
| src/XtremeIdiots.Portal.Web/Views/Players/Details.cshtml | Made Google Maps API key configurable with conditional rendering |
| src/XtremeIdiots.Portal.Web/Views/IPAddresses/Details.cshtml | Made Google Maps API key configurable with conditional rendering |
Comments suppressed due to low confidence (1)
terraform/variables.tf:70
- The GeoLocationApi configuration variables are being removed from Terraform (variables.tf, web_app.tf, and tfvars files), but the application code still requires these settings. The Program.cs file at line 37 selects "GeoLocationApi:*" from Azure App Configuration, and Startup.cs at lines 66-69 requires GeoLocationApi:BaseUrl, GeoLocationApi:ApiKey, and GeoLocationApi:ApplicationAudience to configure the GeoLocationApiClient service.
If these settings are now being sourced exclusively from Azure App Configuration instead of Terraform environment variables, this is acceptable. However, the PR description doesn't mention this change, and there's no documentation update explaining where these configuration values should now be maintained.
Please clarify: Are the GeoLocationApi settings now being managed entirely in Azure App Configuration? If so, consider updating documentation or adding a comment in the PR to explain this migration pattern.
variable "dns_subscription_id" {}
variable "dns_resource_group_name" {}
variable "dns_subdomain" {}
variable "dns_zone_name" {}
variable "tags" {
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
frasermolyneux
temporarily deployed
to
Development
GitHub Actions
Inactive
Contributor
⛔ Superseded — A newer run has replaced this result.🏗️ Terraform Plan
✅ Validate — Passed ✅ Plan
📋 Resource Details
|
frasermolyneux
temporarily deployed
to
Development
GitHub Actions
Inactive
Contributor
🏗️ Terraform Plan
✅ Validate — Passed ✅ Plan
📋 Resource Details
|
|
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.
This pull request centralizes and standardizes the use of configuration values for URLs and default settings across the codebase. Hardcoded URLs and constants are replaced with values sourced from configuration, improving maintainability and flexibility for different environments. The changes impact controllers, tag helpers, and integration services, ensuring consistent URL construction and default behaviors throughout the application.
Configuration-driven URL and Default Value Management:
Replaced hardcoded URLs and constants (such as forum base URLs, admin user IDs, and temp ban durations) in
AdminActionsController,ServerAdminController, and related helpers with values read fromIConfiguration. This allows these settings to be easily changed without code modifications. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10]Updated
AdminActionTopicsto resolve forum and user IDs via configuration with fallback logic, and to dynamically construct portal links using the configured base URL. Added aResolveForumIdhelper to select forum IDs from configuration or fall back to extension methods. [1] [2] [3] [4]External API and Authentication Consistency:
GameTracker Integration Improvements:
Health Check Adjustments:
These changes make the application more robust, easier to configure, and better suited for deployment in multiple environments.