Make 'exec' arguments consistent between k8s and dev/local

[kdmccormick]

Context

The command tutor k8s exec SERVICE COMMAND expects the COMMAND's arguments to be contained within the COMMAND string. For example:

$ tutor k8s exec mysql bash       # No arguments
kubectl exec -i -t --namespace openedx mysql-7cb5f98d7-qqrg4 -- sh -e -c bash
mysql@mysql-7cb5f98d7-qqrg4:/$ exit
exit

$ tutor k8s exec mysql bash -e    # Include '-e' as separate argument: fails 
Usage: tutor k8s exec [OPTIONS] SERVICE COMMAND
Try 'tutor k8s exec -h' for help.
Error: No such option: -e

$ tutor k8s exec mysql 'bash -e'  # Include '-e' within command: succeeds
kubectl exec -i -t --namespace openedx mysql-7cb5f98d7-qqrg4 -- sh -e -c bash -e
mysql@mysql-7cb5f98d7-qqrg4:/$ exit
exit

In comparison, tutor (dev|local) exec SERVICE COMMAND ARGS... expects the COMMAND's arguments to be provided separately, in ARGS:

$ tutor dev exec mysql bash       # No arguments
docker-compose -f /home/kyle/.local/share/tutor/env/local/docker-compose.yml -f /home/kyle/.local/share/tutor/env/dev/docker-compose.yml -f /home/kyle/.local/share/tutor/env/dev/docker-compose.override.yml --project-name tutor_dev exec mysql bash
mysql@5404f46dee7d:/$ exit
exit

$ tutor dev exec mysql bash -e    # Include '-e' as separate argument: succeeds 
docker-compose -f /home/kyle/.local/share/tutor/env/local/docker-compose.yml -f /home/kyle/.local/share/tutor/env/dev/docker-compose.yml -f /home/kyle/.local/share/tutor/env/dev/docker-compose.override.yml --project-name tutor_dev exec mysql bash -e
mysql@5404f46dee7d:/$ exit
exit

$ tutor dev exec mysql 'bash -e'  # Include '-e' within command: fails
docker-compose -f /home/kyle/.local/share/tutor/env/local/docker-compose.yml -f /home/kyle/.local/share/tutor/env/dev/docker-compose.yml -f /home/kyle/.local/share/tutor/env/dev/docker-compose.override.yml --project-name tutor_dev exec mysql bash -e
OCI runtime exec failed: exec failed: container_linux.go:380: starting container process caused: exec: "bash -e": executable file not found in $PATH: unknown
Error: Command failed with status 126: docker-compose -f /home/kyle/.local/share/tutor/env/local/docker-compose.yml -f /home/kyle/.local/share/tutor/env/dev/docker-compose.yml -f /home/kyle/.local/share/tutor/env/dev/docker-compose.override.yml --project-name tutor_dev exec mysql bash -e

Could we make these consistent, without breaking backwards compatibility?

Idea:

• Always word-split the COMMAND. So, exec mysql 'bash -e' would always work.
• Also, always allow additional arguments to be passed using separate ARGS. So ,that exec mysql bash -e would always work.
• If arguments are provided both in the COMMAND itself and in ARGS, then concatenate the argument lists together. For example,
  • tutor (k8s|local|dev) exec 'bash -e' -x would be equivalent to
  • tutor (k8s|local|dev) exec 'bash -e -x' would be equivalent to
  • tutor (k8s|local|dev) exec bash -e -x.
• Make the some change to tutor (local|dev) run for the sake of consistency, even though tutor k8s run doesn't exit.

Acceptance

TBD

[kdmccormick]

Super low priority in my mind, but ready for your eyes nonetheless @regisb.

[regisb]

I agree that the fact that tutor k8s exec mysql bash -e fails is a bug. But I'm less sure about the others...

Always word-split the COMMAND. So, exec mysql 'bash -e' would always work.

This works in the simple cases, but fails with arguments that include spaces. For example:

tutor local exec mysq bash -c "mysql -c 'CREATE USER ...'"

If arguments are provided both in the COMMAND itself and in ARGS, then concatenate the argument lists together.

Again, that would require that we space-split the arguments before passing them to subprocess, and that would break on arguments that include whitespaces.

I think that we should not attempt to do better than what our shells already do, outside of Docker. If you attempt to run "bash -e" in your shell, then the command will fail:

$ "bash -e"
bash -e: command not found

Instead, if people want to pass arguments as strings, then we should encourage them to wrap them in bash -c "...":

bash -c "<my complex command>"

which would then become in tutor:

tutor dev exec lms bash -c "..."

[kdmccormick]

I think that we should not attempt to do better than what our shells already do, outside of Docker.

Good point. Forget my proposed solution.

I agree that the fact that tutor k8s exec mysql bash -e fails is a bug.

So we will fix this 👍🏻 The simplest fix I can image would tweak the tutor k8s exec CLI so that all of these would work:

• tutor k8s exec mysql bash -e
• tutor k8s exec mysql "bash -e"
• tutor [dev|local] exec mysql bash -e

whereas this would continue to fail:

• tutor [dev|local] exec "bash -e"

The end state would be somewhat inconsistent, yet backwards-compatible. Does that sound good? I can provide a PR if it'd make discussion easier.

[kdmccormick]

Will be fixed in overhangio/tutor#637