tighten / takeout

Docker-based development-only dependency manager. macOS, Linux, and WSL2-only and installs via PHP's Composer... for now.
MIT License
1.59k stars 83 forks source link

Fix ordering issue with passthrough parameters #302

Closed josecanhelp closed 1 year ago

josecanhelp commented 1 year ago

Passthrough parameters were being added to the end of the run commands, after the image name and tag. However, those parameters should be passed through to the command before that. The simplest solution, in my opinion, is to reorder those strings at the base command enable function. I tested this with passing custom ports as well as custom volumes.

This PR would resolve #299 as well as #301.

devsquad-alvaro-meireles commented 1 year ago

Hey, @mattstauffer @tonysm ! Can you review that please?

tonysm commented 1 year ago

I'll check it tomorrow (it looks good, but the build is not passing)

tonysm commented 1 year ago

@josecanhelp @devsquad-alvaro-meireles one question: I think the original feature allowed passing arguments to the container process, but with this change, the passthrough feature is now for docker run options, or am I reading this wrong? 🤔

before:

docker run image {container-args}

after:

docker run {docker-run-options} image
josecanhelp commented 1 year ago

@tonysm that seems right. I thought the pass through was for docker run commands, not container processes.

tonysm commented 1 year ago

@josecanhelp maybe we should offer a new option for the run command options, something like:

takeout enable mysql --run="--restart unless-stopped -e 'LOREM=IPSUM'" --  -hsome.mysql.host -usome-mysql-user -p

where it would generate a command like:

docker run {other-options} --restart unless-stopped -e 'LOREM=IPSUM' mysql -hsome.mysql.host -usome-mysql-user -p
devajmeireles commented 1 year ago

@josecanhelp @devsquad-alvaro-meireles one question: I think the original feature allowed passing arguments to the container process, but with this change, the passthrough feature is now for docker run options, or am I reading this wrong? thinking

before:

docker run image {container-args}

after:

docker run {docker-run-options} image

@tonysm The main idea that I've asked in #301 was the ability to set a custom volume to be shareable with the container, so we can easily share a database dump to the MySQL container

tonysm commented 1 year ago

We could offer the extra --volume flag in the enable command, but my issue with that is that we would eventually be adding more and more docker run options to the list of options, and the docker run command "has more options than any other docker command" (stated by the docs).

So, I'd be more inclined to offer a single option for power users to pass any docker run options to the docker run command underneath. Thoughts?

devajmeireles commented 1 year ago

So, I'd be more inclined to offer a single option for power users to pass any docker run options to the docker run command underneath. Thoughts?

Sounds good! :rocket:

tonysm commented 1 year ago

Alright, so I'll revert this change and introduce a new --run option to the enable command to pass docker run options.

josecanhelp commented 1 year ago

Thanks, @tonysm!

tonysm commented 1 year ago

The v2.2.0 tag was released with the new --run= option. \cc @josecanhelp @ajmeireles