-
Notifications
You must be signed in to change notification settings - Fork 428
fix: force GH_HOST=github.com for action SHA resolution in resolveFromGitHub #40031
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
b9f4153
d589e48
7f8bab0
172d5c8
de113f0
997498a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -223,3 +223,24 @@ func SetGHHostEnv(cmd *exec.Cmd, host string) { | |
| cmd.Env = append(cmd.Env, "GH_HOST="+host) | ||
| } | ||
| } | ||
|
|
||
| // ForceGHHostEnv forces GH_HOST=<host> on the command's environment, overriding | ||
| // any GH_HOST already present in the process environment or cmd.Env. | ||
| // Unlike SetGHHostEnv, this always sets GH_HOST — including for "github.com" — | ||
| // so that a GHE host in the process environment cannot be inherited by the subprocess. | ||
| func ForceGHHostEnv(cmd *exec.Cmd, host string) { | ||
| if host == "" { | ||
| return | ||
| } | ||
| base := cmd.Env | ||
| if base == nil { | ||
|
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. [/tdd] 💡 Suggested testAdd a case to the existing table (or a separate cmd := exec.Command("gh", "api", "/test")
cmd.Env = []string{"GH_HOST=stale.ghe.com", "OTHER=value"}
ForceGHHostEnv(cmd, "github.com")
// assert: exactly one GH_HOST=github.com, no stale entry, OTHER preservedThis would guard the dedup loop and ensure
Contributor
Author
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. Fixed: added |
||
| base = os.Environ() | ||
| } | ||
| filtered := make([]string, 0, len(base)+1) | ||
| for _, e := range base { | ||
| if !strings.HasPrefix(e, "GH_HOST=") { | ||
| filtered = append(filtered, e) | ||
| } | ||
| } | ||
| cmd.Env = append(filtered, "GH_HOST="+host) | ||
| } | ||
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] The cleanup for the
"GH_HOST unset"case doesn't restore the originalGH_HOSTvalue if it was set in the environment before this test ran — thet.Cleanupjust callsos.Unsetenvagain instead of restoring what was there.💡 Suggested fix using os.LookupEnv
This mirrors what
t.Setenvdoes internally, ensuring the test doesn't permanently unsetGH_HOSTfor other tests running in the same process (e.g. in CI whereGH_HOSTmay be set globally).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.
Fixed: the "GH_HOST unset" branch now saves the original value with
os.LookupEnvand restores it (or re-unsets) int.Cleanup, exactly as suggested.