Gitlab runner update #26003
Gitlab runner update #26003
Conversation
| @@ -32,6 +32,8 @@ buildGoPackage rec { | |||
| sha256 = "0n8hcj2b1pb95x4bd7fb9ri43vgc4h2dj2v3iiziw2imqjyphfx4"; | |||
| }; | |||
|
|
|||
| patches = [ ./v1-fix-shell-path.patch ]; | |||
There was a problem hiding this comment.
Oh, that patch really applies cleanly to 9.x? Should've tried that. Thanks! 😄
There was a problem hiding this comment.
Yes there has been no change to that file ;) Thanks for the patch anyway 👍
There was a problem hiding this comment.
My problem with that dirty patch is that it breaks gitlab-runner from nixpkgs for non-NixOS environments. I'm trying to convince some colleagues who are proficient in go to implement shell path lookup through the PATH environment variable. So we hopefully get a proper fix upstream soon. :)
There was a problem hiding this comment.
exec's LookPath seems to do what we need here. in case it errors (e.g. b.Shell not found in $PATH), it falls back to the original logic.
far from the perfect solution, but a quick fix for the patch could look something like this:
diff --git a/shells/bash.go b/shells/bash.go
index 839b7781..2b478e1e 100644
--- a/shells/bash.go
+++ b/shells/bash.go
@@ -7,6 +7,7 @@ import (
"gitlab.com/gitlab-org/gitlab-ci-multi-runner/common"
"gitlab.com/gitlab-org/gitlab-ci-multi-runner/helpers"
"io"
+ "os/exec"
"path"
"runtime"
"strconv"
@@ -208,7 +209,11 @@ func (b *BashShell) GetConfiguration(info common.ShellScriptInfo) (script *commo
if info.User != "" {
script.Command = "su"
if runtime.GOOS == "linux" {
- script.Arguments = append(script.Arguments, "-s", "/bin/"+b.Shell)
+ shellPath, err := exec.LookPath(b.Shell)
+ if err != nil {
+ shellPath = "/bin/"+b.Shell
+ }
+ script.Arguments = append(script.Arguments, "-s", shellPath)
}
script.Arguments = append(script.Arguments, info.User)
script.Arguments = append(script.Arguments, "-c", shellCommand)There was a problem hiding this comment.
@elseym Looks reasonable. Are you willing to push this to gitlab runner upstream?
There was a problem hiding this comment.
@bachp eventually, yes. changes like this one belong upstream. imma look into it and open a 'merge request' soon :)
for now, let's just use the patch.
…on NixOS It now tries to find the shell via path first and then falls back to the original behavior.
|
Thanks! |
Motivation for this change
Update to latest upstream release.
Also apply the patch from 1.11 to 9.2.0
Things done
(nix.useSandbox on NixOS,
or option
build-use-sandboxinnix.confon non-NixOS)
nix-shell -p nox --run "nox-review wip"./result/bin/)