Conversation
jigar-f
requested review from
WendelHime,
Copilot,
garmr-ulfr and
myleshorton
There was a problem hiding this comment.
Pull request overview
This pull request introduces environment-aware API endpoint management to support both production and staging environments. It centralizes URL definitions in the common package and updates all API calls throughout the codebase to use environment-specific URLs. The Kindling HTTP client is also modified to disable domain fronting in staging mode and add the staging server to proxyless hosts.
Changes:
- Added environment detection (
Stage()) and URL getter functions (GetProServerURL(),GetBaseURL()) in the common package - Replaced hardcoded API URLs across multiple modules with calls to centralized URL getter functions
- Modified Kindling client to handle staging environments differently (disabling domain fronting)
- Added
Set()function to env package for runtime environment variable updates - Removed unused
sing-dnsdependency - Increased max uncompressed log size from 40 MB to 50 MB in issue reporter
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| common/constants.go | Added URL constants for production and staging, with getter functions |
| common/init.go | Added Stage() function for staging environment detection |
| common/env/env.go | Added Set() function for runtime environment variable updates |
| kindling/client.go | Modified to disable domain fronting and use different transports in staging |
| api/api.go | Updated to use centralized URL getters for pro-server and base URLs |
| api/subscription.go | Updated Stripe billing portal URL to use GetProServerURL() |
| api/user.go | Updated OAuth and streaming URLs to use GetBaseURL() |
| config/fetcher.go | Updated config endpoint URL to use GetBaseURL() |
| issue/issue.go | Updated issue reporting URL to use GetBaseURL(); increased max log size |
| go.mod, go.sum | Removed unused sing-dns dependency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Stage returns true if the application is running in staging environment. | ||
| func Stage() bool { | ||
| e, _ := env.Get[string](env.ENV) |
There was a problem hiding this comment.
The Stage() function is missing the strings.ToLower() call that is used in Dev() and Prod() functions. This inconsistency could cause the function to fail if the environment variable is set with different casing (e.g., "Stage", "STAGE", "Staging", "STAGING"). For consistency and robustness, the environment variable value should be converted to lowercase before comparison, just like in the other environment detection functions.
| e, _ := env.Get[string](env.ENV) | |
| e, _ := env.Get[string](env.ENV) | |
| e = strings.ToLower(e) |
| if common.Stage() { | ||
| //Disable domain fronting for stage environment to avoid hitting staging server issues due to fronted client failures. | ||
| return kindling.NewKindling("radiance", | ||
| kindling.WithPanicListener(reporting.PanicListener), | ||
| kindling.WithLogWriter(logger), | ||
| // Most endpoints use df.iantem.io, but for some historical reasons | ||
| // "pro-server" calls still go to api.getiantem.org. | ||
| kindling.WithProxyless("df.iantem.io", "api.getiantem.org", "api.staging.iantem.io"), | ||
| ) |
There was a problem hiding this comment.
In the staging environment configuration, the AMPCache and DNSTunnel transports are not configured, but they are included in the production configuration. This means staging will lack these fallback transport mechanisms. If this is intentional to simplify staging testing, it should be documented in a comment. If it's unintentional, these transports should be added to the staging configuration as well.
| } | ||
|
|
||
| if common.Stage() { | ||
| //Disable domain fronting for stage environment to avoid hitting staging server issues due to fronted client failures. |
There was a problem hiding this comment.
The comment is missing a space after the double slash. According to Go conventions, there should be a space between "//" and the comment text.
| //Disable domain fronting for stage environment to avoid hitting staging server issues due to fronted client failures. | |
| // Disable domain fronting for stage environment to avoid hitting staging server issues due to fronted client failures. |
| // "pro-server" calls still go to api.getiantem.org. | ||
| kindling.WithProxyless("df.iantem.io", "api.getiantem.org"), | ||
| kindling.WithDomainFronting(f), | ||
| kindling.WithProxyless("df.iantem.io", "api.getiantem.org", "api.staging.iantem.io"), |
There was a problem hiding this comment.
Do we want to also include the staging URL in production?
| // Set updates the env variable in the in-memory map so that subsequent Get calls | ||
| // return the new value. This is needed because init() only runs once at startup, | ||
| // so os.Setenv alone won't be picked up by Get. | ||
| func Set(key Key, value string) { | ||
| parseAndSet(key, value) | ||
| // Also set in the OS environment for consistency | ||
| os.Setenv(key, value) | ||
| } | ||
|
|
There was a problem hiding this comment.
Why do we need this?
This is needed because init() only runs once at startup
This is intentional. The env variables control how some things are configured and so shouldn't be modified after starting. This can also be misleading and adds the potential for bugs in the future as someone will assume that they can change any variable during runtime and have it take affect.
There was a problem hiding this comment.
OK, I see, you can't set environment variables on Android and iOS. Instead of introducing a global Set function just to set the environment to "staging", we could just create a file env_mobile.go with the following:
//go:build android || ios
package env
import "os"
func SetStagingEnv() {
if _, exists := envVars[ENV]; exists {
return
}
envVars[ENV] = "staging"
os.Setenv(string(ENV), "staging")
}This will prevent any value from being modified during runtime, explicitly sets it to "staging", and restricts it to mobile.
2 participants
This pull request refactors how API endpoint URLs are managed throughout the codebase, introducing environment-aware URL selection to support both production and staging environments. The changes centralize URL definitions and logic in the
commonpackage, replacing hardcoded URLs in multiple modules. Additionally, the Kindling client is updated to handle staging environments more gracefully, and a utility function for setting environment variables in-memory is added.Environment-aware URL management:
ProServerURL,StageProServerURL,BaseURL, andStageBaseURLconstants and correspondingGetProServerURL()andGetBaseURL()functions tocommon/constants.gofor dynamic selection of API endpoints based on environment. Also added aStage()function incommon/init.goto detect staging mode. [1] [2]api/api.go,api/subscription.go,api/user.go,config/fetcher.go, andissue/issue.gowith calls to the newcommon.GetProServerURL()andcommon.GetBaseURL()functions, ensuring correct URLs are used for production or staging. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13]Kindling client improvements:
kindling/client.goso that when running in staging, domain fronting is disabled and the staging API endpoint is added to proxyless hosts, improving reliability against staging server issues.Environment variable utilities:
Setfunction tocommon/env/env.goto allow in-memory and OS-level updates of environment variables, making it easier to change environment settings at runtime for testing or configuration purposes.These changes make the codebase more flexible and robust when switching between production and staging environments, and reduce the risk of errors from hardcoded URLs.