diff --git a/docs/adr/44097-extend-envutil-for-typed-env-access.md b/docs/adr/44097-extend-envutil-for-typed-env-access.md new file mode 100644 index 00000000000..a4dfdb541da --- /dev/null +++ b/docs/adr/44097-extend-envutil-for-typed-env-access.md @@ -0,0 +1,47 @@ +# ADR-44097: Extend envutil with Typed Helpers for Boolean and String Environment Variables + +**Date**: 2026-07-08 +**Status**: Draft +**Deciders**: Unknown + +--- + +### Context + +The codebase has a convention of reading environment variables through `pkg/envutil.GetIntFromEnv`, which centralizes defaulting, bounds-validation, and debug logging for integer-typed env vars. However, boolean and string env vars in `pkg/cli` and `pkg/parser` were still read via direct `os.Getenv` calls, suppressed with `//nolint:osgetenvlibrary` annotations. This produced inconsistent behavior: CI detection was replicated inline across three call sites in `pkg/cli` (`os.Getenv("CI") != ""`), `GO_TEST_MODE` was compared as a raw string rather than using Go's canonical boolean parsing, and `CODESPACES` used `strings.EqualFold` on a raw string rather than a shared accessor. Sensitive tokens (`GITHUB_TOKEN`, `GH_TOKEN`) were read directly without debug-safe logging. The growing number of exemptions signalled that the existing helper surface was incomplete for the types of env vars the codebase actually uses. + +### Decision + +We will add `GetBoolFromEnv` and `GetStringFromEnv` helpers to `pkg/envutil`, consistent in signature and behavior with the existing `GetIntFromEnv`, and migrate all direct `os.Getenv` calls in `pkg/cli` and `pkg/parser` to use these helpers or the existing `IsRunningInCI()` / `isRunningInCodespace()` wrapper functions that now delegate to them. The shared internal `warn` function extracted in this PR eliminates duplicated warning routing code across all three typed helpers. + +### Alternatives Considered + +#### Alternative 1: Retain Direct `os.Getenv` with Nolint Suppressions + +Continue using raw `os.Getenv` at each call site with `//nolint:osgetenvlibrary` to satisfy the linter. Each call site would keep its own string-comparison, defaulting, and logging logic. This avoids introducing a new abstraction, but perpetuates inconsistency: `GO_TEST_MODE == "true"` rejects `"1"`, `"TRUE"`, and `"yes"` as truthy, while the rest of the codebase would gain consistent boolean parsing via `strconv.ParseBool`. It also makes debug-safe logging of sensitive env values (never log the value itself) opt-in per call site rather than the default. + +#### Alternative 2: Dependency-Injection of an Environment Reader Interface + +Introduce an `EnvReader` interface and inject it into all consumers, allowing env access to be mocked in tests without `os.Setenv`. This would improve test isolation for packages that currently must manipulate real process environment state. However, it is a significantly larger change, requires threading the interface through many function signatures, and conflicts with the existing `envutil` "static helper" design that callers already adopt. The present PR's test coverage uses `os.Setenv`/`os.Unsetenv` with deferred cleanup, which is adequate for the current test surface. + +### Consequences + +#### Positive +- `GetBoolFromEnv` accepts all spellings recognised by `strconv.ParseBool` (`"1"`, `"t"`, `"TRUE"`, etc.), making boolean env var handling consistent with Go conventions across the codebase. +- `GetStringFromEnv` logs `Using ENV_VAR from environment` rather than the value itself, making it safe to use with secret-like variables (`GITHUB_TOKEN`, `GH_TOKEN`) without risk of echoing credentials in debug output. +- Removing `//nolint:osgetenvlibrary` suppressions from `pkg/cli` and `pkg/parser` means the linter now enforces the centralized pattern without human-reviewed exceptions. +- The shared `warn` helper in `envutil.go` eliminates a duplicated closure that was previously inlined inside `GetIntFromEnv`, reducing surface area for divergence. + +#### Negative +- `GetStringFromEnv` treats an empty string value identically to an unset variable and returns `defaultValue` in both cases. Callers that need to distinguish between `VAR=""` (explicitly blank) and `VAR` unset cannot use this helper and must call `os.Getenv` directly. +- `GetStringFromEnv` does not emit a warning when the env var is absent (unlike `GetIntFromEnv` and `GetBoolFromEnv` for invalid values), so there is no diagnostic signal when an expected variable is missing. +- Adding two new exported functions to `pkg/envutil` slightly grows the stable API surface that future refactors must maintain. + +#### Neutral +- The `warn` helper is package-private; callers outside `pkg/envutil` cannot reuse it for their own typed-parsing scenarios. +- Boolean env vars that previously used non-canonical comparisons (e.g., `os.Getenv("CI") != ""`) now use `strconv.ParseBool`, which does *not* treat a non-empty non-boolean string as truthy — a subtle behavioral change that is intentional and documented in the PR. +- All three new and existing helpers share the same `debugLog *logger.Logger` optional parameter convention; passing `nil` suppresses structured logging and falls back to `os.Stderr` warnings. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* diff --git a/pkg/cli/add_interactive_orchestrator.go b/pkg/cli/add_interactive_orchestrator.go index 8e22194c7ab..7becae55270 100644 --- a/pkg/cli/add_interactive_orchestrator.go +++ b/pkg/cli/add_interactive_orchestrator.go @@ -10,6 +10,7 @@ import ( "charm.land/huh/v2" "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/constants" + "github.com/github/gh-aw/pkg/envutil" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/workflow" ) @@ -70,8 +71,11 @@ type AddInteractiveConfig struct { func RunAddInteractive(ctx context.Context, config *AddInteractiveConfig) error { addInteractiveLog.Print("Starting interactive add workflow") - // Assert this function is not running in automated unit tests or CI - if os.Getenv("GO_TEST_MODE") == "true" || os.Getenv("CI") != "" { //nolint:osgetenvlibrary + // Assert this function is not running in automated unit tests or CI. + // GO_TEST_MODE intentionally uses GetBoolFromEnv so common boolean spellings + // are treated consistently across test and automation environments, while + // IsRunningInCI centralizes the broader CI environment detection logic. + if envutil.GetBoolFromEnv("GO_TEST_MODE", false, addInteractiveLog) || IsRunningInCI() { return errors.New("interactive add cannot be used in automated tests or CI environments") } diff --git a/pkg/cli/add_interactive_workflow.go b/pkg/cli/add_interactive_workflow.go index 3d21d45448a..cb011f1dc2a 100644 --- a/pkg/cli/add_interactive_workflow.go +++ b/pkg/cli/add_interactive_workflow.go @@ -87,7 +87,7 @@ func (c *AddInteractiveConfig) checkStatusAndOfferRun(ctx context.Context) error } // In Codespaces, don't offer to trigger - provide link to Actions page instead - if os.Getenv("CODESPACES") == "true" { //nolint:osgetenvlibrary + if isRunningInCodespace() { addInteractiveLog.Print("Running in Codespaces, skipping run offer and showing Actions link") fmt.Fprintln(os.Stderr, "") fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Running in GitHub Codespaces - please trigger the workflow manually from the Actions page")) diff --git a/pkg/cli/add_wizard_command.go b/pkg/cli/add_wizard_command.go index 07fc827cce6..fe5441dcf5c 100644 --- a/pkg/cli/add_wizard_command.go +++ b/pkg/cli/add_wizard_command.go @@ -2,7 +2,6 @@ package cli import ( "errors" - "os" "github.com/github/gh-aw/pkg/constants" "github.com/github/gh-aw/pkg/logger" @@ -84,7 +83,7 @@ Note: To create a new workflow from scratch, use the 'new' command instead.`, // add-wizard requires an interactive terminal isTerminal := tty.IsStdoutTerminal() - isCIEnv := os.Getenv("CI") != "" //nolint:osgetenvlibrary + isCIEnv := IsRunningInCI() addWizardLog.Printf("Terminal check: is_terminal=%v, is_ci=%v", isTerminal, isCIEnv) if !isTerminal || isCIEnv { return errors.New("add-wizard requires an interactive terminal; use 'add' for non-interactive environments") diff --git a/pkg/cli/codespace.go b/pkg/cli/codespace.go index 5794a49121a..718ab38e2c1 100644 --- a/pkg/cli/codespace.go +++ b/pkg/cli/codespace.go @@ -1,10 +1,10 @@ package cli import ( - "os" "strings" "github.com/github/gh-aw/pkg/console" + "github.com/github/gh-aw/pkg/envutil" "github.com/github/gh-aw/pkg/logger" ) @@ -14,7 +14,7 @@ var codespaceLog = logger.New("cli:codespace") // by checking for the CODESPACES environment variable func isRunningInCodespace() bool { // GitHub Codespaces sets CODESPACES=true environment variable - isCodespace := strings.EqualFold(os.Getenv("CODESPACES"), "true") //nolint:osgetenvlibrary + isCodespace := strings.EqualFold(envutil.GetStringFromEnv("CODESPACES", "", nil), "true") codespaceLog.Printf("Codespace detection: is_codespace=%v", isCodespace) return isCodespace } diff --git a/pkg/cli/interactive.go b/pkg/cli/interactive.go index c4b967a2b76..ed0c8a38d9d 100644 --- a/pkg/cli/interactive.go +++ b/pkg/cli/interactive.go @@ -15,6 +15,7 @@ import ( "charm.land/huh/v2" "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/constants" + "github.com/github/gh-aw/pkg/envutil" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/setutil" "github.com/github/gh-aw/pkg/sliceutil" @@ -63,8 +64,11 @@ func (b *InteractiveWorkflowBuilder) ensureNonTTYScanner(r io.Reader) *bufio.Sca func CreateWorkflowInteractively(ctx context.Context, workflowName string, verbose bool, force bool) error { interactiveLog.Printf("Starting interactive workflow creation: workflowName=%s, force=%v", workflowName, force) - // Assert this function is not running in automated unit tests - if os.Getenv("GO_TEST_MODE") == "true" || os.Getenv("CI") != "" { //nolint:osgetenvlibrary + // Assert this function is not running in automated unit tests or CI. + // GO_TEST_MODE intentionally uses GetBoolFromEnv so common boolean spellings + // are treated consistently across test and automation environments, while + // IsRunningInCI centralizes the broader CI environment detection logic. + if envutil.GetBoolFromEnv("GO_TEST_MODE", false, interactiveLog) || IsRunningInCI() { return errors.New("interactive workflow creation cannot be used in automated tests or CI environments") } diff --git a/pkg/envutil/README.md b/pkg/envutil/README.md index c5b88d20027..226097d9423 100644 --- a/pkg/envutil/README.md +++ b/pkg/envutil/README.md @@ -1,12 +1,12 @@ # envutil Package -> Reads and validates integer-valued environment variables with bounds checking. +> Reads and validates environment variables with consistent default handling. ## Overview -The `envutil` package centralizes the pattern of reading integer-valued environment variables, validating them against configured minimum and maximum bounds, and falling back to a default value when the variable is absent or out of range. It emits warning messages to stderr when an invalid value is encountered, following the console formatting conventions of the rest of the codebase. +The `envutil` package centralizes the pattern of reading environment variables, validating typed values where needed, and falling back to a default value when the variable is absent or invalid. It emits warning messages to stderr when an invalid typed value is encountered, following the console formatting conventions of the rest of the codebase. -The package exposes a single generic helper, `GetIntFromEnv`, which encapsulates the repetitive read-parse-validate-default logic required wherever configurable integer parameters are exposed via environment variables. +The package exposes helpers for integer, boolean, and string values so callers can avoid scattered `os.Getenv` checks while preserving consistent defaulting and debug logging behavior. ## Public API @@ -15,6 +15,8 @@ The package exposes a single generic helper, `GetIntFromEnv`, which encapsulates | Function | Signature | Description | |----------|-----------|-------------| | `GetIntFromEnv` | `func(envVar string, defaultValue, minValue, maxValue int, debugLog *logger.Logger) int` | Reads an integer-valued environment variable, validates it, and returns a default when absent or invalid | +| `GetBoolFromEnv` | `func(envVar string, defaultValue bool, debugLog *logger.Logger) bool` | Reads a boolean-valued environment variable and returns a default when absent or invalid | +| `GetStringFromEnv` | `func(envVar, defaultValue string, debugLog *logger.Logger) string` | Reads a string-valued environment variable and returns a default when absent | #### `GetIntFromEnv` @@ -40,6 +42,46 @@ Reads `envVar` from the process environment, parses it as an integer, validates - SHOULD emit warnings formatted via `console.FormatWarningMessage` to `os.Stderr` when `debugLog` is `nil`. - MAY route warnings through `debugLog.Printf` when `debugLog` is non-nil instead of writing directly to stderr. +#### `GetBoolFromEnv` + +```go +func GetBoolFromEnv(envVar string, defaultValue bool, debugLog *logger.Logger) bool +``` + +Reads `envVar` from the process environment, parses it as a boolean using Go's standard `strconv.ParseBool` rules, and returns `defaultValue` when the variable is absent or unparseable. + +| Parameter | Type | Description | +|-----------|------|-------------| +| `envVar` | `string` | Environment variable name (e.g. `"CI"`) | +| `defaultValue` | `bool` | Value returned when env var is absent or invalid | +| `debugLog` | `*logger.Logger` | Optional logger for debug output; pass `nil` to disable | + +**Behavioral contract**: +- MUST return `defaultValue` when the environment variable is not set (empty string). +- MUST return `defaultValue` and emit a warning when the value cannot be parsed as a boolean. +- MUST log the accepted value via `debugLog` when `debugLog` is non-nil. +- SHOULD emit warnings formatted via `console.FormatWarningMessage` to `os.Stderr` when `debugLog` is `nil`. +- MAY route warnings through `debugLog.Printf` when `debugLog` is non-nil instead of writing directly to stderr. + +#### `GetStringFromEnv` + +```go +func GetStringFromEnv(envVar, defaultValue string, debugLog *logger.Logger) string +``` + +Reads `envVar` from the process environment and returns `defaultValue` when the variable is absent or empty. + +| Parameter | Type | Description | +|-----------|------|-------------| +| `envVar` | `string` | Environment variable name (e.g. `"GITHUB_TOKEN"`) | +| `defaultValue` | `string` | Value returned when env var is absent or empty | +| `debugLog` | `*logger.Logger` | Optional logger for debug output; pass `nil` to disable | + +**Behavioral contract**: +- MUST return `defaultValue` when the environment variable is not set (empty string). +- MUST return the environment variable value unchanged when it is non-empty. +- SHOULD log only that the variable was found, without logging its value, via `debugLog` when `debugLog` is non-nil. + ## Usage Examples ```go @@ -53,19 +95,26 @@ var log = logger.New("mypackage:config") // Read GH_AW_MAX_CONCURRENT_DOWNLOADS, constrained to [1, 20], default 5 concurrency := envutil.GetIntFromEnv("GH_AW_MAX_CONCURRENT_DOWNLOADS", 5, 1, 20, log) +// Read CI, defaulting to false when unset or invalid +isCI := envutil.GetBoolFromEnv("CI", false, log) + +// Read GITHUB_TOKEN, defaulting to empty string when unset +token := envutil.GetStringFromEnv("GITHUB_TOKEN", "", log) + // Suppress debug output by passing nil logger timeout := envutil.GetIntFromEnv("GH_AW_TIMEOUT", 60, 1, 3600, nil) ``` ## Thread Safety -`GetIntFromEnv` is safe for concurrent use. It holds no shared mutable state; each invocation reads the process environment via `os.Getenv` and operates on function-local variables only. +`GetIntFromEnv`, `GetBoolFromEnv`, and `GetStringFromEnv` are safe for concurrent use. They hold no shared mutable state; each invocation reads the process environment via `os.Getenv` and operates on function-local variables only. ## Design Decisions - Warning messages use `console.FormatWarningMessage` so they render consistently in terminals. - All warnings go to `os.Stderr` to avoid polluting structured stdout output. -- The function handles integers only; floating-point or string env vars should be read directly via `os.Getenv`. +- Typed helpers use Go standard library parsing rules (`strconv.Atoi`, `strconv.ParseBool`) for predictable behavior. +- `GetStringFromEnv` logs a message of the form `Using ENV_VAR from environment` when a value is found, never the value itself, so secret-like values can be read without echoing their contents. - When `debugLog` is non-nil, warnings are routed through the logger rather than written directly to stderr, allowing callers to control output formatting. ## Dependencies diff --git a/pkg/envutil/envutil.go b/pkg/envutil/envutil.go index 50237c1bfcd..1b3a07ce573 100644 --- a/pkg/envutil/envutil.go +++ b/pkg/envutil/envutil.go @@ -10,6 +10,17 @@ import ( "github.com/github/gh-aw/pkg/logger" ) +// warn is an internal helper shared by GetIntFromEnv and GetBoolFromEnv. It +// routes environment parsing warnings through the provided logger when +// available, or to stderr using the standard console warning format otherwise. +func warn(debugLog *logger.Logger, msg string) { + if debugLog != nil { + debugLog.Printf("WARNING: %s", msg) + return + } + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(msg)) +} + // GetIntFromEnv is a generic helper that reads an integer value from an environment variable, // validates it against min/max bounds, and returns a default value if invalid. // This follows the configuration helper pattern from pkg/workflow/config_helpers.go. @@ -19,7 +30,7 @@ import ( // - defaultValue: The default value to return if env var is not set or invalid // - minValue: Minimum allowed value (inclusive) // - maxValue: Maximum allowed value (inclusive) -// - log: Optional logger for debug output +// - debugLog: Optional logger for debug output // // Returns the parsed integer value, or defaultValue if: // - Environment variable is not set @@ -28,14 +39,6 @@ import ( // // Invalid values trigger warning messages to stderr, or through the logger if provided. func GetIntFromEnv(envVar string, defaultValue, minValue, maxValue int, debugLog *logger.Logger) int { - warn := func(msg string) { - if debugLog != nil { - debugLog.Printf("WARNING: %s", msg) - } else { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(msg)) - } - } - envValue := os.Getenv(envVar) //nolint:osgetenvlibrary if envValue == "" { return defaultValue @@ -43,12 +46,12 @@ func GetIntFromEnv(envVar string, defaultValue, minValue, maxValue int, debugLog val, err := strconv.Atoi(envValue) if err != nil { - warn(fmt.Sprintf("Invalid %s value '%s' (must be a number), using default %d", envVar, envValue, defaultValue)) + warn(debugLog, fmt.Sprintf("Invalid %s value '%s' (must be a number), using default %d", envVar, envValue, defaultValue)) return defaultValue } if val < minValue || val > maxValue { - warn(fmt.Sprintf("%s value %d is out of bounds (must be %d-%d), using default %d", envVar, val, minValue, maxValue, defaultValue)) + warn(debugLog, fmt.Sprintf("%s value %d is out of bounds (must be %d-%d), using default %d", envVar, val, minValue, maxValue, defaultValue)) return defaultValue } @@ -57,3 +60,56 @@ func GetIntFromEnv(envVar string, defaultValue, minValue, maxValue int, debugLog } return val } + +// GetBoolFromEnv reads a boolean value from an environment variable and returns +// defaultValue if the variable is not set or cannot be parsed as a boolean. +// +// Parameters: +// - envVar: The environment variable name (e.g., "CI") +// - defaultValue: The default value to return if env var is not set or invalid +// - debugLog: Optional logger for debug output +// +// Returns the parsed boolean value, or defaultValue if: +// - Environment variable is not set +// - Value cannot be parsed as a boolean +// +// Invalid values trigger warning messages to stderr, or through the logger if provided. +func GetBoolFromEnv(envVar string, defaultValue bool, debugLog *logger.Logger) bool { + envValue := os.Getenv(envVar) //nolint:osgetenvlibrary + if envValue == "" { + return defaultValue + } + + val, err := strconv.ParseBool(envValue) + if err != nil { + warn(debugLog, fmt.Sprintf("Invalid %s value '%s' (must be a boolean), using default %t", envVar, envValue, defaultValue)) + return defaultValue + } + + if debugLog != nil { + debugLog.Printf("Using %s=%t", envVar, val) + } + return val +} + +// GetStringFromEnv reads a string value from an environment variable and +// returns defaultValue if the variable is not set. +// +// Parameters: +// - envVar: The environment variable name (e.g., "GITHUB_TOKEN") +// - defaultValue: The default value to return if env var is not set +// - debugLog: Optional logger for debug output +// +// Returns the environment variable value, or defaultValue when the variable is +// not set. Empty string values are treated the same as unset variables. +func GetStringFromEnv(envVar, defaultValue string, debugLog *logger.Logger) string { + envValue := os.Getenv(envVar) //nolint:osgetenvlibrary + if envValue == "" { + return defaultValue + } + + if debugLog != nil { + debugLog.Printf("Using %s from environment", envVar) + } + return envValue +} diff --git a/pkg/envutil/envutil_test.go b/pkg/envutil/envutil_test.go index bc16e727097..bff708c4858 100644 --- a/pkg/envutil/envutil_test.go +++ b/pkg/envutil/envutil_test.go @@ -377,6 +377,128 @@ func TestGetIntFromEnv_EmptyString(t *testing.T) { } } +func TestGetBoolFromEnv(t *testing.T) { + const testEnvVar = "GH_AW_TEST_BOOL_VALUE" + originalValue, hadOriginalValue := os.LookupEnv(testEnvVar) + defer func() { + if hadOriginalValue { + os.Setenv(testEnvVar, originalValue) + } else { + os.Unsetenv(testEnvVar) + } + }() + + tests := []struct { + name string + envValue string + defaultValue bool + expected bool + }{ + { + name: "default when env var not set", + envValue: "", + defaultValue: true, + expected: true, + }, + { + name: "valid true value", + envValue: "true", + defaultValue: false, + expected: true, + }, + { + name: "valid false value", + envValue: "false", + defaultValue: true, + expected: false, + }, + { + name: "numeric true value", + envValue: "1", + defaultValue: false, + expected: true, + }, + { + name: "invalid value", + envValue: "invalid", + defaultValue: true, + expected: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.envValue != "" { + os.Setenv(testEnvVar, tt.envValue) + } else { + os.Unsetenv(testEnvVar) + } + + result := GetBoolFromEnv(testEnvVar, tt.defaultValue, nil) + if result != tt.expected { + t.Errorf("Expected %v, got %v", tt.expected, result) + } + }) + } +} + +func TestGetStringFromEnv(t *testing.T) { + const testEnvVar = "GH_AW_TEST_STRING_VALUE" + originalValue, hadOriginalValue := os.LookupEnv(testEnvVar) + defer func() { + if hadOriginalValue { + os.Setenv(testEnvVar, originalValue) + } else { + os.Unsetenv(testEnvVar) + } + }() + + tests := []struct { + name string + setEnv bool + envValue string + defaultValue string + expected string + }{ + { + name: "default when env var not set", + setEnv: false, + envValue: "", + defaultValue: "fallback", + expected: "fallback", + }, + { + name: "default when env var empty", + setEnv: true, + envValue: "", + defaultValue: "fallback", + expected: "fallback", + }, + { + name: "returns env value", + setEnv: true, + envValue: "value", + defaultValue: "fallback", + expected: "value", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.setEnv { + os.Setenv(testEnvVar, tt.envValue) + } else { + os.Unsetenv(testEnvVar) + } + + result := GetStringFromEnv(testEnvVar, tt.defaultValue, nil) + if result != tt.expected { + t.Errorf("Expected %q, got %q", tt.expected, result) + } + }) + } +} + // Benchmark tests func BenchmarkGetIntFromEnv_ValidValue(b *testing.B) { diff --git a/pkg/envutil/spec_test.go b/pkg/envutil/spec_test.go index 3b4a31ad378..c9a41ca40ed 100644 --- a/pkg/envutil/spec_test.go +++ b/pkg/envutil/spec_test.go @@ -169,3 +169,91 @@ func TestSpec_PublicAPI_GetIntFromEnv_DocExample(t *testing.T) { assert.Equal(t, 8, result, "documented example: returns valid value within [1, 20]") } + +// TestSpec_PublicAPI_GetBoolFromEnv validates the documented behavior of +// GetBoolFromEnv as described in the package README.md. +func TestSpec_PublicAPI_GetBoolFromEnv_ReturnsDefault_WhenNotSet(t *testing.T) { + const envVar = "GH_AW_SPEC_TEST_BOOL_UNSET" + os.Unsetenv(envVar) + defer os.Unsetenv(envVar) + + result := GetBoolFromEnv(envVar, true, nil) + assert.True(t, result, + "GetBoolFromEnv should return defaultValue when env var is not set") +} + +func TestSpec_PublicAPI_GetBoolFromEnv_ReturnsDefault_WhenInvalid(t *testing.T) { + const envVar = "GH_AW_SPEC_TEST_BOOL_INVALID" + os.Setenv(envVar, "not-a-bool") + defer os.Unsetenv(envVar) + + result := GetBoolFromEnv(envVar, true, nil) + assert.True(t, result, + "GetBoolFromEnv should return defaultValue when value cannot be parsed as boolean") +} + +func TestSpec_PublicAPI_GetBoolFromEnv_ReturnsParsedValue(t *testing.T) { + const envVar = "GH_AW_SPEC_TEST_BOOL_VALID" + os.Setenv(envVar, "false") + defer os.Unsetenv(envVar) + + result := GetBoolFromEnv(envVar, true, nil) + assert.False(t, result, + "GetBoolFromEnv should return the parsed boolean value") +} + +func TestSpec_PublicAPI_GetBoolFromEnv_AcceptsNonNilLogger(t *testing.T) { + const envVar = "GH_AW_SPEC_TEST_BOOL_LOGGER" + os.Setenv(envVar, "true") + defer os.Unsetenv(envVar) + + log := logger.New("envutil:spec_test") + + assert.NotPanics(t, func() { + assert.True(t, GetBoolFromEnv(envVar, false, log)) + }, "GetBoolFromEnv should not panic when a non-nil logger is passed") +} + +// TestSpec_PublicAPI_GetStringFromEnv validates the documented behavior of +// GetStringFromEnv as described in the package README.md. +func TestSpec_PublicAPI_GetStringFromEnv_ReturnsDefault_WhenNotSet(t *testing.T) { + const envVar = "GH_AW_SPEC_TEST_STRING_UNSET" + os.Unsetenv(envVar) + defer os.Unsetenv(envVar) + + result := GetStringFromEnv(envVar, "fallback", nil) + assert.Equal(t, "fallback", result, + "GetStringFromEnv should return defaultValue when env var is not set") +} + +func TestSpec_PublicAPI_GetStringFromEnv_ReturnsDefault_WhenEmpty(t *testing.T) { + const envVar = "GH_AW_SPEC_TEST_STRING_EMPTY" + os.Setenv(envVar, "") + defer os.Unsetenv(envVar) + + result := GetStringFromEnv(envVar, "fallback", nil) + assert.Equal(t, "fallback", result, + "GetStringFromEnv should return defaultValue when env var is empty") +} + +func TestSpec_PublicAPI_GetStringFromEnv_ReturnsValue(t *testing.T) { + const envVar = "GH_AW_SPEC_TEST_STRING_VALUE" + os.Setenv(envVar, "value") + defer os.Unsetenv(envVar) + + result := GetStringFromEnv(envVar, "fallback", nil) + assert.Equal(t, "value", result, + "GetStringFromEnv should return the env var value when it is non-empty") +} + +func TestSpec_PublicAPI_GetStringFromEnv_AcceptsNonNilLogger(t *testing.T) { + const envVar = "GH_AW_SPEC_TEST_STRING_LOGGER" + os.Setenv(envVar, "token") + defer os.Unsetenv(envVar) + + log := logger.New("envutil:spec_test") + + assert.NotPanics(t, func() { + assert.Equal(t, "token", GetStringFromEnv(envVar, "", log)) + }, "GetStringFromEnv should not panic when a non-nil logger is passed") +} diff --git a/pkg/parser/github.go b/pkg/parser/github.go index e82ad0ae45c..0140dfd31cd 100644 --- a/pkg/parser/github.go +++ b/pkg/parser/github.go @@ -10,6 +10,7 @@ import ( "strings" "github.com/github/gh-aw/pkg/constants" + "github.com/github/gh-aw/pkg/envutil" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/stringutil" ) @@ -62,12 +63,10 @@ func GetGitHubToken() (string, error) { githubLog.Print("Getting GitHub token") // First try environment variable - if token := os.Getenv("GITHUB_TOKEN"); token != "" { //nolint:osgetenvlibrary - githubLog.Print("Found GITHUB_TOKEN environment variable") + if token := envutil.GetStringFromEnv("GITHUB_TOKEN", "", githubLog); token != "" { return token, nil } - if token := os.Getenv("GH_TOKEN"); token != "" { //nolint:osgetenvlibrary - githubLog.Print("Found GH_TOKEN environment variable") + if token := envutil.GetStringFromEnv("GH_TOKEN", "", githubLog); token != "" { return token, nil } diff --git a/pkg/parser/github_wasm.go b/pkg/parser/github_wasm.go index a91bd83a40f..a2f260a40ac 100644 --- a/pkg/parser/github_wasm.go +++ b/pkg/parser/github_wasm.go @@ -4,14 +4,17 @@ package parser import ( "errors" - "os" + + "github.com/github/gh-aw/pkg/envutil" ) func GetGitHubToken() (string, error) { - if token := os.Getenv("GITHUB_TOKEN"); token != "" { + // Wasm callers do not use the package logger, so pass nil to suppress debug + // logging while still centralizing environment variable access. + if token := envutil.GetStringFromEnv("GITHUB_TOKEN", "", nil); token != "" { return token, nil } - if token := os.Getenv("GH_TOKEN"); token != "" { + if token := envutil.GetStringFromEnv("GH_TOKEN", "", nil); token != "" { return token, nil } return "", errors.New("GitHub token not available in Wasm (set GITHUB_TOKEN or GH_TOKEN environment variable)")