-
Notifications
You must be signed in to change notification settings - Fork 458
fix(lint): eliminate env-coupling in workflow_data, add_interactive_engine, outcomes_command #43506
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
Changes from all commits
825dcc0
8ecf0ab
8c68bbf
c814f15
61f05f1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| package cli | ||
|
|
||
| import ( | ||
| "os" | ||
| "sync" | ||
| ) | ||
|
|
||
| type envLookupFunc func(string) (string, bool) | ||
|
|
||
| var ( | ||
| processEnvLookupMu sync.RWMutex | ||
| processEnvLookup envLookupFunc = os.LookupEnv | ||
| ) | ||
|
|
||
| // SetProcessEnvLookup configures how CLI helpers resolve environment values. | ||
| // Passing nil restores the default process environment lookup. | ||
| func SetProcessEnvLookup(lookup func(string) (string, bool)) { | ||
| processEnvLookupMu.Lock() | ||
| defer processEnvLookupMu.Unlock() | ||
| if lookup == nil { | ||
| processEnvLookup = os.LookupEnv | ||
| return | ||
| } | ||
| processEnvLookup = lookup | ||
| } | ||
|
|
||
| func lookupEnv(key string) string { | ||
| processEnvLookupMu.RLock() | ||
| defer processEnvLookupMu.RUnlock() | ||
| // Intentionally ignore the existence flag to preserve os.Getenv semantics: | ||
| // missing variables and explicitly empty variables are both treated as "". | ||
| value, _ := processEnvLookup(key) | ||
| return value | ||
| } | ||
|
|
||
| func lookupEnvOk(key string) (string, bool) { | ||
| processEnvLookupMu.RLock() | ||
| defer processEnvLookupMu.RUnlock() | ||
| return processEnvLookup(key) | ||
| } | ||
|
Comment on lines
+27
to
+40
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No test coverage for the injectable override
Please add at least a simple test similar to the workflow package pattern — for example, verifying that @copilot please address this. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| //go:build !integration | ||
|
|
||
| package cli | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestSetProcessEnvLookup(t *testing.T) { | ||
| t.Cleanup(func() { SetProcessEnvLookup(nil) }) | ||
|
|
||
| SetProcessEnvLookup(func(key string) (string, bool) { | ||
| env := map[string]string{ | ||
| "PRESENT": "hello", | ||
| "EMPTY": "", | ||
| } | ||
| v, ok := env[key] | ||
| return v, ok | ||
| }) | ||
|
|
||
| t.Run("lookupEnv returns value for present key", func(t *testing.T) { | ||
| assert.Equal(t, "hello", lookupEnv("PRESENT")) | ||
| }) | ||
|
|
||
| t.Run("lookupEnv returns empty string for missing key", func(t *testing.T) { | ||
| assert.Empty(t, lookupEnv("MISSING")) | ||
| }) | ||
|
|
||
| t.Run("lookupEnv returns empty string for explicitly empty key", func(t *testing.T) { | ||
| assert.Empty(t, lookupEnv("EMPTY")) | ||
| }) | ||
|
|
||
| t.Run("lookupEnvOk returns value and true for present key", func(t *testing.T) { | ||
| val, ok := lookupEnvOk("PRESENT") | ||
| assert.Equal(t, "hello", val) | ||
| assert.True(t, ok) | ||
| }) | ||
|
|
||
| t.Run("lookupEnvOk returns empty string and false for missing key", func(t *testing.T) { | ||
| val, ok := lookupEnvOk("MISSING") | ||
| assert.Empty(t, val) | ||
| assert.False(t, ok) | ||
| }) | ||
|
|
||
| t.Run("lookupEnvOk returns empty string and true for explicitly empty key", func(t *testing.T) { | ||
| val, ok := lookupEnvOk("EMPTY") | ||
| assert.Empty(t, val) | ||
| assert.True(t, ok) | ||
| }) | ||
| } | ||
|
|
||
| func TestSetProcessEnvLookupNilRestoresDefault(t *testing.T) { | ||
| t.Cleanup(func() { SetProcessEnvLookup(nil) }) | ||
|
|
||
| SetProcessEnvLookup(func(key string) (string, bool) { | ||
| return "overridden", true | ||
| }) | ||
|
|
||
| SetProcessEnvLookup(nil) | ||
|
|
||
| // After restoring the default, missing keys should return "" / false. | ||
| val, ok := lookupEnvOk("__GH_AW_TEST_KEY_THAT_MUST_NOT_EXIST__") | ||
| assert.Empty(t, val) | ||
| assert.False(t, ok) | ||
| } |
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.
[/tdd]
lookupEnvsilently discards theboolexistence flag. The inline comment explains the intent (matchos.Getenvsemantics), which is correct — but it is worth a test that verifies an explicitly-set empty string ("") is returned as""(not treated as missing), so the semantics stay locked in.💡 Suggested edge-case test
@copilot please address this.