Skip to content
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

Plugin assumes that bash is present in container #23

Closed
gh2k opened this issue Apr 23, 2018 · 4 comments
Closed

Plugin assumes that bash is present in container #23

gh2k opened this issue Apr 23, 2018 · 4 comments

Comments

@gh2k
Copy link

gh2k commented Apr 23, 2018

The command is run like this:

docker run "${args[@]}" "${BUILDKITE_PLUGIN_DOCKER_IMAGE}" bash -c "${BUILDKITE_COMMAND}"

When using Windows Server containers this doesn't work without some messing around. In my environment bash -c could be safely dropped. I would have expected that when adding the buildkite command to my pipeline file it would be the entire cmd section, rather than wrapped.

bash also isn't present in some minimal Linux containers IIRC (I think Alpine just has 'sh', or at least it used to)

Personally I'd prefer this to drop the bash -c entirely and assume that the pipeline author would add this if required. If there's a reason for this being there that I'm missing, perhaps you could provide a mechanism for overriding the interpreter in the config. For example:

  - command:  build-windows.ps1
    name: Windows build
    plugins:
      docker:
        image: microsoft/windowsservercore:1709
        always-pull: true
        interpreter: powershell -Command
    agents:
      windows: true
@lox
Copy link
Contributor

lox commented Apr 23, 2018

This is a really tricky one. See buildkite-plugins/docker-compose-buildkite-plugin#124 for some more context.

@toolmantim
Copy link
Contributor

We could potentially also skip an interpreter for simple commands?

@lox
Copy link
Contributor

lox commented Jun 18, 2018

We could potentially also skip an interpreter for simple commands?

@toolmantim yeah, that is a possibility. Perhaps we should just error for multi-line commands when there isn't a shell set and don't do it automatically for folks.

@toolmantim
Copy link
Contributor

Fixed in #50 @gh2k. I'll put out a release now. But we're going to do a major version bump to remove the default shell behaviour, and have the shell an opt-in thing, with some detections if people are trying to run bash-isms without having shell enabled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants