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

Fix ordering issue with passthrough parameters #302

Merged
merged 4 commits into from
Nov 18, 2022

Conversation

josecanhelp
Copy link
Contributor

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

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

@tonysm
Copy link
Contributor

tonysm commented Nov 17, 2022

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

Copy link

@devsquad-alvaro-meireles devsquad-alvaro-meireles left a comment

Choose a reason for hiding this comment

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

🚀 🔥

@tonysm tonysm merged commit ebd8758 into main Nov 18, 2022
@tonysm tonysm deleted the fix/issue-299-passthrough-parameters branch November 18, 2022 14:58
@tonysm
Copy link
Contributor

tonysm commented Nov 18, 2022

@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
Copy link
Contributor Author

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

@tonysm
Copy link
Contributor

tonysm commented Nov 18, 2022

@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
Copy link

@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
Copy link
Contributor

tonysm commented Nov 18, 2022

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
Copy link

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! 🚀

@tonysm
Copy link
Contributor

tonysm commented Nov 18, 2022

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

@josecanhelp
Copy link
Contributor Author

Thanks, @tonysm!

@tonysm
Copy link
Contributor

tonysm commented Nov 18, 2022

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

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.

Docker run command parameter order
5 participants