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

Run all commands through a shell interpreter #124

Closed
wants to merge 3 commits into from

Conversation

lox
Copy link
Contributor

@lox lox commented Apr 16, 2018

Multiple commands get passed through as new-line delimited commands. Docker needs a single command, so this currently doesn't work. This sets a standard command to execute $BUILDKITE_COMMAND in of /bin/sh -e -c, you can provide an alternative via the shell parameter.

The downside is that this are:

  • Adds a layer of shell execution to your docker-compose runs
  • Requires /bin/sh in your container

We could possibly only do this if there is multiple commands, but in some ways that just makes it more magical IMO.

Copy link
Contributor

@toolmantim toolmantim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is going to be a good change! Even though it feels strange in Docker land.

@@ -8,6 +8,7 @@ run_service="$(plugin_read_config RUN)"
container_name="$(docker_compose_project_name)_${run_service}_build_${BUILDKITE_BUILD_NUMBER}"
override_file="docker-compose.buildkite-${BUILDKITE_BUILD_NUMBER}-override.yml"
pull_retries="$(plugin_read_config PULL_RETRIES "0")"
shell="$(plugin_read_config SHELL "/bin/bash -c")"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have -e in the vanilla bootstrap command hook, to ensure that commands don't keep executing if one fails

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we just default to /bin/sh, which on most systems will just be bash anyway?

Copy link
Contributor Author

@lox lox Apr 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, I reckon you are totes correct on both calls. As always you are a fine human to collaborate with.

- echo \$ALPACAS
- pwd
- echo "llamas rock too"
shell: "/bin/sh -c"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to be under the plugin block?

@lox lox force-pushed the add-shell-config-for-multiple-commands branch from d15eb8b to 69aa558 Compare April 16, 2018 04:58
@lox
Copy link
Contributor Author

lox commented Apr 16, 2018

Ugh, so how do we handle containers without a shell in them?

@toolmantim
Copy link
Contributor

Yeah, that's the same trouble I had trying to use the docker plugin with the plugin-linter and trying to pass it command line args. Hrmmmm…

@lox
Copy link
Contributor Author

lox commented Apr 16, 2018

shell: ~ ?

Copy link

@keithpitt keithpitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have any strong feels, happy with what ever you choose!

@toolmantim
Copy link
Contributor

shell: false ?

@lox
Copy link
Contributor Author

lox commented Apr 16, 2018

I guess you'd never want to use /bin/false as your shell 😅

Copy link
Contributor

@toolmantim toolmantim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shell false looks good!

I think we're going to have to update the way we actually pass the command to run if shell is false (4ed5ea4#diff-e0e3e4340eb68e96c2657414fb30f290R121). I don't think we want to quote it.

README.md Outdated
@@ -293,7 +293,7 @@ The default is `false`.

### `shell` (optional, run only)

The shell that is used to invoke commands. This is so that multiple commands can be interpretted.
The shell that is used to invoke commands. This is so that multiple commands can be interpretted. if `false` is specified, the command is invoked directly.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shell that is used to invoke commands. Set to false to disable running the commands via a shell (useful for images that expect command arguments to passed directly).

@lox lox force-pushed the add-shell-config-for-multiple-commands branch from 16a008c to 32b4537 Compare June 10, 2018 07:57
@lox
Copy link
Contributor Author

lox commented Aug 2, 2018

I'm going to close this for now, I think some more thinking is needed around how to handle multiple commands and containers without shells.

@lox lox closed this Aug 2, 2018
@toote toote deleted the add-shell-config-for-multiple-commands branch May 23, 2023 19:51
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

Successfully merging this pull request may close these issues.

3 participants