override shell used for step scripts

[julmb]

This change enables overriding the shell that is used to execute the commands in a step. The motivation for this is that the various Windows images provided by Microsoft do not offer a consistent shell. Some provide powershell, while others provide pwsh, currently making the latter unusable in pipeline steps. See #34 for more details.

After realizing that the proposed changes in #34 were not sufficient, I took a better look at the code. It turns out the drone configuration file format already provides a key shell, available as resource.Step.Shell but not used by drone-runner-docker. The proposed change does not alter the behavior of a step unless it contains a key shell. If it does, this shell is used instead of the default /bin/sh and powershell, respectively.

steps:
- name: test
  image: alpine:3
  shell: /bin/ash
  commands:
  - echo test

steps:
- name: test
  image: mcr.microsoft.com/powershell:nanoserver-ltsc2022
  shell: pwsh
  commands:
  - echo test

I have tried it on Linux and will try it on Windows tomorrow.

[julmb]

I just tested it on windows and was able to use both mcr.microsoft.com/powershell:nanoserver-ltsc2022 (which offers pwsh but not powershell) and mcr.microsoft.com/windows/servercore:ltsc2022 (which offers powershell but not pwsh) within the same pipeline.

Getting images like mcr.microsoft.com/windows/nanoserver:ltsc2022 which only offer cmd to work will probably be more challenging, as the current step script compiler assumes access to some variant of powershell.

[tphoney]

Hey @julmb thanks for this change, we are concerned about this being mostly un-needed for most cases, as this is the first PR for this. However it is needed for this edge case you found, so we want to merge it. I would like to add some docs for this first, would you have a look over them ?

[julmb]

Yes, I realize that this is a fairly specific case, especially since not many people seem to be using Drone on Windows. Nonetheless, it would make life easier in this case. It would also provide some future-proofing, as Microsoft looks to be giving the cross-platform pwsh preferential treatment over the legacy powershell and might eventually deprecate the latter entirely.

I would of course be more than happy to help with documentation.

[julmb]

Looking over the change once more, I realize that maybe we also want to pass the name of the overridden shell to the executing script. Although I am not sure about the role of the file extension here.

Suggested change

	dst.Envs["SHELL"] = "powershell.exe"
	dst.Envs["SHELL"] = sh

I am also not sure if/where a similar thing is done on the Linux side of things.

[julmb]

Should this shell invocation also use the overridden shell? I am not familiar enough with the script compiler to judge whether this would be a good idea or not.

[tphoney]

so looking at what gets eventually gets called is https://github.com/drone/runner-go/blob/0bd0f8fc31c489817572060d17c6e24aaa487470/shell/bash/bash.go#L26

dst.Command = []string{`echo "$DRONE_SCRIPT" | /bin/sh`}

where we pipe the commands into /bin/sh this also needs looked at

[tphoney]

Here are the docs i created for this drone/docs#495

[tphoney]

Hey @julmb i spoke with @bradrydzewski about this, and it gets complicated quickly as you have seen particularly on the linux side. After talking around the problem for a bit we came to the conclusion to limit this to the windows side only for now.
And to limit the shell being used to either powershell or pwsh. (prevents the use of cmd)

we can then add a lint check into https://github.com/drone-runners/drone-runner-docker/blob/master/engine/linter/linter.go#L72
that looks something like this:

if pipeline.Platform.OS == "windows" {
  if step.Shell != "" && step.Shell != "powershell" && step.Shell != "pwsh" {
    return errors.New("linter: on Windows 'shell' used in a step must be 'powershell' or 'pwsh' ")
  }
}

what do you think ?

[julmb]

Hey @julmb i spoke with @bradrydzewski about this, and it gets complicated quickly as you have seen particularly on the linux side. After talking around the problem for a bit we came to the conclusion to limit this to the windows side only for now.

That sounds reasonable. It is a little unfortunate that it cannot be done in a more general way, but I agree that it is really only needed on Windows, so this should cover the main use cases.

And to limit the shell being used to either powershell or pwsh. (prevents the use of cmd)

we can then add a lint check into https://github.com/drone-runners/drone-runner-docker/blob/master/engine/linter/linter.go#L72 that looks something like this:

if pipeline.Platform.OS == "windows" {
  if step.Shell != "" && step.Shell != "powershell" && step.Shell != "pwsh" {
    return errors.New("linter: on Windows 'shell' used in a step must be 'powershell' or 'pwsh' ")
  }
}

This covers all the use cases I have come across so far. However, I am a little worried that a different shell name might be needed at some point. It would then be very frustrating for users to find out that a feature that solves their problem has already been implemented but artificially restricted to not work for their case.

I personally favor warning users about doing questionable things rather than preventing them outright. However, this is a matter of design philosophy, so I will leave that to the Drone developers to decide.

what do you think ?

I would be okay with doing things the way you proposed. How do we proceed? Should I adjust the pull request to only cover the Windows side? (I'm new to contributing to FOSS, so I'm not familiar with how things are done).

[viceice]

This also helps with #13

[julmb]

I know it has been a long time, but I am still using my own patched version of drone-runner-docker for this and it makes things messy and updating difficult. Is there any chance of getting this merged? What is left to be done? Is there anything I can do to help?