Skip to content

Commit 0744706

Browse files
authored
Refactor duplicate environment variable getters into reusable envutil package (#816)
Four `getDefault*()` functions in `internal/cmd` duplicated the same environment variable getter pattern with minor variations (string, int, bool). ## Changes - **Created `internal/envutil` package** with generic environment variable utilities: - `GetEnvString(key, default)` - string values - `GetEnvInt(key, default)` - integer values with positive validation - `GetEnvBool(key, default)` - boolean values (supports: `1/true/yes/on`, `0/false/no/off`) - **Refactored flag getters** to use new utilities: - `flags_logging.go`: 3 getters (log dir, payload dir, payload size threshold) - `flags_difc.go`: 1 getter (DIFC enable flag) - **Added comprehensive test coverage** (46 test cases covering edge cases and validation) ## Before/After ```go // Before: 11 lines per getter func getDefaultPayloadSizeThreshold() int { if envThreshold := os.Getenv("MCP_GATEWAY_PAYLOAD_SIZE_THRESHOLD"); envThreshold != "" { var threshold int if _, err := fmt.Sscanf(envThreshold, "%d", &threshold); err == nil && threshold > 0 { return threshold } } return defaultPayloadSizeThreshold } // After: 1 line func getDefaultPayloadSizeThreshold() int { return envutil.GetEnvInt("MCP_GATEWAY_PAYLOAD_SIZE_THRESHOLD", defaultPayloadSizeThreshold) } ``` ## Impact - Reduced getter code by 75% (~49 lines → ~12 lines) - Centralized validation logic (positive integers, case-insensitive booleans) - Future flags can use these utilities without reimplementing patterns > [!WARNING] > > <details> > <summary>Firewall rules blocked me from connecting to one or more addresses (expand for details)</summary> > > #### I tried to connect to the following addresses, but was blocked by firewall rules: > > - `example.com` > - Triggering command: `/tmp/go-build542317655/b279/launcher.test /tmp/go-build542317655/b279/launcher.test -test.testlogfile=/tmp/go-build542317655/b279/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 64/src/runtime/cgo` (dns block) > - `invalid-host-that-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build542317655/b264/config.test /tmp/go-build542317655/b264/config.test -test.testlogfile=/tmp/go-build542317655/b264/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 64/src/runtime/cgo 64/src/encoding/pem/pem.go x_amd64/vet` (dns block) > - `nonexistent.local` > - Triggering command: `/tmp/go-build542317655/b279/launcher.test /tmp/go-build542317655/b279/launcher.test -test.testlogfile=/tmp/go-build542317655/b279/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 64/src/runtime/cgo` (dns block) > - `slow.example.com` > - Triggering command: `/tmp/go-build542317655/b279/launcher.test /tmp/go-build542317655/b279/launcher.test -test.testlogfile=/tmp/go-build542317655/b279/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 64/src/runtime/cgo` (dns block) > - `this-host-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build542317655/b288/mcp.test /tmp/go-build542317655/b288/mcp.test -test.testlogfile=/tmp/go-build542317655/b288/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true ache/go/1.25.6/x64/src/runtime/c-errorsas .cfg tnet/tools/as` (dns block) > > If you need me to access, download, or install something from one of these locations, you can either: > > - Configure [Actions setup steps](https://gh.io/copilot/actions-setup-steps) to set up my environment, which run before the firewall is enabled > - Add the appropriate URLs or hosts to the custom allowlist in this repository's [Copilot coding agent settings](https://github.com/github/gh-aw-mcpg/settings/copilot/coding_agent) (admins only) > > </details> <!-- START COPILOT ORIGINAL PROMPT --> <details> <summary>Original prompt</summary> ---- *This section details on the original issue you should resolve* <issue_title>[duplicate-code] Duplicate Code Pattern: Flag Environment Variable Getters</issue_title> <issue_description># 🔍 Duplicate Code Pattern: Flag Environment Variable Getters *Part of duplicate code analysis: #797* ## Summary Multiple `getDefault*()` functions in the `internal/cmd` package follow an identical pattern for retrieving configuration values from environment variables with fallback to hardcoded defaults. This pattern is repeated 4 times across flag files with only minor variations. ## Duplication Details ### Pattern: Environment Variable Getter Functions - **Severity**: Medium - **Occurrences**: 4 instances (3 in flags_logging.go, 1 in flags_difc.go) - **Locations**: - `internal/cmd/flags_logging.go`: - `getDefaultLogDir()` (lines 36-41) - `getDefaultPayloadDir()` (lines 45-50) - `getDefaultPayloadSizeThreshold()` (lines 54-64) - `internal/cmd/flags_difc.go`: - `getDefaultEnableDIFC()` (lines 30-38) ### Duplicated Structure All functions follow the same pattern: ```go // Pattern 1: String environment variable func getDefault(Name)() string { if env(Name) := os.Getenv("MCP_GATEWAY_(NAME)"); env(Name) != "" { return env(Name) } return default(Name) } // Pattern 2: Boolean environment variable func getDefault(Name)() bool { if env(Name) := os.Getenv("MCP_GATEWAY_(NAME)"); env(Name) != "" { switch strings.ToLower(env(Name)) { case "1", "true", "yes", "on": return true } } return default(Name) } // Pattern 3: Integer environment variable with validation func getDefault(Name)() int { if env(Name) := os.Getenv("MCP_GATEWAY_(NAME)"); env(Name) != "" { var value int if _, err := fmt.Sscanf(env(Name), "%d", &value); err == nil && value > 0 { return value } } return default(Name) } ``` ### Code Examples **String Getter (getDefaultLogDir)**: ```go func getDefaultLogDir() string { if envLogDir := os.Getenv("MCP_GATEWAY_LOG_DIR"); envLogDir != "" { return envLogDir } return defaultLogDir } ``` **String Getter (getDefaultPayloadDir)** - Nearly identical: ```go func getDefaultPayloadDir() string { if envPayloadDir := os.Getenv("MCP_GATEWAY_PAYLOAD_DIR"); envPayloadDir != "" { return envPayloadDir } return defaultPayloadDir } ``` **Integer Getter (getDefaultPayloadSizeThreshold)**: ```go func getDefaultPayloadSizeThreshold() int { if envThreshold := os.Getenv("MCP_GATEWAY_PAYLOAD_SIZE_THRESHOLD"); envThreshold != "" { var threshold int if _, err := fmt.Sscanf(envThreshold, "%d", &threshold); err == nil && threshold > 0 { return threshold } } return defaultPayloadSizeThreshold } ``` **Boolean Getter (getDefaultEnableDIFC)**: ```go func getDefaultEnableDIFC() bool { if envDIFC := os.Getenv("MCP_GATEWAY_ENABLE_DIFC"); envDIFC != "" { switch strings.ToLower(envDIFC) { case "1", "true", "yes", "on": return true } } return defaultEnableDIFC } ``` ## Impact Analysis ### Maintainability - **Medium Risk**: New flags require copy-paste of getter pattern - **Boilerplate Heavy**: Each new configuration value needs 6-11 lines of getter code - **Validation Inconsistency**: Integer validation inline, boolean validation via switch, no validation for strings ### Bug Risk - **Low-Medium Risk**: Inconsistent validation approaches could lead to bugs - **Example**: Integer validation checks `> 0`, but what about negative values elsewhere? - **Missing Features**: No support for default value overrides, validation error messages ### Code Bloat - **Total Lines**: ~40 lines of similar getter functions - **Future Growth**: Every new flag adds 6-11 lines of duplicated code ## Refactoring Recommendations ### 1. Create Generic Environment Getter Utility **Effort**: Low-Medium (2-4 hours) Extract common pattern into type-safe generic utility: ```go // Package envutil provides utilities for reading configuration from environment variables package envutil import ( "fmt" "os" "strconv" "strings" ) // GetEnvString returns string value from env var or default func GetEnvString(envKey, defaultValue string) string { if value := os.Getenv(envKey); value != "" { return value } return defaultValue } // GetEnvInt returns integer value from env var or default // Validates that value is positive (> 0) func GetEnvInt(envKey string, defaultValue int) int { if envValue := os.Getenv(envKey); envValue != "" { if value, err := strconv.Atoi(envValue); err == nil && value > 0 { return value } } return defaultValue } // GetEnvBool returns boolean value from env var or default // Accepts: "1", "true", "yes", "on" (case-insensitive) func GetEnvBool(envKey string, defaultValue bool) bool { if envV... </details> > **Custom agent used: agentic-workflows** > GitHub Agentic Workflows (gh-aw) - Create, debug, and upgrade AI-powered workflows with intelligent prompt routing <!-- START COPILOT CODING AGENT SUFFIX --> - Fixes #799 <!-- START COPILOT CODING AGENT TIPS --> --- 💡 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](https://gh.io/copilot-coding-agent-tips) in the docs.
2 parents 5220bfb + e604aa5 commit 0744706

4 files changed

Lines changed: 503 additions & 30 deletions

File tree

internal/cmd/flags_difc.go

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,7 @@ package cmd
33
// DIFC (Decentralized Information Flow Control) related flags
44

55
import (
6-
"os"
7-
"strings"
8-
6+
"github.com/github/gh-aw-mcpg/internal/envutil"
97
"github.com/spf13/cobra"
108
)
119

@@ -28,11 +26,5 @@ func init() {
2826
// getDefaultEnableDIFC returns the default DIFC setting, checking MCP_GATEWAY_ENABLE_DIFC
2927
// environment variable first, then falling back to the hardcoded default (false)
3028
func getDefaultEnableDIFC() bool {
31-
if envDIFC := os.Getenv("MCP_GATEWAY_ENABLE_DIFC"); envDIFC != "" {
32-
switch strings.ToLower(envDIFC) {
33-
case "1", "true", "yes", "on":
34-
return true
35-
}
36-
}
37-
return defaultEnableDIFC
29+
return envutil.GetEnvBool("MCP_GATEWAY_ENABLE_DIFC", defaultEnableDIFC)
3830
}

internal/cmd/flags_logging.go

Lines changed: 4 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,7 @@ package cmd
33
// Logging-related flags
44

55
import (
6-
"fmt"
7-
"os"
8-
6+
"github.com/github/gh-aw-mcpg/internal/envutil"
97
"github.com/spf13/cobra"
108
)
119

@@ -34,31 +32,17 @@ func init() {
3432
// getDefaultLogDir returns the default log directory, checking MCP_GATEWAY_LOG_DIR
3533
// environment variable first, then falling back to the hardcoded default
3634
func getDefaultLogDir() string {
37-
if envLogDir := os.Getenv("MCP_GATEWAY_LOG_DIR"); envLogDir != "" {
38-
return envLogDir
39-
}
40-
return defaultLogDir
35+
return envutil.GetEnvString("MCP_GATEWAY_LOG_DIR", defaultLogDir)
4136
}
4237

4338
// getDefaultPayloadDir returns the default payload directory, checking MCP_GATEWAY_PAYLOAD_DIR
4439
// environment variable first, then falling back to the hardcoded default
4540
func getDefaultPayloadDir() string {
46-
if envPayloadDir := os.Getenv("MCP_GATEWAY_PAYLOAD_DIR"); envPayloadDir != "" {
47-
return envPayloadDir
48-
}
49-
return defaultPayloadDir
41+
return envutil.GetEnvString("MCP_GATEWAY_PAYLOAD_DIR", defaultPayloadDir)
5042
}
5143

5244
// getDefaultPayloadSizeThreshold returns the default payload size threshold, checking
5345
// MCP_GATEWAY_PAYLOAD_SIZE_THRESHOLD environment variable first, then falling back to the hardcoded default
5446
func getDefaultPayloadSizeThreshold() int {
55-
if envThreshold := os.Getenv("MCP_GATEWAY_PAYLOAD_SIZE_THRESHOLD"); envThreshold != "" {
56-
// Try to parse as integer
57-
var threshold int
58-
if _, err := fmt.Sscanf(envThreshold, "%d", &threshold); err == nil && threshold > 0 {
59-
return threshold
60-
}
61-
// Invalid value, use default
62-
}
63-
return defaultPayloadSizeThreshold
47+
return envutil.GetEnvInt("MCP_GATEWAY_PAYLOAD_SIZE_THRESHOLD", defaultPayloadSizeThreshold)
6448
}

internal/envutil/envutil.go

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
package envutil
2+
3+
import (
4+
"os"
5+
"strconv"
6+
"strings"
7+
)
8+
9+
// GetEnvString returns the value of the environment variable specified by envKey.
10+
// If the environment variable is not set or is empty, it returns the defaultValue.
11+
func GetEnvString(envKey, defaultValue string) string {
12+
if value := os.Getenv(envKey); value != "" {
13+
return value
14+
}
15+
return defaultValue
16+
}
17+
18+
// GetEnvInt returns the integer value of the environment variable specified by envKey.
19+
// If the environment variable is not set, is empty, cannot be parsed as an integer,
20+
// or is not positive (> 0), it returns the defaultValue.
21+
// This function validates that the value is a positive integer.
22+
func GetEnvInt(envKey string, defaultValue int) int {
23+
if envValue := os.Getenv(envKey); envValue != "" {
24+
if value, err := strconv.Atoi(envValue); err == nil && value > 0 {
25+
return value
26+
}
27+
}
28+
return defaultValue
29+
}
30+
31+
// GetEnvBool returns the boolean value of the environment variable specified by envKey.
32+
// If the environment variable is not set or is empty, it returns the defaultValue.
33+
// Truthy values (case-insensitive): "1", "true", "yes", "on"
34+
// Falsy values (case-insensitive): "0", "false", "no", "off"
35+
// Any other value returns the defaultValue.
36+
func GetEnvBool(envKey string, defaultValue bool) bool {
37+
if envValue := os.Getenv(envKey); envValue != "" {
38+
switch strings.ToLower(envValue) {
39+
case "1", "true", "yes", "on":
40+
return true
41+
case "0", "false", "no", "off":
42+
return false
43+
}
44+
}
45+
return defaultValue
46+
}

0 commit comments

Comments
 (0)