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 exec command strings #5470

Merged
merged 6 commits into from
Nov 22, 2023
Merged

Fix exec command strings #5470

merged 6 commits into from
Nov 22, 2023

Conversation

twelvemo
Copy link
Collaborator

@twelvemo twelvemo commented Nov 22, 2023

What this PR does / why we need it:
Fixes the garden exec command, which wasn't working when given more than two arguments e.g. garden exec backend "ls -la" would fail with executable file not found, while garden exec backend ls succeeds. If not quoting the command with two arguments, garden would interpret the -la as a garden argument and fail as well.

This PR restores the behavior for the command argument as it was in 0.12.x which means that commands are getting split which still fails in cases where there are quoted substrings see #2957.
It also introduces the option to use -- as a separator to specify commands the same way kubectl does it. Using the -- separator results in quoted strings to be interpreted as one argument.
e.g.:
garden exec debian -- /bin/sh -c echo "hello world"

Adding a deprecation warning for using the command argument without the -- separator.

Which issue(s) this PR fixes:

Fixes #2957

Special notes for your reviewer:

Copy link
Contributor

@shumailxyz shumailxyz left a comment

Choose a reason for hiding this comment

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

Thanks for fixing it. I left a comment suggesting to support the old workflow as well as the new one using --.

@twelvemo twelvemo force-pushed the fix-exec-command-strings branch from 26fb76a to 26e7983 Compare November 22, 2023 11:09
@twelvemo twelvemo requested a review from shumailxyz November 22, 2023 13:11
Copy link
Contributor

@shumailxyz shumailxyz left a comment

Choose a reason for hiding this comment

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

Looks great, thanks! 💯

Also thanks for adding the integ tests.

@twelvemo twelvemo added this pull request to the merge queue Nov 22, 2023
Merged via the queue into main with commit 34b07fe Nov 22, 2023
3 checks passed
@twelvemo twelvemo deleted the fix-exec-command-strings branch November 22, 2023 14:02
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.

garden exec splits quoted arguments
2 participants